[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 <[email protected]>
kirandash
requested review from
zurfyx,
fantactuka,
acywatson,
Fetz,
ivailop7,
Sahejkm and
potatowagon
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
kirandash
changed the title
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.
kirandash
changed the title
| 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 <[email protected]>
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