fix LexicalNode.isBefore(), clean up and optimize getCommonAncestor, isBefore, getNodesBetween

[Piechota]

Currently LexicalNode.isBefore() when tested with parent.isBefore(child) will return false.

It seems more logically correct (and similar to editor's node tree) for parent to be before child.

Right now everything that tests for .isBefore() uses only TextNode.

1. in LexicalSelection.ts before isBefore() is called there is (in PointType.isBefore()):

if ($isElementNode(bNode)) {
      const bNodeDescendant = bNode.getDescendantByIndex<ElementNode>(bOffset);
      bNode = bNodeDescendant != null ? bNodeDescendant : bNode;
    }

that looks for first child that is not ElementNode

2. LexicalNode.getNodesBetween() in LexicalNode.getNodes() is also behind getDescendantByIndex()

3. in CodeHighlighter.ts functon $handleShiftLines() that uses isBefore() starts with

 if (
    !$isSelectionInCode(selection) ||
    !($isCodeHighlightNode(anchorNode) || $isTabNode(anchorNode)) ||
    !($isCodeHighlightNode(focusNode) || $isTabNode(focusNode))
  ) {
    return false;
  }

That will early out on parent and second getNodesBetween() is also behind this early out.

From what I saw it works correctly if we assume that parent is at the end of children.
But I feel like it's less intuitive and not consistent with getIndexWithinParent() getDescendantByIndex() that go from 0 to children count.

Original isBefore() is quite old #590
and doesn't explain what it fixes.

[vercel]

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	Dec 2, 2024 7:19pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 2, 2024 7:19pm

[github-actions]

size-limit report 📦

Path	Size
lexical - cjs	30.99 KB (0%)
lexical - esm	30.82 KB (0%)
@lexical/rich-text - cjs	39.76 KB (0%)
@lexical/rich-text - esm	32.63 KB (0%)
@lexical/plain-text - cjs	38.35 KB (0%)
@lexical/plain-text - esm	29.95 KB (0%)
@lexical/react - cjs	41.48 KB (0%)
@lexical/react - esm	34.02 KB (0%)

[Piechota]

Ok, there is one bug with current implementation.
if we have a tree:

ElementNode0
  TextNode1
  TextNode2
  TextNode3
  TextNode4
ElementNode5
  TextNode6
  TextNode7
  TextNode8

Test	Call	Current	New
0	ElementNode0.GetNodesBetween(TextNode7)=	0, 1, 2, 3... 7	0, 1, 2, 3... 7
1	ElementNode0.GetNodesBetween(TextNode4)=	4, 0	0, 1, 2, 3, 4
2	TextNode7.GetNodesBetween(ElementNode0)=	0, 5, 6, 7	0, 5, 6, 7
3	TextNode4.GetNodesBetween(ElementNode0)=	4, 0	0, 1, 2, 3, 4

So it isn't inconsistent with when it goes down the tree and when parent is at the end.
Additionally Test 0 and Test 2 should give the same result, but this is bug on GetNodesBetween side. it shouldn't early out when parent is last, should go through all children first. (it also happens with my changes)

With changed version it's more consistent, but it doesn't fix GetNodesBetween, it will have to wait for another PR.

[Piechota]

Additionally cleaned few other methods, and fixed bug in getNodesBetween:
if we have a tree:

ElementNode0
  TextNode1
  TextNode2
ElementNode3
  TextNode4

for TextNode2.getNodesBetween( TextNode4 ) it will return TextNode2, ElementNode0, ElementNode3, TextNode4.
Additionally it would give different results if caller and target are switched
with this change it's fixed

[Piechota]

So it turned out getNodesBetween has to return every parent that is touched because selection expects that.
I didn't investigate why, maybe in different PR.

This one still fixes inconsistency when switching caller with target