[Breaking Change][lexical][lexical-playground] Bug Fix: Improve character deletion around shadow roots and decorators #7155
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
…st table in the editor
etrepum
changed the title
etrepum
changed the title
etrepum
changed the title
etrepum
requested review from
zurfyx,
fantactuka,
acywatson,
Fetz,
ivailop7,
Sahejkm and
potatowagon
as code owners
| return false; | ||
| } | ||
| if (!isBackward) { | ||
| // inherit strange forward delete behavior per e2e election test |
There was a problem hiding this comment.
If you want to change the behaviour that forward deletion should not delete the collapsible section, I'm with you.
There was a problem hiding this comment.
I don't have strong opinions about it, it's in a playground plug-in after all. I figured that breaking zero tests would maybe speed up review but it's been a week 🤷
There was a problem hiding this comment.
Let's make it better. I'll do my part on the reviewing side this evening.
There was a problem hiding this comment.
I created a follow-up #7203 to address this separately
| if (target) { | ||
| if ($isLineBreakNode(possibleLineBreak)) { | ||
| possibleLineBreak.remove(); | ||
| } | ||
| target.select().insertRawText(textContent); |
There was a problem hiding this comment.
How come this toolbar logic has now become so manual? Is this the final shape? It used to be pretty straightforward to replicate once it was backed by insertNodes
There was a problem hiding this comment.
It's because insertNodes is buggy and I didn't want to also fix that in the scope of this PR
Breaking Change
The heuristic used in
RangeSelection.deleteCharacterto handle merging blocks and deleting decorators from a collapsed selection has been refactored for consistency. It now traverses the node tree directly instead of attempting to use browser selection APIs.This generally has much more predictable results, aligned with how this method is expected to behave, but it is not "bug for bug" compatible with how it worked previously. Notably, it will no longer pierce shadow roots when merging blocks. For example, performing a forward deletion from the end of a block before a
CollapsibleContainerNodewould pierce the shadow root of itsCollapsibleTitleNodeand merge its first block child (e.g. the preceding paragraph and the title's paragraph would be merged). Since there was a test that expected this strange behavior, this functionality was moved to theCollapsiblePlugin'sDELETE_CHARACTER_COMMANDcommand listener.Description
Allowing the native platform to control deletion only really makes sense when the content to be deleted includes a TextNode, otherwise we are better off handling it ourselves. There's also some "interesting" edge cases around nested elements (with or without shadow roots) and decorators that need to be addressed by this sort of manual navigation.
Other fixes:
$setBlocksTypeworkaround for code nodes (playground only) Bug: Unexpected behavior with transform selected lines to code block #5686$setBlocksTypesupport for selection types other than RangeSelection Bug: Block Type Change to Normal Fails for Selected Table Cells in Playground #7082selection.removeText()toRangeSelection.insertNodes(…)Closes #7082
Closes #7090
Closes #5686
Closes #7173
Closes #6763
Closes #7184
Test plan
Before
After