fix(lexical): calculate range selection formatting

[LuciNyan]

Description

Fix range selection formatting.

It looks like this bug is related to the format of our rangeSelection calculation. Here our calculation is not quite the same as the google doc. As an example:

• In google doc. When we have three letters 1(italic) 2(bold) 3(underline), and we select them, the selection will show that the format is nothing at this time. Only when all textNodes have a format at the same time will the selection show that it has a format.

• And in lexical, our logic is that as long as the anchorNode or focusNode has a certain style, we will show that selection.format has that style.

• When we can calculate the selection range correctly, the problem mentioned in the issue does not exist anymore.

---

closes #2626

2022-07-14.14.55.20.mov

[vercel]

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

Name	Status	Preview	Updated
lexical	✅ Ready (Inspect)	Visit Preview	Jul 18, 2022 at 9:38AM (UTC)
lexical-playground	✅ Ready (Inspect)	Visit Preview	Jul 18, 2022 at 9:38AM (UTC)

[zurfyx]

Thank you for digging into this Luci! While I don't disagree with the new proposed heuristic, the root of the problem is hiding in the beforeInput behavior. Notice how it still behaves oddly when doing the reverse thing <bold>Hello</bold> world

Also, is this GDocs heuristic? Haven't played with it extensively but my hunch is that they're using the first node instead (that is focus when selection is reversed, anchor otherwise)

[LuciNyan]

Thank you for digging into this Luci! While I don't disagree with the new proposed heuristic, the root of the problem is hiding in the beforeInput behavior. Notice how it still behaves oddly when doing the reverse thing <bold>Hello</bold> world

Also, is this GDocs heuristic? Haven't played with it extensively but my hunch is that they're using the first node instead (that is focus when selection is reversed, anchor otherwise)

Based on your concern, I have tried the same on onenote and microsoft word, both of which have this logic. It doesn't seem to be unique to GDocs.

When we select some textNodes, the formatting displayed is actually the intersection of the formatting owned by all these textNodes (while we are using a concatenation of anchorNode and focusNode formatting).

So when I go to select 1 2 3 in the first row, the toolbar doesn't show that they have any formatting.

You can see that when the second row of 2 3 has the same formatting and the selection contains only 2 3, the selection shows their common formatting.

2022-07-14.17.14.31.mov

2022-07-14.17.15.40.mov

[LuciNyan]

Notice how it still behaves oddly when doing the reverse thing <bold>Hello</bold> world

Thank you for pointing this place out. I've tried it on google doc and onenote as well as microsoft. Their common logic is that the new text will inherit the style of the top textNode in the deleted text. When we change the calculation of the selection formatting, lexical behaves consistently with these editors.

2022-07-14.17.25.23.mov

2022-07-14.17.29.57.mov

2022-07-14.17.31.11.mov

[acywatson]

This looks like the right solution, behaviorally, but I'm a little bit concerned about putting selection.getNodes and iteration through the result in this path. This could get a bit hot if someone is drag selecting a bunch of nodes.

I know we do some caching in getNodes and I actually haven't been able to detect a practical slowdown in my environment, but I want to test this a bit more. Will circle back shortly.

@zurfyx @fantactuka do you seen any concerns here?

[LuciNyan]

This looks like the right solution, behaviorally, but I'm a little bit concerned about putting selection.getNodes and iteration through the result in this path. This could get a bit hot if someone is drag selecting a bunch of nodes.

I know we do some caching in getNodes and I actually haven't been able to detect a practical slowdown in my environment, but I want to test this a bit more. Will circle back shortly.

Thank you for the reminder! I think we can break the iteration early when the combinedFormat is equal to 0. This will avoid a lot of unnecessary calculations.

[zurfyx]

@acywatson, @LuciNyan - Had a look over the cost. A potential optimization could be to store a cache of the result of an arbritary array of selection.getNodes so that we can reuse the value later if it hasn't changed.

However, this is not immediate because selection._cachedNodes changes at every selection changes. We would have to rebuild the logic to have a more stable array that avoids changing reference at every selection.

That said, while I think the above optimizations can be interesting performance-wise, I don't foresee a noticeable regression as part of the introduction of this logic for the majority of the users.

1. We are currently already running selection.getNodes on every selection change (on the Playground). The toolbar, floating toolbar and TreeView trigger it multiple times
2. This logic only runs on non-collapsed selection

I think it's worth listing down as an issue to optimize later as it can potentially yield some wins even beyond this PR (and non-collapsed selection changes) but we can likely do without them now.

[fantactuka]

Wonder if it's worth adding giving it's a hot path and can be costly when selecting whole document. IIRC quip does anchor/focus check only

[LuciNyan]

Wonder if it's worth adding giving it's a hot path and can be costly when selecting whole document. IIRC quip does anchor/focus check only

Or can we do the combinedFormat intersection operation at the same time as selection.getNodes?

Let's say we add a new method to selection like getCommonFormat, then it will do the same thing as getNodes, it will iterate over each node, but do the intersection operation at the same time. Once the result of the combinedFormat is 0, we can quit iterating. So even if the user selects the whole document, it looks like we won't be traversing all the nodes. By doing this, I think we'll be able to exit the traversal pretty quickly in most cases.

I'm wondering if you think this would be better?

[zurfyx]

LGTM, thank you Luci, will wait until @acywatson's stamp too since he wanted to have a second pass to it

[zurfyx]

@acywatson, @LuciNyan - Had a look over the cost. A potential optimization could be to store a cache of the result of an arbritary array of selection.getNodes so that we can reuse the value later if it hasn't changed.

However, this is not immediate because selection._cachedNodes changes at every selection changes. We would have to rebuild the logic to have a more stable array that avoids changing reference at every selection.

That said, while I think the above optimizations can be interesting performance-wise, I don't foresee a noticeable regression as part of the introduction of this logic for the majority of the users.

1. We are currently already running selection.getNodes on every selection change (on the Playground). The toolbar, floating toolbar and TreeView trigger it multiple times
2. This logic only runs on non-collapsed selection

I think it's worth listing down as an issue to optimize later as it can potentially yield some wins even beyond this PR (and non-collapsed selection changes) but we can likely do without them now.

[acywatson]

I'm good with this - I don't see any issues at the moment. I do agree that this is an area we could probably optimize in the future.

Nice work and thanks again for the contribution!