Skip to content

[lexical-table] Bug Fix: Improve nested table selection by using monolithic pointer event handling#8193

Merged
etrepum merged 5 commits intofacebook:mainfrom
randal-atticus:refactor-one-table-selection-helper
Mar 5, 2026
Merged

[lexical-table] Bug Fix: Improve nested table selection by using monolithic pointer event handling#8193
etrepum merged 5 commits intofacebook:mainfrom
randal-atticus:refactor-one-table-selection-helper

Conversation

@randal-atticus
Copy link
Copy Markdown
Contributor

@randal-atticus randal-atticus commented Mar 5, 2026

Description

Continuation of #8187.

Previously, clicking a nested table would result in each table firing its own pointerDown handler, and also register their own window.onPointerMove handlers, resulting in difficulty clicking on any cells in nested tables (see "Before").

This PR moves to register just a single pointerDown event for all tables (and associated onPointerMove/onPointerUp). Now these handlers only run against the deepest relevant table.

This is not the final PR in this series. Some additional handling is needed to correctly exit isHighlightingCells mode, as well as handle dragging between inner-table cells and the parent.

Currently no new E2E tests because the previous behaviour is quite hard to test reliably.

Test plan

Before

Observe that clicking and dragging on cells in nested tables often results in the parent table being selected as well (because its onPointerMove handler is firing as well)

Screen.Recording.2026-03-05.at.6.45.52.pm.mov

After

Screen.Recording.2026-03-05.at.6.46.11.pm.mov

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 5, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
lexical Ready Ready Preview, Comment Mar 5, 2026 5:13pm
lexical-playground Ready Ready Preview, Comment Mar 5, 2026 5:13pm

Request Review

@meta-cla meta-cla 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 Mar 5, 2026

return mergeRegister(
registerWindowHandlers(),
registerTableWindowHandlers(editor, tableSelections),
Copy link
Copy Markdown
Contributor Author

@randal-atticus randal-atticus Mar 5, 2026

Choose a reason for hiding this comment

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

This is leaking the Map of TableObservers out, but felt better than putting a bunch of selection-specific code into LexicalTablePluginHelpers. Perhaps a better way would to pass down a TableObservers class that limits the exposure?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it's fine for internal implementation details to leak across the implementation, it's not part of the API

@randal-atticus randal-atticus marked this pull request as ready for review March 5, 2026 10:22
@etrepum etrepum added the extended-tests Run extended e2e tests on a PR label Mar 5, 2026
Copy link
Copy Markdown
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

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

I didn't look super closely at every change but it looks like a straightforward refactor and should be fine to merge assuming all tests are passing


return mergeRegister(
registerWindowHandlers(),
registerTableWindowHandlers(editor, tableSelections),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it's fine for internal implementation details to leak across the implementation, it's not part of the API

@etrepum
Copy link
Copy Markdown
Collaborator

etrepum commented Mar 5, 2026

GitHub actions are currently broken so I will have to check back in later

@etrepum etrepum added this pull request to the merge queue Mar 5, 2026
Merged via the queue into facebook:main with commit 2136192 Mar 5, 2026
34 of 60 checks passed
@etrepum etrepum mentioned this pull request Mar 19, 2026
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.

2 participants