[lexical] Bug Fix: handle triple-click overselection in $setBlocksType#8517
Conversation
…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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
$setBlocksType
|
I’m not sure removing it is going to be the right call. Handling over-selection gracefully in $setBlocksType is a good idea but I don’t think it’s the only case remaining where over-selection is an issue. It’s better to prevent it from happening in the first place whenever we can, but since this over-selection can be intentional in certain cases it’s ideal to manage it in places such as the click handler where we are certain it’s happening unintentionally. |
|
@etrepum After spending a few days trying to fix the triple-click handler to address #7592 and other edge cases (triple-click on a Just to share my sense of the complexity: in Lexxy (an editor built on Lexical) this was the PR that tried to "fix" triple-click handling (note this approach can probably be used in Lexical, and might cover more edge cases than #8490): basecamp/lexxy#1048 But what we eventually went with was this change, to stop propagation of the click event: basecamp/lexxy#1050 If the decision is made to keep the click handler in Lexical, would you be open to moving it into an ordinary handler attached to the |
|
Open to anything that works and can be done in a backwards compatible enough manner. The new |
|
One thing that would be particularly helpful is to establish a set of acceptance tests that cover all of the cases that you think should work, especially if some of them are failing today. |
|
I haven't done anything more than manual QA but I think a solution like this works, I agree that the paragraphNode.select(0) solution was naive and brute force but it's fairly straightforward to address the over-selection more precisely (with APIs that have been added since the original workaround). It might also make sense to intercept the DOM selection change eagerly so there isn't a frame of overselection displayed. diff --git a/packages/lexical/src/LexicalEvents.ts b/packages/lexical/src/LexicalEvents.ts
index 1f3c370fc..d096fa404 100644
--- a/packages/lexical/src/LexicalEvents.ts
+++ b/packages/lexical/src/LexicalEvents.ts
@@ -23,19 +23,30 @@ import invariant from 'shared/invariant';
import warnOnlyOnce from 'shared/warnOnlyOnce';
import {
+ $caretRangeFromSelection,
+ $getCaretRange,
+ $getCaretRangeInDirection,
+ $getChildCaret,
$getPreviousSelection,
$getRoot,
$getSelection,
+ $getSiblingCaret,
$isBlockElementNode,
+ $isChildCaret,
$isDecoratorNode,
$isElementNode,
$isLineBreakNode,
$isNodeSelection,
$isRangeSelection,
$isRootNode,
+ $isSiblingCaret,
$isTabNode,
$isTextNode,
+ $isTextPointCaret,
+ $normalizeCaret,
+ $rewindSiblingCaret,
$setCompositionKey,
+ $setSelectionFromCaretRange,
BLUR_COMMAND,
CLICK_COMMAND,
COMMAND_PRIORITY_EDITOR,
@@ -526,16 +537,49 @@ function onClick(event: PointerEvent, editor: LexicalEditor): void {
// case visually it looks like a single element content is selected, focus node
// is actually at the beginning of the next element (if present) and any manipulations
// with selection (formatting) are affecting second element as well
- const focus = selection.focus;
- const focusNode = focus.getNode();
- if (anchorNode !== focusNode) {
- const parentNode = $findMatchingParent(
- anchorNode,
- node => $isElementNode(node) && !node.isInline(),
+ const range = $getCaretRangeInDirection(
+ $caretRangeFromSelection(selection),
+ 'next',
+ );
+ let focusCaret = range.focus;
+ // Move it out of the next TextNode if none of it is selected
+ if (
+ $isTextPointCaret(focusCaret) &&
+ range.anchor.origin !== focusCaret.origin &&
+ focusCaret.offset === 0
+ ) {
+ focusCaret = $rewindSiblingCaret(focusCaret.getSiblingCaret());
+ }
+ // Move it behind a single LineBreakNode
+ if (
+ $isSiblingCaret(focusCaret) &&
+ range.anchor.origin !== focusCaret.origin &&
+ $isLineBreakNode(focusCaret.origin)
+ ) {
+ focusCaret = $rewindSiblingCaret(focusCaret);
+ }
+ // Move the focus out of the start of any elements
+ while (
+ $isChildCaret(focusCaret) &&
+ range.anchor.origin !== focusCaret.origin
+ ) {
+ focusCaret = $rewindSiblingCaret(
+ $getSiblingCaret(focusCaret.origin, 'next'),
);
- if ($isElementNode(parentNode)) {
- parentNode.select(0);
+ }
+ if (focusCaret !== range.focus) {
+ // Move it inside the containing element
+ if (
+ $isSiblingCaret(focusCaret) &&
+ $isElementNode(focusCaret.origin)
+ ) {
+ focusCaret = $normalizeCaret(
+ $getChildCaret(focusCaret.origin, 'previous'),
+ ).getFlipped();
}
+ $setSelectionFromCaretRange(
+ $getCaretRange(range.anchor, $normalizeCaret(focusCaret)),
+ );
}
}
} else if (event.pointerType === 'touch' || event.pointerType === 'pen') { |
etrepum
left a comment
There was a problem hiding this comment.
With tests and without the events code change I think this looks good
Co-authored-by: Bob Ippolito <bob@redivi.com>
…Type Carries forward the $setBlocksType fix from PR facebook#8517 with a correction to the last commit, plus regression tests. Background: On a triple-click, browsers expand the selection to cover an entire block but leave the focus point at offset 0 of the *next* block. $setBlocksType now skips the focus block when the focus is a text point at offset 0 of that block's first descendant and the block differs from the anchor block. The last commit on PR facebook#8517 ("Apply suggestions from code review") relaxed $isPointAtBlockStart by dropping the point.type !== 'text' guard. That broke the existing element-point cases: * "Two empty elements, same top-element" * "Full editor selection with a mix of top-elements" where the focus is an element point at offset 0 of an empty paragraph and the user *does* want that paragraph converted. The text-type guard is restored; the node.is(block) key-equality change from the same commit is kept since it is a safe robustness improvement. Tests added for the forward triple-click scenarios that the fix specifically targets: * Plain adjacent paragraphs * Focus inside a nested inline (link) at offset 0 of next block * Non-zero focus offset still converts both blocks * Three-block forward selection only skips the focus block * Focus at offset 0 of a non-first descendant still converts the block
|
@etrepum Thank you for cleaning this PR up. The final changes work well in Lexxy, and so I'll remove our triple-click workaround. 🎉 |
Fixes #7592. Alternative to #8490.
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:
PR #4512 worked around these bugs in
onClick(LexicalEvents.ts) by detectingevent.detail === 3and forcibly re-selecting the parent block..The
onClickworkaround 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 #7592Status of the original motivations
A lot has changed since #4512!
onClickworkaround.$setBlocksType) is the only remaining caller depending on the workaround.Which means the triple-click handler can be removed if
$setBlocksTypeis 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
onClicktriple-click workaround is removed.Tests
onClickworkaround (Selection.spec.mjs,TextFormatting.spec.mjs).$setBlocksTypeunit tests inpackages/lexical-selection/src/__tests__/unit/LexicalSelection.test.tsxcontinue to pass; the fix only affects text-type focus points, so the existing element-point-based tests are unchanged.Before
I will add screencaps this evening.
After
I will add screencaps this evening.