Skip to content

[lexical-table] Refactor: Call $handleTableSelectionChangeCommand once instead of per-table#8200

Merged
etrepum merged 6 commits intofacebook:mainfrom
randal-atticus:fix-nested-table-partial-selection
Mar 11, 2026
Merged

[lexical-table] Refactor: Call $handleTableSelectionChangeCommand once instead of per-table#8200
etrepum merged 6 commits intofacebook:mainfrom
randal-atticus:fix-nested-table-partial-selection

Conversation

@randal-atticus
Copy link
Copy Markdown
Contributor

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

Description

Further refactors the Table's SELECTION_CHANGE_HANDLER ($handleTableSelectionChangeCommand) so that it only needs to run once.

Cheating a bit at the end because there's still some logic that runs over each table, but I think it's preferable to refactor this in pieces.

Key changes:

  • nextFocus, shouldCheckSelection are hoisted up into an extension-wide 'TableObservers' class (which now also contains the old map of Table NodeKey -> TableObserver). Think of it as a home for state across all tables.
  • nextFocus/shouldCheckSelection therefore only need to be handled once, instead of per-table.
  • Split the remaining handler logic into 3 parts.
    • $fixTableSelectionForSelectedTable (this also now only needs to run once)
    • $fixRangeSelectionForTable
    • $syncTableSelectionState

Note: No existing bugs fixed in this PR but these refactors are a precursor to some important nested table bug fixes.

Test plan

Before

Insert relevant screenshots/recordings/automated-tests

After

Insert relevant screenshots/recordings/automated-tests

…nextFocus, shouldCheckSelection

introduces TableObservers for global table state (nextFocus, shouldCheckSelection),
…d blocks, and pull table-selection-fixup out
@vercel
Copy link
Copy Markdown

vercel bot commented Mar 9, 2026

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

Project Deployment Actions Updated (UTC)
lexical Ready Ready Preview, Comment Mar 10, 2026 11:30pm
lexical-playground Ready Ready Preview, Comment Mar 10, 2026 11:30pm

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 9, 2026
);
}
$setSelection(newSelection);
$addHighlightStyleToTable(editor, tableObserver);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Realised that with the $syncTableSelectionState pass happening after all of this, there was no need to explicitly add the styling.

@etrepum
Copy link
Copy Markdown
Collaborator

etrepum commented Mar 9, 2026

Looks like there's a firefox issue here

…ged selection showing wrong menu)

clear selection
if ($isTableSelection(selection)) {
const currentSelectionCounts = computeSelectionCount(selection);
updateSelectionCounts(computeSelectionCount(selection));
const isCollapsedTableSelection = selection.anchor.is(selection.focus);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you merge a cell and then enter a single-cell table selection, it would consider the cell mergeable. This just happened to break one of the tests in Firefox because it's easier to set up a single-cell table selection (click, then shift-click a cell - this only works in Firefox).

In the code before, a mergeable cell is not considered un-mergeable so one of the tests was failing in Firefox.

Now:

Image

@randal-atticus
Copy link
Copy Markdown
Contributor Author

Assuming tests pass on the latest commit, one bug was a silly typo in getAndClearShouldCheckSelectionForTable, another was a selection quirk when unmerging cells, last one was what seemed to be a bug with detecting mergeable/unmergeable table selections in the playground.

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