Retrospective: Adding indentation in the Markdown editor breaks many other things

The Problem

There is no way to indent code in the Markdown editor. This is frustrating when writing nested lists or code blocks. At this point, I just write my notes in VS Code and paste them into GitLab when I'm done.

The Solution

!28914 (merged) added the ability to (un)indent using Control/Cmd [ and Control/Cmd ], as well as smart indents like in IDEs.

The Problem with the Solution

It introduced several regressions relating to how the Markdown editor (specifically gl_form.js) interfaces with other code:

  1. A custom undo/redo stack was implemented. However, sometimes an undo/redo would change the text in the textarea but not change the state of the container form, so you could do an undo, submit, and the old text would be submitted.
  2. Typing a @username or ~label followed by Enter would sometimes apply the auto-suggesion but also insert a newline
  3. The editor would sometimes not expand to fit new content.
    • Oddly, I did not encounter this regression during testing, even though I used inputs that should have triggered this regression.

Regressions

The MR ended up being reverted in !31391 (merged).

Working on !28914 (merged) revealed several technical and process issues, which I'll try to summarize here.

Technical Issues

Technical Debt in the Markdown Editor

The Markdown editor includes JS, jQuery, Vue, and HAML code. Some components, like the notes editor on issues and MRs, are Vue-ified. Other parts, like the wiki page editor, are still in HAML. The shared editing logic resides in gl_form.js, which is jQuery/JS.

In short, gl_form.js and friends are incredibly fragile.

This makes it incredibly difficult to implement new features in the editor. Moreover, the editor is essentially a <textarea> with features nailed onto it from several different sources. This is the primary reason for the regressions in !28914 (merged). Specifically:

  1. Regression 1 was due to the undo-redo logic in gl_form.js not always communicating with the backing Vue app, which maintained its own event listeners and state separate from the textarea.
  2. Regression 2 was due to misprocessing keydown events when using a third-party autocomplete solution.
  3. Regression 3 was due to captured keydown events not triggering auto-sizing of the textarea sometimes.

Annoyingly, some of these regressions, which are present on issues/MRs, don't exist on wiki pages, because wiki pages don't use Vue.

#63183 (moved) suggests refactoring the Markdown editor into its own re-usable component, or using a third-party editor like Ace, Monaco, etc.

Cross-Browser Undo-Redo Stack

Changing a textarea's contents programmatically can kill the browser's native undo stack. There is no undo-redo API for JS, and I did not find existing solutions to work well in all our supported browsers. Therefore, I decided to implement a custom undo-redo stack that bypasses the native one completely.

Worth mentioning: On gitlab.com, certain operations, like using any of the buttons at the top-right of the Markdown editor, also kill the native undo stack. @gtsiolis on Slack: "The undo and redo functionality inside the markdown editor has been problematic since forever"

Mousetrap

We use an old version of Mousetrap to capture keyboard shortcuts. This version doesn't allow listening for keyboard events from a single element - only globally. We get around this using a hack that is increasingly difficult to maintain the more it is used. Unfortunately, upgrading Mousetrap breaks existing code, for reasons yet unclear.

!28914 (merged) implemented a new function, keystroke(), that gets around this hackiness by comparing a KeyboardEvent to a desired key chord. It was cleaner at the time, but we should move to the new Mousetrap version as soon as we can.

Related issues: #63182 (moved) and #64246 (moved).

The Range API

The two major functions of interest from the JS Range API are setRangeText() and setSelectionRange(). The compatibility of these is very poorly documented. IE/Edge don't support setRangeText() at all, and JSDOM contains a bug in its implementation of setRangeText().

An alternative, execCommand() on contentEditable elements, also isn't very well-supported across all browsers.

Process Issues

Testing

We are very good at writing unit tests with Jest, but we're in a weird place where we don't have a solid solution for cross-browser integration tests. Why?

!28914 (merged) has comprehensive unit tests, but since its regressions were caused by the way gl_form.js interacts with other code, they would sooner have been caught by integration tests. A good chunk of dev time was also spent getting IE/Edge to work, which both have their own bugs that had to be addressed.

Async Collaboration and UX-Readiness

The original issue (#25070 (closed)) was marked as ~"UX ready" two to three years ago. However, there was considerable back-and-forth with our UX folks during development in !28914 (merged). Our async philosophy made it so that it took several days to agree on a thing. Formerly, I took this issue because it was a pain point for me and it looked like a moderately-easy fix, but it quickly spiralled into something 10x more complex than I thought.

Repeated iterations in the same MR and prolonged reviews led to several rebases and conflicts. By the end, the commit history of the CE and EE MRs was funky and difficult to reason about.

Supernatural Issues

I'm now convinced that this feature is cursed 👻

@pslaughter:

this is a notoriously flaky feature in a notoriously flaky domain 🤷...

What Did We Learn?

  • Technical debt and legacy HAML/Vue and JS/jQuery code makes things very fragile and blocks development of new features
  • We should automate cross-browser testing (Browserstack?)
  • If we still want to roll out this feature, we need to have solid UX right off the bat and we should really consider looking into #63183 (moved).

cc @gweaver @donaldcook

Edited by Martin Hanzel