Undoing inline marks doesn't work as expected

[ctbarna]

Do you want to request a feature or report a bug?

Feel free to close this report if it's a duplicate – I searched "undo" but nothing seemed to quite fit. I am not currently developing with Slate.js but discovered a reproducible bug in the example editor.

What's the current behavior?

Tested in Chrome 58 on Mac

1. Visit http://slatejs.org/#/rich-text
2. On the top paragraph of the editor, select "ch text, mu"
3. Hit delete.
4. Hit cmd-z.

Current behavior: The re-done text is "mu ch"

What's the expected behavior?

The editor should restore text to the original state: "ch text, mu"

[erquhart]

@ianstormtaylor just wondering if you had thoughts on this and issues like it (selecting text and applying transforms via keydown often applies the transform to the previous selection, for another example). It feels like a core stability issue with how selections work, wanted to know if you have an idea of what's going on or if these issues are undiagnosed.

[ianstormtaylor]

Hey @erquhart I haven't looked into what is causing this, but if you do I'd love to know and then happy to discuss it with you.

[erquhart]

Yep, I'll probably have to as our implementation moves forward, definitely intend to contribute. I was more fishing for "it's probably x" from you as the architect, having the most intimate knowledge of the codebase, that's all.

[ianstormtaylor]

In the case of this specific issue, I'd guess that the "operations" aren't expressing their inverses properly, resulting in a bad undo. But it sounds like you're talking about a different case?

[erquhart]

Some of the selection bugginess seemed to maybe share the same cause, so I was trying to determine if there's a centralized ticket or effort for that cause, but it doesn't sound like there's a single underlying issue. Just learned, for example, about trackpad dragging having a 300ms timeout enforced at the OS level for Mac, which botches quick select/backspace maneuvers.

I'll probably revisit this once we get through our initial implementation.

[erquhart]

Adding this as it's probably related (botched undo operation with marked text), brought up by another developer in Slack:

1. Apply a mark to some text, e.g. bold, but leave some text without the mark
2. Select all text and toggle the mark (it will be removed from all text
3. Undo

Expected: previously bold text should be bold, and non-bold text should not be bold Actual: all text is bold

[kgdev]

How hard is it to fix this?

[erquhart]

Hard to say, you'd need to debug history around marked node operations, specifically for undo since redo appears to be okay.

[kgdev]

In Transforms.removeMarkAtRange,

adding the following logic seems to solve the problem:

      const ranges = text.getRanges()
      let rangeIndex = 0
      ranges.forEach(range => {
          const {marks, text} = range
          const rangeLength = text.length

          if (marks.has(mark)) {
              const start = Math.max(index, rangeIndex)
              const end = Math.min(index + length, rangeIndex + rangeLength)

              if (start < end) {
                transform.removeMarkByKey(key, start, end - start, mark, { normalize })
              }
          }

          rangeIndex += rangeLength
      })