Skip to content

Conversation

@randal-atticus
Copy link
Contributor

@randal-atticus randal-atticus commented Feb 2, 2026

Description

I was working on a fix for #7154, that is, improving the selection behaviour for nested cells.

I gave up on the fix because the current selection handler in LexicalTableSelectionHelpers.ts is registered for each table. As it stands, a selection in a nested cell is also considered a selection inside the parent cell. Both tables end up firing $setSelection and triggering each others SELECTION_CHANGED_COMMAND, and they end up fighting for control. This leads to flickering and the wrong element being selected, see:

Screen.Recording.2026-02-02.at.2.23.41.pm.mov

I would like to refactor table selection to use a monolithic handler which is only registered and fired once.

I propose merging these test to demonstrate the bug and then refactoring (with a fix for these tests) in another PR.

Test plan

Before

Visual demonstration of the E2E tests (if the .fixme is removed). Apologies for the poor quality, had to downscale to fit Github limits.

Screen.Recording.2026-02-02.at.2.14.40.pm.mov

After

No changes to logic.

@vercel
Copy link

vercel bot commented Feb 2, 2026

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

Project Deployment Actions Updated (UTC)
lexical Ready Ready Preview, Comment Feb 2, 2026 3:30am
lexical-playground Ready Ready Preview, Comment Feb 2, 2026 3:30am

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 Feb 2, 2026
if (mouseDown) {
await page.mouse.down();
}
await page.mouse.move(toX, toY, slow ? 10 : 1);
Copy link
Contributor Author

@randal-atticus randal-atticus Feb 2, 2026

Choose a reason for hiding this comment

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

I think this argument was wrong? The dragging behaviour was only testable once I used the steps parameter instead of passing the number in. https://playwright.dev/docs/api/class-mouse#mouse-move

I changed the function to take the number of steps rather than a boolean, and reduced the "slow" value from 10 to 5 to speed up some tests (at 10 steps, the test takes an additional 200ms)

@randal-atticus randal-atticus marked this pull request as ready for review February 2, 2026 03:59
@randal-atticus randal-atticus changed the title [lexical-playground] Chore: Add (disabled) E2E tests for nested tables [lexical-playground] Chore: Add (disabled) E2E tests for nested table drag-selection Feb 2, 2026
@etrepum etrepum added the extended-tests Run extended e2e tests on a PR label Feb 2, 2026
etrepum
etrepum previously approved these changes Feb 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 agree that the table handling code should be monolithic, and I'm sure that it could be made more efficient with some API changes. The last refactor of that code was focused on fixing bugs without the sort of structural changes it really needs.

@etrepum etrepum dismissed their stale review February 3, 2026 01:42

I think these are legit e2e test failures, but github has been having issues today so I restarted them. Some of these changes might break existing tests?

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