Slate: Arrow navigation issue with single-character text node adjacent to inline element

[dmarkow]

Do you want to request a _feature_ or report a _bug_?

Bug

What's the current behavior?

https://codesandbox.io/s/slate-reproductions-qdi9i

Click anywhere in the first link. Use the right/left arrow keys to move around. You cannot move the cursor to the left of the first x or the right of the last x. Clicking before/after them works fine though.

If the text nodes have more than 1 character, they work fine (see the second editor in the example)

Slate: 0.57.1
Browser: Chrome
OS: Mac

What's the expected behavior?

Arrow keys should navigate to the end of the line

[dmarkow]

Actually, it seems to affect any adjacent nodes, not just inline. So for example, have ABC where the B is bolded, doesn't allow arrow navigation before the A or after the C.

[davetodd]

I can also reproduce this on the rich text example at https://www.slatejs.org/examples/richtext.

I've tracked this down as far as Editor.positions(). That function doesn't return the { offset: 0, path: [0,0] } position if the first node is only a single character. It's pretty easy to create a failing unit test for this, but it's not obvious to me how to fix it.

As far as I can tell, the previous node is "consuming" the first/last node's text, such that this line causes it to short-circuit before returning the final position: https://github.com/ianstormtaylor/slate/blob/master/packages/slate/src/interfaces/editor.ts#L1065

[majelbstoat]

It is also the case that if you take the steps in the Rich Text example to :

• start in an empty paragraph
• type ABC
• bold B
• move cursor between B and C
• press Enter

the paragraph splits, but the cursor stays on the same line. Feels like the same root cause?

[beorn]

I also discovered this bug, I think, and made a different test scenario with debug info: https://codesandbox.io/s/slate-reproductions-wbub5

I've found one bug that Chrome, Firefox, Safari shares, and one that only Chrome and Firefox has.

My debug output also includes a list of all positions, and it doesn't look like the positions are missing, but rather there's some off-by-one error when iterating over the positions, both ways.

[beorn]

I can also reproduce this on the rich text example at https://www.slatejs.org/examples/richtext.

I've tracked this down as far as Editor.positions(). That function doesn't return the { offset: 0, path: [0,0] } position if the first node is only a single character. It's pretty easy to create a failing unit test for this, but it's not obvious to me how to fix it.

As far as I can tell, the previous node is "consuming" the first/last node's text, such that this line causes it to short-circuit before returning the final position: https://github.com/ianstormtaylor/slate/blob/master/packages/slate/src/interfaces/editor.ts#L1065

@davetodd
The PR above (#3644) does fix the case where a position might be missed at the end. Can you supply a test case where the position is missed at the beginning?

The other issues mentioned here - such as the ones I outlined in my https://codesandbox.io/s/slate-reproductions-wbub5 - are probably not related since they behave differently for different browsers, and Editor.positions() does not interact with the DOM/browsers, it only operates on Slate nodes. Maybe related to #3148.

[beorn]

I tracked down two other related problems:

(1) UNIT=CHARACTER FOR MOVEMENTS
Moving the cursor around results in a Transforms.move() with unit='character' (slate/transforms/selection.ts), which if the neighboring nodes doesn't have any characters will not result in any move. Is there any reason why we don't default to unit='offset' for this and other keyboard input stuff (like delete* transforms)?

(2) CURSOR (SELECTION) IS RESET TO MATCH BROWSER'S BUGGY BEHAVIOR
Due to browser bugs (#3148 etc) Editable (slate-react/editable.ts) resets selection upon most DOM inputevents to be consistent with what the browser believes the selection should be, even though the browser is buggy.

That is, where browser shows the cursor and where Slate inserts text will be visually consistent. But the user's cursor movements will be somewhat needlessly hampered by the browser's bugs; it makes it impossible to use Slate to navigate to positions that the browser cannot display.

I think we should have at least an option to disable this behavior. The browser bugs seem to impact where the cursor is visually shown, and if you don't add a lot of spacing around inline elements, whether the cursor is at the start of an inline or at the end of the node before that is visually indistinguishable; they are collapsed to the same visual location. I found using visual indicators that don't adjust the spacing to be quite effective at communicating "is the cursor in this inline or not", for example changing the background color of the inline if it's focused & selected.

(3) END-OF-INLINE OVERRIDE IN Editor.insertText
Also to account for the browser issues insertText in slate/create-editor will disregard the insertion point and insert text outside the inline if the insertion point was at the end of the inline. This also needs to be disabled or overridden to enable inserting into the end of the inline. (See #3260 #3260)

Summary: I think PR #3644, together with setting unit='character' on at least Transforms.move(), disabling the selection-override in Editable, and disabling the end-of-inline override in insertText will take care of the bugs described here with (as far as I can see) manageable downside.

[BrentFarese]

For number (3), perhaps we can make the default behavior the way it is now and simply include an option to disable that behavior if the Slate user wants to? In our implementation, and in many modern editors, the Editor.insertText behavior for inlines is correct and you can't type at the end of the inline but I have seen use cases where Slate users don't want that behavior.

[beorn]

@BrentFarese Fortunately, it is relatively easy to override Editor.insertText in your plugin to create 'sticky inlines', but you do end up duplicating half of the logic that's in the stock Editor.insertText, which is not ideal (see example below). Sadly, (1) and (2) are not easy to fix without updating SlateJS code, and without those, cursor movement and editing of inline elements is still problematic - maybe even more so due to inconsistencies caused by (2).

export function withCoreOverrides(editor) {
  const { insertText } = editor

  editor.insertText = (text, options = {}) => {
    const { selection, marks } = editor
    let { at = selection } = options
    if (Range.isRange(at) && !Range.isCollapsed(at)) {
      return insertText(text, options)
    }
    if (marks) {
      const node = { text, ...marks }
      Transforms.insertNodes(editor, node)
    } else {
      const { path, offset } = at.anchor
      editor.apply({ type: 'insert_text', path, offset, text })
    }
    editor.marks = null
  }
  return editor
}

[BrentFarese]

Yes that's true people can write their own plugin of insertText (a plugin can be written the other way too so you can type at the end of an inline) but the change in (3) would be a breaking change. I would suggest we do something more like insertText: (text: string, enableInlineTyping?: boolean) where the enableInlineTyping flag can be used within insertText to allow typing at the end of an inline. The enableInlineTyping flag can be defaulted to false so it's optional and has no impact on those who rely on this feature already.

[beorn]

Regarding ways to override insertText, there was some discussion on this in the original implementation #3260, but the code was later changes after this discussion too.

Also interesting wrt the Editor.positions refactor and inlines in general, is the discussion in #2865 - specifically this comment - regarding having "fake" insertion points. It would be interesting to have a discussion about this.

[davetodd]

The PR above (#3644) does fix the case where a position might be missed at the end. Can you supply a test case where the position is missed at the beginning?

@beorn The test you have in your PR is actually the one I was thinking of, though if you wanted to be extra complete, you could swap a + b so that the inline is the first element. Both cases worked when I tested your branch locally. The bug + your fix also applied to a single character adjacent to another leaf, rather than an inline, which is 👍

I did notice while testing this that if you insert a break before a new leaf, the cursor remains on the original line. But if you insert a break before an inline, the cursor moves to the new line. Here's a fork of your sandbox that shows that: https://codesandbox.io/s/slate-reproductions-dfgd8 ...it looks like the positions array is the same in both cases, so this seems pretty unrelated and more likely related to Transforms.splitNodes().