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-playground] Bug Fix: Preserve the selection using the link editor from a table #6865

Merged
merged 4 commits into from
Nov 26, 2024

Conversation

etrepum
Copy link
Collaborator

@etrepum etrepum commented Nov 25, 2024

Description

When used in a table cell, the link editor loses focus and the link is not applied. Finding the root cause of this one was tricky, but the gist of it was that we were calling editor.update on mousemove for TableHoverActions and TableCellResizer and because of how reconciliation currently works the DOM selection is always resolved during an editor.update cycle so it would get reset under these circumstances since the DOM selection was in a portal for the link editing modal.

Failed debugging strategies:

  • Trying to narrow it down by disabling one mousemove plugin at a time didn't work because two plugins had the same bug
  • Replacing EditorState._selection with a getter/setter that had debugging logs and a breakpoint didn't work because this was happening during reconciliation when this change goes through module global variables and not a $setSelection

What worked:

  • Setting a breakpoint in updateDOMSelection when the selection changed in this specific way (prevSelection from a RangeSelection, nextSelection to null, domSelection anchor on .link-input)
  • Adding a local variable that stored updateFn to the scheduleMicroTask(() => { … }) closure so that I could track down which update function was responsible for this happening in the debugger scope (normally this information is lost since that function only closes over editor)

Future considerations:

I've never loved how editor reconciliation always tries to eagerly do things with the selection based on the DOM, because of situations like this. Maybe we should at least add an update tag similar to skip-scroll-into-view which avoids the forced selection reconciliation without having to temporarily set the editor to readonly or do other weird things.

Closes #6821

Test plan

Before

link-edit-before.mov

After

link-edit-after.mov

Copy link

vercel bot commented Nov 25, 2024

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 Nov 26, 2024 5:14pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 26, 2024 5:14pm

@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 Nov 25, 2024
Copy link

github-actions bot commented Nov 25, 2024

size-limit report 📦

Path Size
lexical - cjs 30.91 KB (0%)
lexical - esm 30.79 KB (0%)
@lexical/rich-text - cjs 39.67 KB (0%)
@lexical/rich-text - esm 32.65 KB (0%)
@lexical/plain-text - cjs 38.27 KB (0%)
@lexical/plain-text - esm 29.93 KB (0%)
@lexical/react - cjs 41.4 KB (0%)
@lexical/react - esm 34 KB (0%)

@etrepum etrepum changed the title [lexical-playground] Bug Fix: Restore the selection when submitting the link editor [lexical-playground] Bug Fix: Preserve the selection using the link editor from a table Nov 25, 2024
@etrepum etrepum added this pull request to the merge queue Nov 26, 2024
Merged via the queue into facebook:main with commit ec66f75 Nov 26, 2024
43 checks passed
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: [lexical-link] can't add link successful
3 participants