[lexical-table] Bug Fix: Prevent adjacent cell selection on triple-click#7213
Conversation
This commit fixes the issue where triple-clicking a table cell would incorrectly select adjacent cells. The implementation is based on @etrepum's approach from PR facebook#7005. The fix adds two new handlers: - Adds to: - Handle triple-click events in table cells - Check if block's parent is a table cell - Select only the block within the cell - Prevent selection of adjacent cells - Adds removeTripleClickHandler to: - Prevent default browser behavior for triple clicks - Use capture phase for event listeners - Handle all clicks >= 3 consistently Both handlers work together to ensure proper table cell selection behavior without affecting adjacent cells. Fixes facebook#6973 Co-authored-by: Bob Ippolito <bob@redivi.com>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Changed to use select(0) instead of selectStart() to properly select the entire block content within a table cell when triple-clicked. Updated Selection.spec.mjs test to expect consistent behavior - selecting the clicked cell's content when triple-clicked, removing browser-specific test conditions that previously expected different behaviors between Firefox and Chromium.
| const onMouseDown = (event: MouseEvent) => { | ||
| if (event.detail >= 3) { | ||
| // Prevent default multi-click behavior | ||
| event.preventDefault(); |
There was a problem hiding this comment.
Did you try putting an event.preventDefault() in the $tableClickCommand listener? Directly listening to the rootElement probably isn't necessary. Preventing default for all triple clicks in the document will possibly cause other problems that are hard to track down and may be difficult for users to work around.
| return false; | ||
| } | ||
| blockNode.select(0); | ||
| return true; |
There was a problem hiding this comment.
I think this might have the same intended effect as that click handler?
| return true; | |
| event.preventDefault(); | |
| return true; |
There was a problem hiding this comment.
Thanks for the suggestion! I tried removing the removeTripleClickHandler and keeping just the preventDefault() in $tableClickCommand, but I noticed an issue (I've shared a video demonstrating this):
Without the root-level triple click handler, there's a brief "flicker" where the adjacent cell gets selected before our table cell selection takes effect. This is probably happening because:
- The default triple-click behavior tries to select adjacent cells first
- Then our
$tableClickCommandruns and corrects the selection to just the clicked cell - This creates a visible "flicker" where you see the wrong selection briefly before it's corrected
The current implementation with both handlers prevents this flicker because:
- The root handler prevents the default triple-click behavior immediately
- Then
$tableClickCommandhandles the proper cell selection
Would you prefer we:
- Keep both handlers to prevent the flicker
- Remove the root handler and accept the flicker as a minor visual artifact
- Look for an alternative approach to prevent the flicker without the root handler?
You can see the flicker behavior clearly in the video I've shared. Let me know which direction you'd prefer to go with this.
7213-flickering-issue.mov
There was a problem hiding this comment.
I think the best approach is something like 3 to prevent problems due to the root handler. If we do have a root handler we should at least scope it down to only prevent events that $tableClickHandler would otherwise be handling. The issue here is that with this kind of handler in place we have no way of getting the native behavior back in situations where it might be desired (e.g. inside of a decorator that may or may not be in the table).
There was a problem hiding this comment.
Thanks for the feedback @etrepum! I've implemented the changes to move the triple-click handling to the table level:
- Removed the root-level handler
- Added table-specific triple-click handler in
applyTableHandlers - Scoped
preventDefault()to only trigger for table cell targets
This preserves native triple-click behavior outside tables while maintaining the desired selection behavior within tables. Please let me know if you'd like any adjustments to this implementation.
Previously, triple-click handling was done at the root level which prevented native triple-click behavior everywhere. This change: - Removes root-level triple-click handler (removeTripleClickHandler) - Adds table-specific triple-click handler in applyTableHandlers - Scopes preventDefault() to only trigger for table cell targets - Preserves native triple-click behavior outside tables Co-authored-by: Bob Ippolito <bob@redivi.com>
etrepum
left a comment
There was a problem hiding this comment.
Looks great, gave it a try in Chrome and Firefox and it seems to work well too. Nice job!
…le-click workaround Fixes facebook#7592. \## Context Browsers expand triple-click selections to cover an entire block, but leave the focus point at offset 0 of the *next* block. The selection visually looks like one paragraph is selected, but actually extends one position into the following block. Historically, this selection behavior caused some issues, including: - facebook#2648 Converting to other blocks after selection (e.g. Heading) - facebook#2660 Over-selection - facebook#3784 Selections have incorrect format - and another bug with table cell select-and-deletion mentioned in facebook#4512 PR facebook#4512 worked around these bugs in `onClick` (`LexicalEvents.ts`) by detecting `event.detail === 3` and forcibly re-selecting the parent block.. The `onClick` workaround is a poor fit, however, because it mutates the user's selection on *every* triple-click — breaking common cases like triple-clicking a paragraph with soft line breaks:(`<br>`), or triple-click-and-drag to select multiple lines, leading to issues like the one described in facebook#7592 \## Status of the original motivations A lot has changed since facebook#4512! - Table cell deletion was fixed independently in facebook#7213; the tables no longer rely on the `onClick` workaround. - facebook#2660 No longer manifests without the triple-click handler - facebook#3784 No longer manifests without the triple-click handler - facebook#2648 The heading format tool (and related tools — paragraph, quote, code block — all routed through `$setBlocksType`) is the only remaining caller depending on the workaround. Which means the triple-click handler can be removed if `$setBlocksType` is updated to handle overselection. \## This change Fix `$setBlocksType` (`@lexical/selection`) to skip converting the focus's block when no content of that block is actually selected — specifically, when the focus is a text-type point at offset 0 of the block's first descendant and the block differs from the anchor's block. This matches the visual semantics users expect, and covers both triple-click overselection and deliberate drag-select-to-start-of-next-block. With that root cause fixed, the `onClick` triple-click workaround is removed. \## Tests - Existing e2e tests for triple-click + heading conversion continue to pass without the `onClick` workaround (`Selection.spec.mjs`, `TextFormatting.spec.mjs`). - `$setBlocksType` unit tests in `packages/lexical-selection/src/__tests__/unit/LexicalSelection.test.tsx` continue to pass; the fix only affects text-type focus points, so the existing element-point-based tests are unchanged.
Description
Current behavior:
Changes added by this PR:
$tableClickCommandto handle triple-click events specifically for table cells: (The implementation is based on @etrepum's approach from PR [WIP][lexical-table] Feature: Use custom selection behavior for table cell triple click #7005)select(0)to properly select the entire block contentremoveTripleClickHandlerto:Selection.spec.mjs:Both handlers work together to ensure proper and consistent table cell selection behavior.
Closes #6973
Closes #7005
Test plan
Before
lexical-bug-6973.mov
After
lexical-6973-fix.mov