[lexical] Bug fix: $dfsCaretIterator should be able to stop at its last descendant#7978
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
packages/lexical-utils/src/index.ts
Outdated
| let endCaret = $getEndCaret(start, direction); | ||
| if (endNode) { | ||
| endCaret = $getAdjacentChildCaret( | ||
| $getChildCaretOrSelf($getSiblingCaret(endNode, direction)), | ||
| ); | ||
| if (endCaret === null) { | ||
| const next = $getAdjacentSiblingOrParentSiblingCaret( | ||
| $getSiblingCaret(endNode, direction), | ||
| ); | ||
| endCaret = next && next[0]; | ||
| } | ||
| } |
There was a problem hiding this comment.
This looks like a copy pasted version of $getEndCaret(endNode, direction)
| let endCaret = $getEndCaret(start, direction); | |
| if (endNode) { | |
| endCaret = $getAdjacentChildCaret( | |
| $getChildCaretOrSelf($getSiblingCaret(endNode, direction)), | |
| ); | |
| if (endCaret === null) { | |
| const next = $getAdjacentSiblingOrParentSiblingCaret( | |
| $getSiblingCaret(endNode, direction), | |
| ); | |
| endCaret = next && next[0]; | |
| } | |
| } | |
| const endCaret = endNode | |
| ? $getAdjacentChildCaret( | |
| $getChildCaretOrSelf($getSiblingCaret(endNode, direction)), | |
| ) || $getEndCaret(endNode, direction) | |
| : $getEndCaret(start, direction); |
|
While there is clearly some sort of bug here I think if we're going to try and fix anything with these functions it should be a more comprehensive fix to the tests and API docs to fully specify what the behavior actually should be. The caret APIs were added because functions like this one (and many of the functions that work with selections) are so ambiguous about how the edges and ElementNodes should be handled. These functions aren't even used at all in lexical itself (outside of a few tests) except for a single From the API docs it's not really even clear whether endNode should be inclusive or not nor does it specify what happens if it's an ElementNode with children. |
|
@etrepum , I made the changes you requested - simplified the code and added to the jsdocs for the relevant functions. To maintain backwards compatibility, both startNode and endNode should be inclusive. However, just to be clear, the problem being solved here is not whether the startNode and endNode are inclusive or not, but that in this situation (where endNode has no next sibling), the caller supplies an endNode, the iterator hits the end node, and then just keeps going and going all the way to the end of the ENTIRE editor tree. The API is pretty clear that if the caller supplies an endNode, the iterator should stop at the endNode - no ambiguity there. |
|
I don't really understand your point about ElementNodes being ambiguous. To me, it seems like the rules of Depth-First-Search are pretty clear when it comes to a node with children. Can you explain your point a bit more? |
|
My point is about an ElementNode being used as the endNode. ElementNode are basically visited twice, once on enter and once on exit. You could stop at or after either depending on the expected semantics of the function, because without carets to distinguish between them it’s very ambiguous. Depending on the definition there may or may not be a way to specify “include this ElementNode but not any of its children”. |
|
To be clear there’s definitely a problem with the implementation of endNode before this PR, but it’s also not really clear what this function was really supposed to do with endNode. There were not any docs, tests, or usage to specify what the edge case handling should do. |
packages/lexical-utils/src/index.ts
Outdated
| * @param startNode - The node to start the search, if omitted, it will start at the root node. | ||
| * @param endNode - The node to end the search, if omitted, it will find all descendants of the startingNode. | ||
| * @param startNode - The node to start the search (inclusive), if omitted, it will start at the root node. | ||
| * @param endNode - The node to end the search (inclusive), if omitted, it will find all descendants of the startingNode. |
There was a problem hiding this comment.
This does not specify (in docs or tests) whether endNode is also inclusive of all descendants of endNode
|
Got it! Thanks for the explanation. I can make those changes! 👍 |
etrepum
left a comment
There was a problem hiding this comment.
This looks great, now the function clearly specifies how it's supposed to behave so it's finally clear what is a bug and what isn't
Description
When you have an element node and you want to iterate through all it's children, you might do something like:
You would expect it to end at the last descendant inside of the elementNode. However, currently it will iterate through all the descendants, and then just continue right on through to the end of the entire editor's tree.
Describe the changes in this pull request
I checked the condition where endCaret is null even though the user did pass in an endNode. In this case, we should get the adjacent parent as the end caret.
Closes #
Test plan
Unit test included.
Before
It would return all nodes to the end of the editor's tree
After
It will properly stop at the node that is passed in as the endNode