Fix(editable component): reselect the range created by triple click #4455

bytrangle · 2021-08-16T10:03:35Z

Description
When triple clicking inside the editable component of Slate, it doesn't select the correct block. This poses two problems when Slate is to be used as a rich text editor:

Give false visual feedback, e.g. the wrong toolbar button will be highlighted which leads to confusion
Unable to apply new style to the selected block only.

This PR tries to select the correct block that users triple click on.

Issue
Fixes #4329

Example

Before the fix:

After the fix:

Context

Given a node at path [1].

const editor = {
  children: [
    { // first node
    }
    {
      type: 'paragraph', // the node that receives triple click
      children: [
        { text: "Since it's rich text, you can do things like turn a selection of text " }, // path: [1, 0]
        { text: "bold", bold: true }, // path: [1, 1]
        { text: ", or add a semantically rendered block quote in the middle of the page, like this:" // path: [1, 2]
      ]
    }
  ]
}

Suppose we triple click just before the letter "S" of the paragraph.

Inside the onClick event check if there is triple click with event.detail === 3.
By design, the node that users click on will be the text node at path [1, 0]. Call Editor.above() to retrieve the closest element ancestor of that text node.
If the element exists, get all the children of the element. Create an array of their respective path. It should be
[[1, 0], [1, 1], [1, 2]]
Create a range whose anchor is the first child and focus is the third child. Its anchor offset is 0 (because the selection starts from the very start of the block) and focus offset is the end point of the third child (82) in this case.
Calling Transforms.select(range) to select that range.

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 · 2021-08-16T10:03:37Z

⚠️ No Changeset found

Latest commit: 23405b8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

dylans

Thanks, this change seems reasonable to me. I've made a few minor suggestions.

dylans · 2021-08-16T11:30:59Z

packages/slate-react/src/components/editable.tsx

+                // Check if user clicks thrice
+                if (event.detail === 3) {
+                  // Check the closest element block above the selection
+                  const elem = Editor.above(editor, {


Might be clearer if it was const block rather than const elem, since it's not returning a match if it's not a block?

dylans · 2021-08-16T11:31:48Z

packages/slate-react/src/components/editable.tsx

+                    match: n => Editor.isBlock(editor, n),
+                  })
+                  if (elem) {
+                    // Get all the children of the element above


Suggested change

// Get all the children of the element above

// Get all the children of the block element above the selection

dylans · 2021-08-16T11:37:42Z

packages/slate-react/src/components/editable.tsx

+                    // Get all the children of the element above
+                    const children = Node.children(editor, elem[1])
+                    const childrenPath = []
+                    // Loop through the generator of element children and create a list of their respective paths.


Please run the linter on this file, this comment and a few below likely exceed the line length rules. (Note it's not currently complaining, but I might still suggest getting them to be 80 characters wide.

bytrangle · 2021-08-16T12:58:24Z

Thanks, this change seems reasonable to me. I've made a few minor suggestions.

Thank you, I will fix them tomorrow.

But I've noticed that when I triple click on the second paragraph of the Rich Text example, the Quote Block button is highlighted for a split second and then it's not highlighted.

Do you experience the same?

bytrangle · 2021-08-17T09:59:22Z

Hello @dylans ,
I've discovered that when I triple click on a block, the Editor.nodes() method is supposed to grab a generator of nodes at the current selection, but somehow it always includes the node below the clicked node.

I've traced the problem down to Slate's transform operator.

This is what happens on the third click:

Between createDraft and finishDraft, the selection object has been reassigned:

transform(editor, op): {
  // ...
  let selection = editor.selection && createDraft(editor.selection)
  try {
    selection = applyToDraft(editor, selection, op)
  } finally {
      // ...
      if (selection) {
        editor.selection = isDraft(selection) // this evaluates to true
          ? (finishDraft(selection) as Range)
          : selection
    }
  }
}

I think the culprit is probably the helper function applyToDraft(), so please allow me a few more days to figure it out.

dylans · 2021-08-17T14:34:46Z

@bytrangle thanks, I appreciate you digging into this further.

xenostar · 2021-08-24T01:53:07Z

Just wanted to chime in and say I had some time to test this fix today and it is looking really good. This problem has been plaguing Slate for along time and I'm really looking forward to this fix!

xenostar · 2021-08-24T20:09:34Z

packages/slate-react/src/components/editable.tsx

-                const start = Editor.start(editor, path)
-                const end = Editor.end(editor, path)
+                // Check if user clicks thrice
+                if (event.detail === 3) {


Have you thought about setting this to event.detail >= 3 instead? I noticed when trying a similar approach to this fix locally that if you spam click many times, event.detail continues to increment so this wouldn't be fired once you go past 3 clicks.

… click" Reason: attaching a handler for `onClick` event is no longer needed.

Reason: Triple clicking an element in Chrome will falsely set the focus node as the next sibling node with focusOffset 0

dylans · 2021-08-25T11:59:14Z

@bytrangle to clarify, is this really only an issue in Chrome/Chromium-based browsers? I ask as it is a very different PR from your original. Thanks!

bytrangle · 2021-08-26T10:46:57Z

@dylans
AFAIK, Firefox doesn't have this problem. You can checkout this JsFiddle in both Chrome and Firefox. When you open the console and run the script, then triple clicking the first paragraph, you can see that Chrome will identify the focusNode as the next block (a quoteblock), while Firefox is able to get the focusNode as the paragraph.

The triple-click "bug" is mentioned as a Chromium issue here. I think it's not so much an issue with Chrome, but an issue with the triple-click behaviour not specified in the spec.

bytrangle · 2021-08-26T10:50:55Z

Hello @xenostar . Thanks for chiming in. I've used a new approach that doesn't require checking for event.detail. The reason is Slate calls for window.selection() way before the click event is fired. So it will get the incorrect focusNode and send highlight the wrong toolbar button before I can do anything in the click event handler.

Hope that makes sense. Tell me what you think.

dylans

Thanks @bytrangle this looks great and has my approval. I'll leave it open for a couple more days in case there's further feedback or anything I might have missed.

bytrangle · 2021-08-27T10:00:06Z

@dylans Thank you, and actually that's a good call because I've written an e2e test for this triple-click behavior.

It passed, but it opened up a torrent of Typescript warnings. I've tried to fix it as I can, but since I've never used Typescript and just learn as I go, please take a look and tell me what you think.

dylans · 2021-08-28T12:02:26Z

@dylans Thank you, and actually that's a good call because I've written an e2e test for this triple-click behavior.

It passed, but it opened up a torrent of Typescript warnings. I've tried to fix it as I can, but since I've never used Typescript and just learn as I go, please take a look and tell me what you think.

Your TS seems reasonable. Maybe there are ways to make it less messy, but I think it's fine. Landing this now.

dylans · 2021-09-06T19:21:14Z

@bytrangle This appears to have caused a regression in Chrome... when you triple check on a link with marks (e.g. bold or italic) and press the backspace/delete key, it only deletes back to the point of the most recent mark.

The example in examples/richtext , triple click on the first line and press backspace to reproduce.

TheSpyder · 2021-09-08T00:15:26Z

Looking at the stack trace generated by #4490 the use of ! on values that might be null is to blame. I haven't been paying enough attention to PRs, sorry, I would've flagged that as a concern.

bytrangle · 2021-09-08T09:24:59Z

@dylans You mean this?

2021-09-08--16-22-05__triple-click-on-marks.mp4

…or#4455)

* Remove changeset for the fix for regression caused by triple click fix * fix: incorrect Slate range when triple clicking a block (#4455)

* Remove changeset for the fix for regression caused by triple click fix * fix: incorrect Slate range when triple clicking a block (ianstormtaylor#4455)

…anstormtaylor#4455) * fix(editable component): reselect the range created by triple click * Revert "fix(editable component): reselect the range created by triple click" Reason: attaching a handler for `onClick` event is no longer needed. * fix(react-editor): reselect DOMSelection when triple clicked Reason: Triple clicking an element in Chrome will falsely set the focus node as the next sibling node with focusOffset 0 * test: add e2e test for triple click

* Remove changeset for the fix for regression caused by triple click fix * fix: incorrect Slate range when triple clicking a block (ianstormtaylor#4455)

… unhanging range with void

…4588) * revert #4455 / #4512; fix triple-click by unhanging range with void * added changeset

fix(editable component): reselect the range created by triple click

c892e1d

dylans requested changes Aug 16, 2021

View reviewed changes

xenostar reviewed Aug 24, 2021

View reviewed changes

bytrangle added 2 commits August 25, 2021 11:13

Revert "fix(editable component): reselect the range created by triple…

ff0744e

… click" Reason: attaching a handler for `onClick` event is no longer needed.

fix(react-editor): reselect DOMSelection when triple clicked

32af81c

Reason: Triple clicking an element in Chrome will falsely set the focus node as the next sibling node with focusOffset 0

dylans approved these changes Aug 26, 2021

View reviewed changes

test: add e2e test for triple click

23405b8

dylans merged commit 2d1aaaf into ianstormtaylor:main Aug 28, 2021

TheSpyder mentioned this pull request Sep 8, 2021

Crash during mouse selection on Chrome over multiple nodes #4490

Closed

jaked mentioned this pull request Sep 8, 2021

on Chrome, shift-arrow down then delete doesn't delete text #4496

Closed

bytrangle mentioned this pull request Sep 8, 2021

Fix: regression caused by triple click fix #4498

Merged

4 tasks

bytrangle added a commit to bytrangle/slate that referenced this pull request Sep 9, 2021

fix: incorrect Slate range when triple clicking a block (ianstormtayl…

7b9f05e

…or#4455)

bytrangle mentioned this pull request Sep 9, 2021

Add changeset for triple click fix #4503

Merged

5 tasks

dylans pushed a commit that referenced this pull request Sep 9, 2021

Add changeset for triple click fix (#4503)

2065c5b

* Remove changeset for the fix for regression caused by triple click fix * fix: incorrect Slate range when triple clicking a block (#4455)

dylans mentioned this pull request Sep 16, 2021

Not possible to triple click when checklist is the next block #4528

Open

This was referenced Sep 28, 2021

Deleting a selection only deletes the last element #4492

Closed

Triple-click selection and Selection API issue #4329

Closed

jaked added a commit to jaked/slate that referenced this pull request Oct 10, 2021

revert ianstormtaylor#4455 / ianstormtaylor#4512; fix triple-click by…

1e8a6af

… unhanging range with void

jaked mentioned this pull request Oct 10, 2021

revert #4455 / #4512; fix triple-click by unhanging range with void #4588

Merged

5 tasks

dylans pushed a commit that referenced this pull request Oct 11, 2021

revert #4455 / #4512; fix triple-click by unhanging range with void (#…

ae65ae5

…4588) * revert #4455 / #4512; fix triple-click by unhanging range with void * added changeset

github-actions bot mentioned this pull request Oct 11, 2021

Version Packages #4592

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix(editable component): reselect the range created by triple click #4455

Fix(editable component): reselect the range created by triple click #4455

bytrangle commented Aug 16, 2021

changeset-bot bot commented Aug 16, 2021 •

edited

Loading

dylans left a comment

dylans Aug 16, 2021

dylans Aug 16, 2021

dylans Aug 16, 2021 •

edited

Loading

bytrangle commented Aug 16, 2021 •

edited

Loading

bytrangle commented Aug 17, 2021 •

edited

Loading

dylans commented Aug 17, 2021

xenostar commented Aug 24, 2021

xenostar Aug 24, 2021

dylans commented Aug 25, 2021

bytrangle commented Aug 26, 2021 •

edited

Loading

bytrangle commented Aug 26, 2021

dylans left a comment

bytrangle commented Aug 27, 2021

dylans commented Aug 28, 2021

dylans commented Sep 6, 2021

TheSpyder commented Sep 8, 2021

bytrangle commented Sep 8, 2021

	// Get all the children of the element above
	// Get all the children of the block element above the selection

Fix(editable component): reselect the range created by triple click #4455

Fix(editable component): reselect the range created by triple click #4455

Conversation

bytrangle commented Aug 16, 2021

changeset-bot bot commented Aug 16, 2021 • edited Loading

⚠️ No Changeset found

dylans left a comment

Choose a reason for hiding this comment

dylans Aug 16, 2021

Choose a reason for hiding this comment

dylans Aug 16, 2021

Choose a reason for hiding this comment

dylans Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

bytrangle commented Aug 16, 2021 • edited Loading

bytrangle commented Aug 17, 2021 • edited Loading

dylans commented Aug 17, 2021

xenostar commented Aug 24, 2021

xenostar Aug 24, 2021

Choose a reason for hiding this comment

dylans commented Aug 25, 2021

bytrangle commented Aug 26, 2021 • edited Loading

bytrangle commented Aug 26, 2021

dylans left a comment

Choose a reason for hiding this comment

bytrangle commented Aug 27, 2021

dylans commented Aug 28, 2021

dylans commented Sep 6, 2021

TheSpyder commented Sep 8, 2021

bytrangle commented Sep 8, 2021

changeset-bot bot commented Aug 16, 2021 •

edited

Loading

dylans Aug 16, 2021 •

edited

Loading

bytrangle commented Aug 16, 2021 •

edited

Loading

bytrangle commented Aug 17, 2021 •

edited

Loading

bytrangle commented Aug 26, 2021 •

edited

Loading