[lexical] Bug Fix: Move new paragraph outside inline element in insertParagraph#8158
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
etrepum
left a comment
There was a problem hiding this comment.
Okay so I took a closer look at this problem and the issue is that the document itself has an invalid structure. Inline elements are not allowed to contain blocks in the first place. It doesn't really make sense to add workarounds to improve the behavior when the document's structure is invalid. The only maintainable approach is to prevent these invalid structures from being created in the first place and/or to use transforms to reorganize the document to be valid when the rules are violated.
The two approaches here would be:
- Fix LinkNode's importDOM to not create invalid documents
- Add a LinkNode transform that will fix invalid documents (if a block child is encountered it should split the link and the containing block and then an equivalent LinkNode should be wrapped around the children of the previously contained block, if there are any)
etrepum
left a comment
There was a problem hiding this comment.
This approach doesn't look very comprehensive, I think probably the simplest thing to do here is to use some other logic that already has the logic to split nodes recursively and make sure that the insert happens in the correct parent node. $insertNodeToNearestRootAtCaret would probably do the trick. Maybe something like this:
for (const caret of $getChildCaret(link, 'next')) {
const node = caret.origin;
if (!node.isInline()) {
$insertNodeToNearestRootAtCaret(
node,
$rewindSiblingCaret(caret),
{$shouldSplit: () => false},
);
}
}|
also note that there might be some other logic to handle to make sure that the selection remains valid, if either point of the selection is keyed to an ElementNode on the right side of any of those splits the key and/or offset may no longer be valid. The caret code generally won't touch the selection to fix that stuff up, it's lower-level and avoids those kinds of side-effects (e.g. if using RangeSelection.insertNodes it's just going to replace the current selection in many cases, which is likely not what you want in the cases where this transform is applied). |
etrepum
left a comment
There was a problem hiding this comment.
I think there are more edge cases to the selection here. There two base cases that should be tested:
- Selection is in the paragraph that gets split to the right of the LinkNode
- Selection is in the LinkNode to the right of the non-inline node
These cases also branch depending on if there are siblings to the right of the non-inline node (which may even be another non-inline node) and/or to the right of the LinkNode in the cases where there is no sibling (which determines whether or not a new paragraph is created).
etrepum
left a comment
There was a problem hiding this comment.
These tests aren't actually checking the selection at all after the transforms run, so it could be anywhere at all (even be set to null) and the tests would still pass.
I also don't think this is the correct algorithm for fixing up the selection because there may not be a next child when the LinkNode is split at the end. The correct key and offset could be on a new node or any parent node (including the root node) since when the link is split there might not be a right side left at all.
packages/lexical-link/src/__tests__/unit/LexicalLinkNode.test.ts
Outdated
Show resolved
Hide resolved
packages/lexical-link/src/__tests__/unit/LexicalLinkNode.test.ts
Outdated
Show resolved
Hide resolved
packages/lexical-link/src/__tests__/unit/LexicalLinkNode.test.ts
Outdated
Show resolved
Hide resolved
8da4fdb to
0c9b443
Compare
packages/lexical-link/src/__tests__/unit/LexicalLinkNode.test.ts
Outdated
Show resolved
Hide resolved
packages/lexical-link/src/__tests__/unit/LexicalLinkNode.test.ts
Outdated
Show resolved
Hide resolved
…tore caret position
etrepum
left a comment
There was a problem hiding this comment.
I think this looks good assuming tests still pass. Confirmed that the paste behavior in #8083 is fixed by this transform
Clipboard
<meta charset='utf-8'><a jsname="UWckNb" class="zReHs" href="https://lexical.dev/" data-ved="2ahUKEwi9_Pao6PKSAxVKADQIHV5YH_QQFnoECBoQAQ" ping="/url?sa=t&source=web&rct=j&opi=89978449&url=https://lexical.dev/&ved=2ahUKEwi9_Pao6PKSAxVKADQIHV5YH_QQFnoECBoQAQ" style="color: rgb(153, 195, 255); text-decoration: none; -webkit-tap-highlight-color: rgba(255, 255, 255, 0.1); outline: 0px; font-family: Roboto, Arial, sans-serif; font-size: small; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; white-space: normal; background-color: rgb(31, 31, 31);"><h3 class="LC20lb MBeuO DKV0Md" id="_nvWdab21LsqA0PEP3rD9oA8_46" style="font-weight: 400; margin: 3px 0px 0px; padding: 5px 0px 0px; font-size: 20px; line-height: 26px; font-family: "Google Sans", Roboto, Arial, sans-serif; display: inline-block; text-wrap: wrap; transform: scaleY(-1);">Lexical</h3></a>Result
root
└ (3) heading
└ (6) link "https://lexical.dev/"
> └ (4) text "Lexical"
selection: range
├ anchor { key: 4, offset: 7, type: text }
└ focus { key: 4, offset: 7, type: text }
| if ($isRangeSelection(selection)) { | ||
| $restoreCaretPair(selection.anchor, anchorPair!); | ||
| $restoreCaretPair(selection.focus, focusPair!); | ||
| $normalizeSelection__EXPERIMENTAL(selection); |
There was a problem hiding this comment.
Normalization is already done in $restoreCaretPair
| $normalizeSelection__EXPERIMENTAL(selection); |
There was a problem hiding this comment.
Two tests fail with this change — $normalizeCaret (per-caret) doesn't resolve element points to text points like $normalizeSelection__EXPERIMENTAL does, so the selection ends up on root at offset 1 instead of the correct text node.
Should I keep the $normalizeSelection__EXPERIMENTAL call, or is there a different way to handle this?
There was a problem hiding this comment.
Ok, I can look into that separately after this is merged
Description
Fixed
insertParagraphto move the new paragraph outside of inline elements when a block is nested inside one.Closes #8083
Before
After