:bug: Fixed range selection splicing text

[9larsons]

closes Facebook/Lexical#4658 -root key range selections should not splice text, though this is appropriate for text range selections -root key range selections seem to be the best way to handle selecting decorator + text node content, and this bug prevents that from being a good solution

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 30, 2023 2:50pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 30, 2023 2:50pm

[zurfyx]

If a text node is the last node in root, the content is spliced based on the root offset, which doesn't make sense.

Thank you, based on the issue you reported, having a leaf node such as TextNode immediately under RootNode is not a valid state. We do not handle invalid states on Lexical Core (this and many other cases). At some point we might introduce Schemas to identify and throw on these invalid states but we do not want to overcomplicate the logic to handle them all gracefully.

[9larsons]

@zurfyx to be clear, it's root > paragraph > text, not text directly under root. Did you check the branch behavior? I can mock up the action in playground if needed.

[9larsons]

https://github.com/9larsons/lexical/tree/root-selection-bug here's a branch with the behavior that you can see in this video:

https://github.com/facebook/lexical/assets/21961100/2ee3e9ae-e062-4482-96f0-1255d48fdf0b

[zurfyx]

@9larsons - Thanks for the detailed test plan! If I'm looking at this right your selection does not appear to be valid, element selection is only valid when the node is not a text node and only within the immediate parent whereas text selection is only valid on the text descendant. Hence, a root selection is only valid when you're targetting an element that's empty and an immediate descendant of root (includes DecoratorNode) or an empty RootNode

For some reason, CMD + A seems to be broken on Chrome for this specific case (it does not select) but Firefox shows a correct selection which will delete properly

See $normalizeSelection__EXPERIMENTAL

[9larsons]

Yeah, that's interesting - I (clearly) haven't been testing with Firefox. With that, I'm also not sure how I'd implement $normalizeSelection, could you share some more details there?

It seems like the 'issue' with the selection is that a root key is used regardless of it being text or element - is that accurate? If there's a place you have more of this documented I'd very much appreciate having a reference. The concept of simply using the root offsets to create a selection that wrapped all kinds of nodes was from the other PR. This works really well except for that splice line that this PR is targeting.

It's a whole lot more difficult to look at what the first and last nodes of root are, see if their first/last descendant is text, and set the offset appropriately. I will give that a go, though, as that seems to be what's needed if I look at how the selection object is constructed in Firefox.

Edit: as another note, ctrl+a works in chrome if you have a decorator node selected but not if your cursor is in text.

[9larsons]

@zurfyx I think this is what is correct based on what you describe. Could you please take a look when able? https://github.com/facebook/lexical/commit/72601c2d905d25612a5650975a0e20c024beea6d

[zurfyx]

Sorry for the back and forth. I didn't realize that you were performing the selection from the youtube node 🤦‍♂️ (I should've paid more attention to the anchor and focus)

This is the normalization bugfix I was referring to https://github.com/facebook/lexical/pull/4664 - this should also fix the issue pointed out above. It may be worth rechecking whether the insertText logic works as intended when the selection is normalized.

The reason why deletion was failing on select all was more actually more generic than just root, it turns out that our lastNode is correct, a TextNode, but the offset was not transferred to the children. Notice in the video below how offset is the number of the children under root instead of the TextNode offset

https://github.com/facebook/lexical/assets/193447/8a89f9a0-0964-4d80-be39-7212b634e82e

[9larsons]

I think we're still a bit mixed up. I left a comment on that PR as it does fix coming from a NodeSelection but still doesn't work when the Youtubenode is first and you Cmd+A from a TextNode (ie coming from RangeSelection).

https://github.com/facebook/lexical/commit/72601c2d905d25612a5650975a0e20c024beea6d is fully functional for our uses and I think for Lexical's playground - haven't broken it yet, anyways.

With Select All it's much easier to handle whether we've got a text or decorator node so we can appropriately set the selection to text and not element. That seems to be another way to come at the issue, if we can't get the fix applied.

I figured we could use the root element selection for text nodes but that's where we're colliding with Lexical's handling. I don't think this would harm anyone and at any rate, sure has a very confusing output when you remove text from a node that isn't selected (in other words, even if it's not 'correct' selection handling, it's still buggy behavior).

I'm going to see where I can get by implementing additional adjacent node checking so I can know in a situation like:

Decorator rootindex=0
ParagraphNode rootindex=1
* TextNode
Decorator rootindex=2
ParagraphNode rootindex=3 key=3
* TextNode

... Let's say I'm selecting going down the page and want the first three children (Decorator, Paragraph/Text, Decorator). Here's the two possibilities:

in either case the anchor will be the same anchor: {key: 'root', offset: 0, type: 'element'} (zero index of the root editor)

now which focus is right? focus: {key: 'root', offset: 3, type: 'element'} (this is what I proposed as a solution; my understanding is it's wrong, but it would sit at the right place in root) focus: {key: 3, offset: 0, type: 'text'} (start of the text immediately following the decorator)

I'll try to apply the handling to accomplish the latter but it's a lot of overhead for something that could be easily handled with that suggested one-liner and the first solution.

[9larsons]

Not sure why it closed.

[zurfyx]

Let me read this in detail tomorrow but happy to merge what you have for now. Mind adding some test for it to make sure it doesn't regress? Also, feel free to DM me directly on Discord

[9larsons]

Thank you. I was looking around and am not seeing a good spot for the test - could you advise?

The trouble about creating an e2e test is that I can't recreate the range selection using root using keyboard inputs (without adding more code), but I also can't seem to use $setSelection in the e2e tests. To test, I would need to:

• add two paragraph>text nodes with some content
• select the first one with: anchor: {key: 'root', offset: 0, type: 'element'} focus: {key: 'root', offset: 1, type: 'element'}
• delete the selection
• verify the second paragraph's content is not mutated

[zurfyx]

Does https://github.com/facebook/lexical/pull/4671 solve your issue?

[9larsons]

Getting the PRs a bit confused - copying this over (as well as the latest change to this commit) from #4586:

I know we had some conversations outside of this PR. I took another pass at this and had a few notes.

$normalizeSelection mutates the actual selection object, so using $setSelection(normalizedSelection) isn't necessary in the current state (I saw this here) - not sure if that's the intent.

You mentioned needing to do normalization - or that it'd be helpful - because following the browser behavior leads to issues/confusion. It looks like here the browser is using the leading position (e.g. if on the end of the line, it's grabbing the node ahead of it).

Without some serious changes to selection behavior (i.e. not including the subsequent node in the selection.getNodes() call when lastPoint.getNode() is the root node), I think it makes the most sense to just clean up the conditional statement.

I changed it to check if the endPoint was the root node as that's the only place we do splicing, and this accounts for the anchor & focus being reversed. Ultimately it's just more flexible.

The only other way I see to get around this is to lessen the number of nodes returned by selection.getNodes() but this will surely have impacts in a lot of places. I don't think there's a 'right' answer here as the cursor exists in two places at once in this kind of selection.

[9larsons]

@zurfyx hey, could we revisit this or https://github.com/facebook/lexical/pull/4586? I think it would make sense to have some shift+up handling that supports decorators within Lexical, but if needed, we can handle that in our own code base. What's necessary at minimum is the change within this PR. I know it's been a while since we talked about this last so please reach out on Discord if that's easiest.