-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[lexical] [lexical-html] Bug Fix: Sanitize Invalid Nodes On HTML Paste #7982
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Hi @duvallj! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
etrepum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem like a very good approach to solving this problem, it's unexpected and inefficient to call createParentElementNode unless it's known that one needs to be created. Generally these kinds of normalizations are handled by transforms and/or the importDOM implementations.
|
@etrepum I think I see what you mean; you're saying I should hook into existing transform machinery by returning a I can't see a great way for that to work. (1) The only I agree that it's probably not good we're calling |
|
Node Transforms is what I was referring to by transforms, it's typically used to provide various sorts of normalization regardless of how the nodes are created |
|
There are already a few transforms registered for ListNode and ListItemNode, but none of them currently handle this specific scenario. Here's one of them: https://github.com/facebook/lexical/blob/main/packages/lexical-list/src/index.ts#L126-L152 |
|
@etrepum Ah ok, those links are helpful for explaining thanks. I still don't see how a node transform is well-suited to solve the problem at hand, however. There are a couple approaches I see:
(2) is so inefficient I think we can disregard it. (1) is better, since like you say it ensures that Hard to compare exactly the perf differences between "thing that runs once on DOM import" vs "thing that runs every time a list is updated." Though I guess in general, the former might happen more often across the userbase than the latter. |
|
The check for 1 is fairly cheap, there's already code running every time a list item is updated, it just doesn't have a check to see exactly what its parent is. Adding an |
Description
In our use of lexical, we'd often see crashes for lexical error 40, "A ListItemNode must have a ListNode for a parent." We inspected our code for signs where we could be manipulating Lexical nodes manually that would lead to this case, but no luck. Then, I wanted to see if we could break this invariant using just regular Lexical functionality. Turns out we can! Pasting invalid HTML will lead to corrupted internal state, which, if left unchecked, can trigger this error code.
This PR sanitizes nodes on HTML paste so that they aren't invalid. I chose this option because:
insertNodesorinsertAfterseems like a bad idea.Test plan
npm run test-unitBefore
After