Skip to content
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

[lexical] Bug Fix: Handle MutationObserver/input event re-ordering when using contentEditable inside of an iframe #7045

Merged
merged 2 commits into from
Jan 13, 2025

Conversation

etrepum
Copy link
Collaborator

@etrepum etrepum commented Jan 12, 2025

Description

Sometimes when an iframe is used as the contentEditable then the MutationObserver can fire after the input event, even if the mutation actually did happen. This causes problems that were undiagnosed prior to the addition of $validatePoint in __DEV__ (part of #6759) because the Lexical selection would refer to DOM coordinates that were not yet valid in the Lexical document, because the DOM was mutated before getting the selection. When the mutation observer happens first this doesn't occur because either the mutation is applied and/or the selection code is skipped due to getIsProcessingMutations() (see $internalCreateRangeSelection). This is related to the optimization(?) in #794 that avoids explicitly flushing root mutations on input.

This PR:

  • Adds a vanilla-js-iframe example as a testbed for checking that iframe events are handled correctly, with a dev:monorepo script so you can run it with the monorepo like the playground rather than with the npm installed lexical for easier development/debugging (maybe we add this to other examples?)
  • Fixes up some incorrect $function naming
  • Uses updateEditorSync instead of updateEditor in the places that I visited along the way, for in-order execution
  • Adds a currently internal event option to UpdateOptions so that the reason for the update can be captured in a way that's accessible by the code in $internalCreateRangeSelection that checks to see if it's from a mutation or from some specific set of events before deciding whether to use the DOM selection or not. We don't want to use the DOM selection before input because it might refer to DOM coordinates that do not exist in the lexical document yet.
  • Consistently use getWindow for editor._window in the core package (just a general clean-up while investigating)

Closes #7028

Test plan

I'm not exactly sure how to reasonably add tests for this

Before

Error when typing into the end of a TextNode in an iframe on Chrome or Firefox

After

No more errors

@etrepum etrepum added the extended-tests Run extended e2e tests on a PR label Jan 12, 2025
Copy link

vercel bot commented Jan 12, 2025

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 Jan 12, 2025 8:45pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 12, 2025 8:45pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 12, 2025
Copy link

size-limit report 📦

Path Size
lexical - cjs 31.48 KB (0%)
lexical - esm 31.2 KB (0%)
@lexical/rich-text - cjs 40.54 KB (0%)
@lexical/rich-text - esm 33.21 KB (0%)
@lexical/plain-text - cjs 39.06 KB (0%)
@lexical/plain-text - esm 30.37 KB (0%)
@lexical/react - cjs 42.43 KB (0%)
@lexical/react - esm 34.48 KB (0%)

@etrepum etrepum marked this pull request as ready for review January 12, 2025 21:25
Copy link
Member

@zurfyx zurfyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a tricky one, thanks for looking into this! 😮

@@ -290,7 +290,7 @@ function onSelectionChange(
return;
}
}
updateEditor(editor, () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we foresee any performance impact of sync update events? I presume that this makes it possible for multiple reconciliations to happen almost simultaneously

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No performance impact, the behavior only differs for nested updates in which case it’s inlined like dispatchCommand instead of deferred to after the current updateFn returns. It just runs code in the order you expect it to happen, doesn’t otherwise change semantics of reconciliation which still happens on the tick after the updating started (note that no tests had to change here).

@etrepum etrepum added this pull request to the merge queue Jan 13, 2025
Merged via the queue into main with commit 8bd22d5 Jan 13, 2025
45 of 73 checks passed
@etrepum etrepum deleted the iframe-mutation-example-and-fix branch January 16, 2025 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Error: $validatePoint: anchor point.offset > node.getTextContentSize() in StencilJS
3 participants