Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Triple-click selection and Selection API issue #4329

Closed
julienmonnard opened this issue Jun 9, 2021 · 6 comments · Fixed by #4455 or #4588
Closed

Triple-click selection and Selection API issue #4329

julienmonnard opened this issue Jun 9, 2021 · 6 comments · Fixed by #4455 or #4588
Labels

Comments

@julienmonnard
Copy link

Description
Hello! We are currently facing a bug with triple-click selection. When we triple-click on a word, the expectation is that the whole text block gets selected, a paragraph for example. The problem here is that for 2 of the 3 browsers I tested (it happens on Chrome and Edge, not on Firefox), Slate will also select the following (or part of the following) sibling node.

You can see recordings for both behaviours, the first for Chrome (bug) and the second on Firefox (no bug).

Recording

Chrome.mov
Firefox.mov

Steps
To reproduce the behavior:

  1. Go to https://www.slatejs.org/examples/images
  2. Triple-click on the first paragraph
  3. On Chrome the image gets selected as well, not on Firefox

Expectation
The expectation would be to have the selection only on the text block that was actually targeted, and not the following one.

Environment

  • Slate Version: latest
  • Operating System: macOS 10.15.7
  • Browser: Chrome, Edge

Context
From my understanding, there is two things that feel are part of the issue.

  1. The first might be because of the way offsets are considered within a selection.
    At this line:

    let isLast = offset === node.childNodes.length

    offset is compared to childNodes.length
    But according to the Selection API the offset represents the number of characters selected, not the number of child nodes, so this is not comparing the same things at all.

  2. The second thing is, if the value of focusOffset is 0, it means there is nothing selected in the focusNode and therefore, this node should be removed from the selection range. Currently, maybe because of the first point, Slate keeps digging into this focusNode to find a selectable child and include it in the range. On top of it, if focusNode is a void element, it can create more problems, as Slate is not supposed to handle the content of a void element.

@bytrangle
Copy link
Contributor

@julienmonnard Thanks for the great demo and explanation. Are you working on this issue?

@julienmonnard
Copy link
Author

@bytrangle Unfortunately I currently don't have enough free time to work on it... If you or anybody want to take it you're very welcome to do so.

@bytrangle
Copy link
Contributor

bytrangle commented Aug 6, 2021

More findings in case anyone wants to take on this issue:
This triple-click issue also manifests in the Rich Text example.

  • Hover anywhere over the second block "Since it's rich text..."
  • Triple click to see that the block has been selected visually.
  • You'll see that the Quote Block toolbar button is highlighted, which indicates that the quote block below the currently selected paragraph block has also been selected.

triple-click-selection

The Quote Block button shouldn't be highlighted in this case.

The method responsible for checking if a block is selected is isBlockActive at site/examples/richtext.tsx.

const isBlockActive = (editor, format) => {
  const [match] = Editor.nodes(editor, {
    match: n => {
      if (!Editor.isEditor(n) && SlateElement.isElement(n)) {
        console.log({n})
      }
      const isMatching = !Editor.isEditor(n) && SlateElement.isElement(n) && n.type === format
      return isMatching
    }
  })
  return !!match
}

Inside, it calls Editor.nodes() which takes two parameter:

  • editor object
  • an optional parameter which must be an object that takes a number of properties like match, at, mode etc.

Editor.nodes() returns a generator of nodes at the location provided by at. If no at is given, the default is the current selection.

In case of the Rich Text example, Editor.nodes()'s match property is given a value so that it only returns nodes that are not the top-level editor object, they have to be elements and their types have to match the type of the Toolbar button being clicked.

Let's add a console.log to output the element node returned.

After building Slate locally, visit the rich text example.

This is what happens when single clicking the second paragraph block:

single-click

The element node we get is correct.

When click and drag to select the second block:

click-drag-selection

Please don't mind numerous element nodes output to the console. It's because I click and drag, the selection will change continuously and the isBlockActive is also fired repeatedly. What matters is the final element node. It's also the paragraph block we've selected.

And this is what happens when I triple click:

element-nodes-triple-click

We get exactly four element nodes. I've tried it many times, and it's always four. I think it happens like this:

  • The first element node corresponds to the first click
  • The second element node corresponds to the second click
  • The third element node (paragraph type) and fourth element (quote-block type) correspond to the third click

Because on the third click, Editor.nodes() returns the second paragraph block and the quote block below, the isBlockActive method returns true, even though it shouldn't.

Do you have any idea what cause this @julienmonnard? I've read your hypothesis above, but it doesn't seem to explain the case of single click. In that case, the anchorOffset and focusOffset are both 0, which means nothing is selected, but Slate is able to get the correct element node.

@Kidlike
Copy link

Kidlike commented Aug 6, 2021

(I worked with @julienmonnard on the initial report, so it's possible that I'm somewhat biased :) )

EDIT: Funny thing, both me and Julien were replying to this. He has way more in-depth experience with Slate, and everyone should read his post first ✌️


@bytrangle I would suggest to try and fully understand the selection API.

Just to clarify something on your post:

In that case, the anchorOffset and focusOffset are both 0, which means nothing is selected

This is not entirely true. You're missing 1 more condition: anchorNode == focusNode. So in total, when nothing is selected, this expression would be true: anchorNode == focusNode && anchorOffset == 0 && focusOffset == 0.
Also, the API is kind enough to already tell you this, by setting isCollapsed to true.

So, for any such collapsed selections, things are pretty easy, because all browsers behave the same, and basically the above condition is always true.


I think there are 2 composite "problems":

  1. Firefox and Chrome use the Selection API differently (at least for triple-click), but semantically they are both correct.

    • Chrome will set the focusNode on the next element (the first element outside the actual selection) with focusOffset = 0. With focusOffset = 0, it means to not include anything from the focusNode.

    • Firefox will set the focusNode on the last visibly selected element on the screen, and set focusOffset to the actual number of characters selected.

    • For example: let's say that you triple-click the middle (largest) text here: https://www.slatejs.org/examples/richtext

      Chrome will tell you that the selection ends 0 characters inside the "A wise quote" element (which can be translated to "everything before this position").

      Whereas Firefox will tell you that the selection ends 82 characters inside the ", or add a semantically rendered block quote in the middle of the page, like this:" element (basically all of it).

  2. Slate is not implementing the full API, but only the non-textual element case (the case where focusOffset is counting nodes and not characters). From the MDN docs:

    If focusNode is a text node, this is the number of characters within focusNode preceding the focus. If focusNode is an element, this is the number of child nodes of the focusNode preceding the focus.

    Slate implements only the second half, while also assuming it's the only case that exists.

    Of course it's possible that this situation is not Slate's fault. Since the API is still marked as Experimental, it might have changed in the meantime since Slate implemented this feature.


I hope this helps... it's a pretty complicated situation for anyone with fresh eyes.

@julienmonnard
Copy link
Author

So first of all, to not mix everything below, there is different things at play here:

  • 2 different types of nodes: DOM Node and Slate Node, which are not the same.
  • 2 different types of selection object: Selection from Selection API and the Slate Selection, different things as well.

I will try to keep these names.

If you do a single click on you paragraph, for example between the letters o and l of the word bold, the Selection object (from Selection API) will contain this (not only, but I keep the useful part here)

{
    anchorNode: text, // "text" is a `DOM Node` representing the word "bold"
    anchorOffset: 2,
    focusNode: text, // "text" is a `DOM Node` representing the word "bold"
    focusOffset: 2,
}

Here, the two text are DOM Nodes representing the word bold and the 2 is after how many characters of that word you clicked. Based on that, Slate will return the first valid Slate Node in which this text is. In our case, the full paragraph. It will then assign it to the Slate Selection.

Now if you do the "click-and-drag", your Selection object (from API) will be:

// If you dragged from beginning to end
{
    anchorNode: text, // "text" is a `DOM Node` representing the text until just before "bold"
    anchorOffset: 0,
    focusNode: text, // "text" is a `DOM Node` representing the text from the comma until the end
    focusOffset: 82,
}
// If you dragged from end to beginning
{
    anchorNode: text, // "text" is a `DOM Node` representing the text from the comma until the end
    anchorOffset: 82,
    focusNode: text, // "text" is a `DOM Node` representing the text until just before "bold"
    focusOffset: 0,
}

For Slate, it's the same as before, it will get the first valid Slate Node that contains your selection, in this case still the same paragraph, give it to the Slate Selection.

And for the triple click:

// in Chrome
{
    anchorNode: text, // "text" is a `DOM Node` representing the text until just before "bold"
    anchorOffset: 0,
    focusNode: text, // Now "text" is a `DOM Node` representing the text of the quote
    focusOffset: 0,
}

// in Firefox
{
    anchorNode: text, // "text" is a `DOM Node` representing the text until just before "bold"
    anchorOffset: 0,
    focusNode: text, // "text" is a `DOM Node` representing the text from the comma until the end
    focusOffset: 82,
}

Since here the Selection API returns the text of the quote as a focusNode (DOM Node), Slate detects that this is not part of the paragraph you "triple-selected", and will therefore add the Quote element to your Slate Selection. And this last part will trigger all the issues we noticed, like the trigger of isBlockActive and the image issue.

The problem here is that since the focusOffset is at 0, it means that no characters from the focusNode is actually selected, so Slate should not be giving that DOM Node to the Slate Selection. To make the connection with my initial issue, this is not a problem with Firefox because unlike Chrome, it already removes this "unselected node" before returning the selection, so Slate doesn't even know about it

@jaked
Copy link
Contributor

jaked commented Sep 30, 2021

I've been investigating / trying to fix this (see comments on #4492) and wanted to record some findings:

The basic issue as I understand it is that the DOM selection API supports two kinds of selection endpoint:

  • text node + character offset (I'll call it a "text endpoint")
  • element node + child offset (I'll call it an "element endpoint"), used to signify that entire elements are in the selection, not just the text inside them

but Slate selections support only text endpoints.

When a DOM selection has an element endpoint, Slate looks for a corresponding text endpoint (ReactEditor.toSlatePoint). In most cases, this text endpoint is after the original element endpoint; so when the focus of a forward selection is an element endpoint, the Slate selection is "hanging": its focus is at the beginning of the following block.

Browsers vary on exactly what actions produce element endpoints, but they are generally actions that sensibly mean "select the whole element":

  • triple-click on a paragraph
  • place the cursor at the beginning of a paragraph, shift-arrow-down past the end of the paragraph
  • place the cursor at the beginning of a paragraph, shift-arrow-right to the end of the paragraph (selects the text within the paragraph), then shift-arrow-right again (selects the whole paragraph)
  • drag the cursor from the beginning of a paragraph to the end (selects the text within the paragraph), then drag a little further / down (selects the whole paragraph)

All of these generate element endpoints (in most cases) in the browsers I've tested (Chrome, Firefox, Safari, all on a Mac), and in most cases the element endpoints turn into a hanging selection in Slate.

It's good to have hanging selections! There's no other way in Slate to represent the selection of a whole block, and there are lots of actions that sensibly work on a whole block: e.g. if you triple-click a paragraph or shift-arrow-down a paragraph and hit delete, it's sensible for the whole paragraph to be deleted, not just the text within it. (I haven't tested a lot of rich text editors on this but it seems pretty standard.)

But hanging selections have some bugs:

First, browsers differ in exactly how they generate selection endpoints for the actions above:

  • Chrome always generates an element endpoint for the selection focus, which points to an element preceding the next editable text location, so Slate generates a hanging selection.
  • Safari behaves like Chrome, except when there is an image before the next editable location; then it generates a text endpoint in the original paragraph (so Slate generates a non-hanging selection).
  • Firefox on triple-click generates an element endpoint for the selection focus, which points to the selected paragraph with offset past the last text child. But Slate turns this into a non-hanging selection (because DOM.normalizeDOMPoint searches backwards for a text node when the offset is past the last child).
  • Firefox on the other actions generates a text endpoint at the next editable text location (i.e. the DOM selection is already hanging, and Slate generates a hanging selection), except when there is an image before the next editable location; then it generates an element endpoint preceding the next editable text location, so Slate generates a hanging selection.

(I feel like these differences are not really a problem—applications should do something sensible with both hanging and non-hanging selections; so inconsistencies in when hanging / non-hanging selections are generated don't really matter. Just my opinion!)

Second, applications don't always handle hanging selections correctly:

  • In the rich text example, as @bytrangle points out above, the quotations toolbar button is incorrectly highlighted for hanging selections, because the selection is not unhanged with Range.unhang before checking the nodes. I think this is a bug in the example—it should properly handle both hanging and non-hanging selections. (Note that you can't currently reproduce this in Chrome due to Fix(editable component): reselect the range created by triple click #4455 / fix(react-editor): reset focus offset when triple clicking #4512; it's reproducible in Safari with a triple-click or shift-arrow-down, or in Firefox with shift-arrow-down.)
  • In the images example, as @julienmonnard points out above, the image is highlighted for hanging selections (and is deleted if you press delete). I don't totally understand what's happening here, but it seems like a hanging selection that ends inside a void node is not handled correctly in Slate. This is separate from the varying browser behavior with following images (which affects whether the selection is hanging or not).

@bytrangle made an attempt at fixing these bugs with #4455 / #4512. I've been looking into alternative fixes because @bytrangle's fixes have caused other bugs (#4492), because they address Chrome only (the issue is more widespread), and because they fix the bugs by getting rid of hanging selections (which I think is a mistake; hanging ranges are useful).

I've been working on a branch that addresses these bugs by always looking for the text endpoint inside the DOM selection, never outside. This works without browser-specific fixups, but it also gets rid of hanging selections (as above I think this is a mistake). I'd rather address the bugs by fixing the examples where needed, and by understanding / fixing the behavior of hanging selections inside void nodes. What do you think? @ianstormtaylor would love your input here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants