Skip to content

[lexical-table] Refactor: use monolithic listener for table SELECTION_CHANGE_COMMAND and deselection handler#8187

Merged
etrepum merged 8 commits intofacebook:mainfrom
randal-atticus:feat-singleton-table-selection-handler
Mar 4, 2026
Merged

[lexical-table] Refactor: use monolithic listener for table SELECTION_CHANGE_COMMAND and deselection handler#8187
etrepum merged 8 commits intofacebook:mainfrom
randal-atticus:feat-singleton-table-selection-handler

Conversation

@randal-atticus
Copy link
Contributor

Description

As discussed in #8107 the SELECTION_CHANGE_COMMAND and various pointer events are registered for each table in applyTableHandler. There is also a lot of recursive updates going on. It would be better to have a monolithic handler that handles table selection events, as it makes it far easier to reason about what is happening when you have nested tables.

This PR tackles some low hanging fruit:

  • SELECTION_CHANGE_COMMAND is hoisted up to registerTableSelectionObserver (i.e. only once). It still runs the old logic ($handleTableSelectionChangeCommand) on each table in sequence though
  • pointerDownCallback, which handles clearing table highlight selections, is similarly hoisted up.

Next up is a refactor of $handleTableSelectionChangeCommand so that it just runs once and behaves as expected across all the tables.

Test plan

Before

N/A, this is a refactor

After

N/A no behaviour changes, all tests passing

@vercel
Copy link

vercel bot commented Mar 2, 2026

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

Project Deployment Actions Updated (UTC)
lexical Ready Ready Preview, Comment Mar 4, 2026 6:35am
lexical-playground Ready Ready Preview, Comment Mar 4, 2026 6:35am

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 2, 2026
@etrepum etrepum added the extended-tests Run extended e2e tests on a PR label Mar 2, 2026
Copy link
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've only done a quick look at the code here and it seems good, but it might make sense to explicitly add some test coverage to make sure the multi-table (and nested-table?) scenarios work as expected? I don't know what our current coverage of that looks like.

@randal-atticus
Copy link
Contributor Author

randal-atticus commented Mar 4, 2026

I'll add more multi-table coverage. The nested table scenarios are not well tested, because the current behaviour (when using the mouse) is frankly quite broken, with seemingly nondeterministic outcomes. I tried to add some tests for that in #8107 but I feel like it would now be better to include new nested table tests alongside the a more stable selection handler.

@etrepum etrepum added this pull request to the merge queue Mar 4, 2026
Merged via the queue into facebook:main with commit a3d33be Mar 4, 2026
37 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