[lexical-utils] Add a colors option to markSelection / selectionAlwaysOnDisplay (#7492)#8710
[lexical-utils] Add a colors option to markSelection / selectionAlwaysOnDisplay (#7492)#8710pro-vi wants to merge 1 commit into
Conversation
…book#7492) markSelection's default styler hardcodes background: 'Highlight', so a consumer that wants a different selection color has to re-derive the whole styler. This adds an optional `colors` argument ({ background }) to markSelection, selectionAlwaysOnDisplay, and SelectionAlwaysOnDisplayExtension, and exposes a createDefaultOnReposition(colors) factory so a custom onReposition can reuse the default rect styling with a different color. Defaults to 'Highlight', so every current call site is byte-identical. The other half of facebook#7492 (the selection going invisible over text that has its own background, e.g. code blocks) is a paint-order problem rather than a color choice; the contrast measurement and the proposed fix are in the PR description.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
mayrang
left a comment
There was a problem hiding this comment.
Not a deep review, but a few notes from skimming — hopefully helpful while you iterate.
On the implementation side:
I think the extension config colors should have a concrete default like {background: 'Highlight'} rather than undefined. An undefined-default field forces every consumer that overrides anything else to remember it's not really a partial — and once a second undefined-default lives on the same config, the pattern is harder to walk back later.
The nested colors object would also be safer with a mergeConfig. Right now it has one field so a shallow merge looks safe — but the type is clearly designed to grow (your own open question 4 hints at foreground and a lift opt-in). If a second consumer ever passes colors: {background: 'red'}, it would wipe whatever sibling field a previous layer set. I'd land the mergeConfig shape with this PR rather than retrofit later.
The test for createDefaultOnReposition covers the no-colors and concrete-color paths but doesn't pin colors: {} or colors: {background: undefined} — the guard handles both, so it's worth covering. An end-to-end pass on selectionAlwaysOnDisplay(editor, undefined, {background: 'red'}) would also exercise the threading itself, not just the factory.
On the open questions:
-
I think the parity (util + extension both) is the right call, since
markSelectionand the extension already mirror each other. -
I'd keep it positional rather than moving to an options bag. The function is public with a
.js.flowdeclaration, and switching the shape would churn every existing caller for one new option. The lexical utilities I've looked at (mergeRegister,\$findMatchingParent,positionNodeOnRange) are all positional too. -
I think
createDefaultOnRepositionshould be exported. The motivation in the issue is "I want to change the color without re-deriving the offset styling", and keeping the factory internal would undo that.positionNodeOnRangeis already public on the same layer. -
I'd defer the z-index lift to #8709 and document the limitation here. Your contrast measurement is the convincing part — lifting the blurred rect buys visibility, but
::selectiondoesn't apply when the selection isn't focused, so the token contrast can't really be recovered. The focused-selection path in #8709 is what can set::selection { color }, so the lift fits naturally there.
These are just my read though — the maintainer may see it differently. Hope something in here is useful.
What
markSelection'sdefaultOnRepositionhardcodesbackground: 'Highlight', so a consumer that wants a different selection color has to re-derive the whole styler (copying itsmarginTop/paddingTop/paddingBottomoffsets). That is the configurability #7492 asks for.This adds an optional
colorsargument ({ background }) tomarkSelection,selectionAlwaysOnDisplay, andSelectionAlwaysOnDisplayExtension, and exports acreateDefaultOnReposition(colors)factory so a consumer passing its ownonRepositioncan reuse the default rect styling with a different color:backgrounddefaults to'Highlight', so every current call site is byte-identical. Covered bycreateDefaultOnReposition.test.ts; the existingmarkSelectiontest still passes.The harder half of #7492 (not coded here, asking first)
The reporter's other case is the fake selection going invisible over text that carries its own background, and that one is a paint-order problem, not a color choice.
positionNodeOnRangeprepends itsposition: relativewrapper toroot.parentElement, so the rects sit behind the glyphs and below the editable. When a span has its own opaque background (inline code, a code block), that background paints over the behind-text rect and the selection disappears for the length of that span. A differentbackgroundcolor cannot fix that, the rect is occluded, not mis-colored.The way to make it visible is to lift the rect above the content (
z-index). But a translucent rect lifted over syntax-highlighted code then washes the token colors out. I measured this by canvas-compositing each token color over the rect versus over the code background, in dark mode:Below ~2:1 selected code is hard to read, so lifting the rect makes the selection visible but the code less legible, a net loss for exactly the code-block case the issue is about.
One correction, since I had this muddled at first: the clean way to recover the contrast is to recolor the selected text to one foreground color via a
::selectionrule, but that only works for a focused native selection.markSelection/selectionAlwaysOnDisplaydraw the fake selection when the editor is blurred (the real selection is elsewhere), so there is no native::selectionon the editor text to override. That::selectionnormalization belongs to the focused selection, which is the separate WebKit drawSelection work in #8709, not here. Recoloring the blurred fake selection's text would mean touching the actual nodes, which this path deliberately leaves alone.So the honest scope:
colors.background(this PR) is the clean answer to the literal "make the color configurable" ask. Making the blurred fake selection both visible and legible over an opaque code background is a genuine open problem, the z-index lift buys visibility but the contrast can't be bought back the same way, and I'd rather hear how you'd want to approach it (lift behind an option, a documented limitation) than guess.Test plan
createDefaultOnReposition.test.ts: default color, custom color, offsets preserved.pnpm vitest --project unitgreen; existingmarkSelection.test.tsxunaffected.check-flow-typesall clean.Open questions
colorsthroughmarkSelection,selectionAlwaysOnDisplay, and the Extension config for parity.colors?(zero-churn back-compat, what this PR does) or an options bag (markSelection(editor, {onReposition, colors})) that soft-deprecates the positionalonReposition?createDefaultOnReposition, or keep it internal? It lets a custom-onRepositionconsumer reuse the default styling, at the cost of a slightly wider public surface.z-indexlift behind an option here, or better left to the focused-selection path ([lexical-utils] Add dedupeSelectionRects: fix duplicate/extra selection rects on WebKit (#7106, #7492) #8709) and documented as a limitation of the blurred fake selection? Open to either.