[lexical] Bug Fix: Point.isBefore could return incorrect result due to normalization #7256
Conversation
etrepum
requested review from
zurfyx,
fantactuka,
acywatson,
Fetz,
ivailop7,
Sahejkm and
potatowagon
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
| @@ -98,10 +98,18 @@ export function $setBlocksType<T extends ElementNode>( | |||
| prevNode.replace(element, true); | |||
| if (newSelection) { | |||
| if (key === newSelection.anchor.key) { | |||
| newSelection.anchor.key = element.getKey(); | |||
| newSelection.anchor.set( | |||
There was a problem hiding this comment.
This function was the only remaining place I could find, outside of the tests, where we were manually setting properties on the Point. This is probably something we should disallow in the future (e.g. make it readonly in the PointType interface) so that caches and such can be updated correctly. I can see us eventually wanting to store some more information on the point such as a cached NodeCaret or a direction or something like that which we may clear when it's changed.
| if (this.key === b.key) { | ||
| return this.offset < b.offset; |
There was a problem hiding this comment.
This is left in only as an optimization, could be removed for code size
| const aCaret = $normalizeCaret($caretFromPoint(this, 'next')); | ||
| const bCaret = $normalizeCaret($caretFromPoint(b, 'next')); | ||
| return $comparePointCaretNext(aCaret, bCaret) < 0; |
There was a problem hiding this comment.
Normalization is done here for backwards compatibility. PointCaret's ordering before normalization is more precise - two points can refer to the same "visual" location but be "logically" distinct. An 'element' point with the offset of a TextNode is before the a 'text' point at offset 0 to the same TextNode (because you would descend into the TextNode after encountering it as a child of the element), but these are indistinguishable visually and to the descendant-based algorithm initially used by Point.
There are cases where the Point class can still distinguish between "logical" points that are "visually" the same. For example given a textNode that has a text node sibling, the 'text' point at textNode with offset textNode.getTextContentSize() and the 'text' point at textNode.getNextSibling() with offset 0 are visually indistinguishable, but the first point isBefore the second point. This is true both before and after this fix, PointCaret normalization does not bias TextPointCaret.
| const anchorCaret = $caretFromPoint(anchor, 'next'); | ||
| const focusCaret = $caretFromPoint(focus, 'next'); | ||
| const direction = | ||
| $comparePointCaretNext(anchorCaret, focusCaret) <= 0 ? 'next' : 'previous'; |
There was a problem hiding this comment.
Refactored this to use the better implementation of ordering now that we have one, this code was written before #7135 and not refactored in that PR
Description
Fixes a long-standing bug in Point.isBefore by using the NodeCaret implementation of
$comparePointCaretNextCloses #3181
Test plan
Before
If two points that are not normalized are compared, and isBefore's incorrect normalization causes the compared nodes to match (without fixing up the offsets) then the result can be wrong because the offsets refer to a different node entirely.
After
The normalization is now correctly performed with carets, and there's a unit test to confirm that the known broken situation is now fixed.