[lexical-utils] Add dedupeSelectionRects: fix duplicate/extra selection rects on WebKit (#7106, #7492)#8709
[lexical-utils] Add dedupeSelectionRects: fix duplicate/extra selection rects on WebKit (#7106, #7492)#8709pro-vi wants to merge 10 commits into
Conversation
…OnRange createRectsFromDOMRange (and Range.getClientRects()) can return duplicate or contained rects, and WebKit emits a spurious wider rect alongside the real text rect on some blocks (balanced or letter-spaced headings). Drawn as fills, the duplicates read brighter than a single rect and the wider rect paints the "extra empty selection area" reported in facebook#7106. dedupeSelectionRects drops zero-area rects and any rect that contains another, keeping the smaller text-hugging one, with a 1px tolerance. positionNodeOnRange now runs the rects through it, so every fake-selection consumer (markSelection, selectionAlwaysOnDisplay) is cleaner on WebKit. Pure function, exported, covered by 7 unit tests, no API change.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…w test coverage Three cases confirming the keep-smaller-on-containment rule only fires for the duplicate / spurious-wider cases, never a legitimate rect: horizontally disjoint same-row boxes both survive, a wide rect (trailing whitespace) is not clipped against a disjoint narrower row, and every row of a ragged multi-row selection (e.g. RTL) is kept.
|
Not a deep review, just a couple of notes from reading through. On the placement open question — I'd lean toward keeping it inside One thing about the same-line containment comment — the inputs here aren't raw The These are just my read though — the maintainer may see it differently. Hope it helps. |
…integration and under-paint tests createRectsFromDOMRange's selectionSpansElement already drops the full-block-width spurious rect on the positionNodeOnRange path, so there dedupeSelectionRects mainly prevents duplicate-doubling; the facebook#7106 extra-area paint is addressed for consumers that feed raw getClientRects(). Document that, and the keep-smaller under-paint limitation for overlapping inline content -- there is no rect-only fix, since the wide rect is geometrically indistinguishable from the spurious-wider facebook#7106 rect. Tests: a createRectsFromDOMRange + dedupe integration block (its asymmetric, adjacent-only pass can leave a same-row contained pair; dedupe collapses it deterministically, keeping the smaller by design), and a Chromium + WebKit browser characterization of the overlapping-content under-paint.
|
Following up — I tried hard to construct your case, and the result splits in two. Your literal scenario (normal inline-block nesting, no overlap) I could not construct: in normal flow same-line rects tile horizontally disjoint, so the only wide rect is the block-box one, which But the underlying worry is real — I found a different selection that breaks the same assumption (Chromium + WebKit, real Two honest qualifications:
So: scoped to overlapping inline content (incl. baseline-shifted inline decorators, which isn't exotic), no clean dedupe fix, and the real fix is the focused-draw follow-up. I've scoped the docstring's "horizontally disjoint" caveat and the PR text accordingly rather than change the rule. Agreed on keeping the dedupe in the consumer over folding it into |
potatowagon
left a comment
There was a problem hiding this comment.
Reviewed by Navi (Tater Thoughts Bobblehead) on behalf of @potatowagon.
Assessment: LGTM ✅
What this PR does: Adds a new dedupeSelectionRects utility to @lexical/utils that removes duplicate/contained client rects before they are drawn as selection fills. Fixes the "extra empty selection area" bug reported in #7106 and the doubled-opacity duplicate-rect issue in #7492. Integrates into positionNodeOnRange as a post-filter after createRectsFromDOMRange.
What I checked:
-
Algorithm correctness: The dedupe logic is sound — drops zero-area rects, then for each candidate: (a) skips if it contains an already-kept smaller rect, (b) evicts larger kept rects that contain it. The 1px tolerance accounts for sub-pixel jitter. The "keep smaller" heuristic is well-justified: the spurious wider rect from #7106 always contains the real text rect, so keeping the smaller one is correct.
-
Edge cases: The known limitation (overlapping inline content via negative margins/transforms can cause under-painting) is explicitly documented in the function docstring AND covered by a dedicated characterization test (
dedupeSelectionRectsUnderpaint.test.ts). This is honest engineering — the tradeoff is acknowledged and tested rather than hidden. -
Test coverage: Excellent — 2 test files:
- Unit tests (188 lines): identical rects, contained pairs, order independence, multi-row, zero-area, disjoint same-row, ragged RTL, pipeline integration with
createRectsFromDOMRange - Browser test (132 lines): real layout characterization of the under-paint limitation with CSS overlapping inline content
- Unit tests (188 lines): identical rects, contained pairs, order independence, multi-row, zero-area, disjoint same-row, ragged RTL, pipeline integration with
-
API surface: New export
dedupeSelectionRectsfrom@lexical/utils. Flow types updated. Generic typing (<Rect extends RectLike>) accepts bothDOMRectListandClientRect[]. No removed/renamed exports. -
www compat: No concerns — purely additive export, no behavioral changes to existing APIs. The integration point (
positionNodeOnRange) is an internal pipeline helper; adding the dedupe step only removes spurious rects that were already visual artifacts. -
Performance: O(n²) in the worst case but n is always small (number of client rects in a selection, typically <20). No concern.
Ready to approve. Clean, well-documented fix with explicit tradeoff documentation.
|
Not to derail any of this, but did you investigate using the CSS Custom Highlight API as an alternative to computing the rects when available? #8550 added some support for using them with yjs if configured and when the platform supports it. Might be a better solution going forward. We should absolutely improve this path either way, not a blocker at all for this PR (I have not read the code yet), just thought I'd mention it in case that wasn't on your radar. |
…tion platform-independent The browser characterization hard-coded pixel thresholds tuned to macOS/Linux monospace metrics, so it failed on Windows (Consolas renders the run narrower than the 90px threshold). Rebuild it so the wide rect is a text run with a contained, sub-pixel-raised overlapping box, and assert structurally -- find the contained pair, assert the wider rect is dropped, and probe that its left edge is left uncovered -- with no pixel thresholds. Passes Chromium, Firefox and WebKit.
What
Range.getClientRects()can return rects that are duplicated or contained within another, and WebKit emits a spurious wider rect alongside the real text rect on some blocks (balanced or letter-spaced headings are the reproducible case). Drawn as semi-transparent fills, the duplicates read brighter than a single rect, and the wider rect paints the "extra empty selection area" reported in #7106.This adds
dedupeSelectionRectsto@lexical/utilsand runs the rects through it inpositionNodeOnRange, so every fake-selection consumer (markSelection,selectionAlwaysOnDisplay, the Extension) gets clean rects on WebKit.It drops zero-area rects and any rect that contains another, keeping the smaller text-hugging one, with a 1px tolerance for sub-pixel jitter. Pure, exported, no API change. Typed on the structural subset of
ClientRectit reads, so it also accepts a liveDOMRectListfromgetClientRects()and is unit-testable without a DOM.What it actually does on each path (measured in Chromium + WebKit)
Being precise about scope:
positionNodeOnRangepath (this PR's wiring):createRectsFromDOMRange's ownselectionSpansElementfilter already drops the full-block-width spurious rect for the reproducible Bug: Text selection on safari has extra empty selection area #7106 shapes (short / tracked / balanced headings). So herededupeSelectionRectsmainly prevents the duplicate-doubling that survives that helper's asymmetric, adjacent-only pass.getClientRects()path: consumers feeding raw rects (the focused-draw below) have noselectionSpansElement, so the block-box spurious rect reaches the dedupe and keep-smaller is what removes the Bug: Text selection on safari has extra empty selection area #7106 extra-area paint there.So the dedupe stands on its own as a doubling guard for the existing utilities, and is the primitive the focused-draw follow-up is built on.
Why this is only part of #7106
The dedupe cleans the rects but does not stop WebKit's native
::selectionfrom painting across the whole block box in the first place. The extra empty area #7106 reports is two things stacked: that block-box native paint, and the spurious widergetClientRects()rect. The first has no CSS fix — you cannot constrain WebKit's block-box::selectionwidth back to the glyph box — so the complete fix is the CodeMirrordrawSelectionapproach: stop relying on native::selectionfor the focused selection and draw your own rects from the text client-rects, gated to WebKit.I have that shipped in a Tauri / WKWebView build, as a
registerDrawSelection(editor, options?): one overlay layer, rAF-coalesced triggers (the coalescing is load-bearing — WebKit's triple-click over-selects and Lexical corrects it the same tick, so a raw draw flashes the pre-correction state for one frame), and a.draw-selection-active ::selection { background-color: transparent; color: <fg>; -webkit-text-fill-color: <fg> }contract that hides the native paint and normalizes the selected-text color so syntax-highlighted code stays legible. That contrast normalization also answers #7492.I'm keeping that out of this PR because it is a new public API surface, and I'd rather get a read on where it belongs (a
@lexical/utilssibling ofmarkSelection/selectionAlwaysOnDisplay, or a@lexical/extensionnext toSelectionAlwaysOnDisplayExtension) before coding it here. The dedupe stands on its own and helps the existing utilities immediately, so it seemed worth landing first. Happy to open theregisterDrawSelectionfollow-up once you point me at the right home.Known limitation — overlapping inline content
The keep-smaller rule assumes same-line rects are horizontally disjoint, which holds in normal flow. It does not hold for overlapping inline content (a negative margin, a transform, or a baseline-shifted inline decorator): a real sub-fragment can sit inside a wider real-text rect on the same row, and if a sub-pixel top offset also lets both clear
createRectsFromDOMRange's asymmetric overlap filter, keep-smaller drops the wider rect and under-paints the glyphs it uniquely covered (reproduced in Chromium + WebKit — seededupeSelectionRectsUnderpaint.test.ts). There is no rect-only fix: that wider rect is geometrically indistinguishable from the spurious-wider (#7106) rect, so keeping it would re-introduce the extra-area paint. It is documented in the source; the real resolution is the focused-draw path, not a dedupe change.Test plan
dedupeSelectionRects.test.ts(unit): keep-smaller / duplicate / zero-area / empty / sub-pixel / disjoint / ragged-row cases, plus acreateRectsFromDOMRange + dedupeSelectionRectsintegration block showing the helper's asymmetric pass can leave a same-row contained pair and the dedupe collapses it deterministically (order-independent), keeping the smaller by design.dedupeSelectionRectsUnderpaint.test.ts(browser, Chromium + WebKit): characterizes the overlapping-content under-paint above against real layout and the real functions.check-flow-typesall clean.Open questions
positionNodeOnRange(this PR), or folded intocreateRectsFromDOMRange? Leaning to keep it in the consumer —createRectsFromDOMRangeis a public export, and changing what it returns changes the contract for external callers (thanks @mayrang). The measured finding above reinforces this: in that path the block-box spurious rect is already handled byselectionSpansElement, so folding would not add Bug: Text selection on safari has extra empty selection area #7106 value there.registerDrawSelectionfollow-up should live:@lexical/utilsor@lexical/extension.