Fix selections with non-void non-editable focus

[TyMick]

Description

Currently, when I drag my browser selection focus into a contentEditable={false} DOM element that's being rendered by a non-void Slate element renderer, Slate's selection doesn't get updated. At times, this causes non-highlighted text to be deleted when pressing Backspace (first example below). At other times, this causes a Backspace press to crash the page (second example below).

The fix in this PR is to set the focus point to the edge of the nearest Slate leaf when the selection focus is in a non-void, non-editable node, instead of disregarding such selections.

Issue

Fixes #5714

Example

These examples use https://www.slatejs.org/examples/check-lists, where the “non-void, non-editable” node is this <span>.

Incorrect deletion

Before

After

Page crash

Before

After

Checks

• The new code matches the existing patterns and styles.
• The tests pass with yarn test.
• The linter passes with yarn lint. (Fix errors with yarn fix.)
• The relevant examples still work. (Run examples with yarn start.)
• You've added a changeset if changing functionality. (Add one with yarn changeset add.)

[changeset-bot]

🦋 Changeset detected

Latest commit: f4654c3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
slate-react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[TyMick]

I'm not sure focusNodeSelectable is still the best name for this variable, since the non-void non-editable nodes still aren't selectable. ReactEditor.toSlateRange just changes the focus to a node that is selectable now. Can't think of a better name at the moment, though.

[TyMick]

Also, this PR only fixes non-selectable focus points now, but should non-selectable anchor points be fixed as well? It wouldn't be necessary for my project, but conceivably, if a non-void non-editable node contained non-interactive text, it would be easy to make a browser selection with the anchor inside that node and the focus outside it.

[TyMick]

Fixing non-selectable anchor points would add extra complexity in that we'd have to decide what to do when a selection starts and ends in the same non-selectable node... This PR is already an improvement on the existing behavior as-is, so maybe fixing anchors can be saved for a future PR? 😅

[TyMick]

Changed the variable name to focusNodeInEditor with 98630d1.

[dylans]

Thanks @TyMick I will look at this one sometime this week, and try to answer your open questions as well.

[TyMick]

Thanks, Dylan!

[TyMick]

I tried adding a Jest regression test for this, but my attempts to create a selection range with Testing Library's UserEvent.pointer weren't able to affect editor.selection, which remained null at the end of this test:

test('fixes DOM selections that end in non-editable nodes', async () => {
  const user = userEvent.setup()
  const editor = withReact(createEditor())
  const initialValue = [
    {
      type: 'check-list-item',
      checked: false,
      children: [{ text: 'Test item' }],
    },
  ]

  render(
    <>
      <div>Something outside the editor</div>
      <Slate editor={editor} initialValue={initialValue}>
        <Editable renderElement={CheckListItemElement} />
      </Slate>
    </>
  )

  await user.pointer([
    // Mouse down in the middle of the checklist item text
    { keys: '[MouseLeft>]', target: screen.getByText('Test item'), offset: 4 },
    // Move mouse outside editor
    { target: screen.getByText('Something outside the editor'), offset: 2 },
    // Move mouse to the checkbox
    { target: screen.getByRole('checkbox') },
    // Mouse up
    { keys: '[/MouseLeft]' },
  ])

  await waitFor(() => expect(editor.selection).toBeTruthy())
})

function CheckListItemElement({ attributes, children, element }: any) {
  const { checked } = element
  return (
    <div {...attributes}>
      <span contentEditable={false}>
        <input type="checkbox" checked={checked} />
      </span>
      <span suppressContentEditableWarning>{children}</span>
    </div>
  )
}

I think this may not be testable without adding an end-to-end framework like Playwright.

[dylans]

I tried adding a Jest regression test for this, but my attempts to create a selection range with Testing Library's UserEvent.pointer weren't able to affect editor.selection, which remained null at the end of this test:

test('fixes DOM selections that end in non-editable nodes', async () => {
  const user = userEvent.setup()
  const editor = withReact(createEditor())
  const initialValue = [
    {
      type: 'check-list-item',
      checked: false,
      children: [{ text: 'Test item' }],
    },
  ]

  render(
    <>
      <div>Something outside the editor</div>
      <Slate editor={editor} initialValue={initialValue}>
        <Editable renderElement={CheckListItemElement} />
      </Slate>
    </>
  )

  await user.pointer([
    // Mouse down in the middle of the checklist item text
    { keys: '[MouseLeft>]', target: screen.getByText('Test item'), offset: 4 },
    // Move mouse outside editor
    { target: screen.getByText('Something outside the editor'), offset: 2 },
    // Move mouse to the checkbox
    { target: screen.getByRole('checkbox') },
    // Mouse up
    { keys: '[/MouseLeft]' },
  ])

  await waitFor(() => expect(editor.selection).toBeTruthy())
})

function CheckListItemElement({ attributes, children, element }: any) {
  const { checked } = element
  return (
    <div {...attributes}>
      <span contentEditable={false}>
        <input type="checkbox" checked={checked} />
      </span>
      <span suppressContentEditableWarning>{children}</span>
    </div>
  )
}

I think this may not be testable without adding an end-to-end framework like Playwright.

We do actually have Playright in there, just not too many tests yet.

[TyMick]

Oh perfect, I should've looked around more! Taking a look at check-lists.test.ts now.

[TyMick]

Nope. I have spent hours trying to

1. mouse down in the first checklist item's text
2. move mouse into the paragraph text above it
3. move mouse into the checkbox
4. mouse up
5. press Backspace

in a Playwright test, but the behavior is far too flaky to be able to reliably create the same text selection by dragging the mouse. And I can't reproduce the bug with just the keyboard. Can't think of any other way to test it.

[dylans]

@TyMick I appreciate you trying. These interactions are not easy to test.