fix(studio): colour hex editing and shortcuts panel dismissal

[miguel-heygen]

What

Two Studio panels that would not take or keep user input correctly.

Bugs fixed

1. The colour hex field snaps back while you type. updateColorDraft stamped a canonical hex back over every keystroke, so pressing backspace immediately restored the character.
2. A non-repeating hex value was silently replaced with the wrong colour. Same root cause: the canonical stamp won over what the user actually typed.
3. A typed hex never committed on outside click. The commit hung off a private onBlur that never fires, because the panel unmounts on outside click before blur is delivered.
4. The shortcuts panel could not be dismissed with Escape.
5. The shortcuts panel would not dismiss on outside click during a canvas gesture. Its hand-rolled bubble-phase mousedown listener never ran, because a gesture such as a marquee start calls preventDefault on pointerdown, which suppresses the mousedown compat event entirely.
6. A 3-digit hex shorthand such as #F00 was silently dropped. Found in review of this PR's own fix. parseCssColor accepts both 3 and 6 digit hex, but the new gesture resolver gated on 6, so shorthand neither previewed nor committed. The old onBlur parsed it, so this was a behavioural loss rather than a pre-existing gap.
7. aria-modal on a non-modal popup. The panel does not trap focus and leaves the rest of the editor operable, so aria-modal told assistive tech the whole app was inert while it was open.

Why

Bugs 1 to 3 mean the colour field cannot be used by typing at all, which is the only way to enter an exact brand colour. Bug 5 leaves a panel stuck open over the canvas mid-gesture.

How

Split hex-draft ownership so the hex input is the sole author of its own text while editing, and route hex typing through the shared gesture transaction so outside click and Escape settle or cancel it like every other inspector field.

Replace the shortcuts panel's hand-rolled listener with the shared useContextMenuDismiss hook, which already handles Escape and the pointerdown preventDefault case. Wire role=dialog plus aria-controls between trigger and panel, which is the correct non-modal disclosure shape.

Test plan

• Unit tests added/updated
• Manual testing performed
• Documentation updated (if applicable)

Studio suite green on this branch: 2997 passing, 0 failures. Typecheck, lint and format clean.

Not covered

• Second of a 7-PR stack. Stacked on the ruler seek and pointer target PR below it.
• The colour fix covers the hex text field. The picker surface and the swatch grid are unchanged.
• The resolver's accepted hex lengths are now exactly the ones parseCssColor accepts, 3 and 6 digits. 4 and 8 digit alpha hex is out of scope because parseCssColor does not accept it either, and alpha is owned by the panel's own slider.

[miguel-heygen]

• fix(studio): author every new tween against one element #2849
• fix(studio): announce real track numbers and wire lane aria-controls #2848
• fix(studio): target one element when adding a keyframe at the playhead #2847
• fix(studio): lane every tween and attribute tweens to their real target #2846
• fix(studio): sub-composition clip timing and expanded rows #2845
• fix(studio): colour hex editing and shortcuts panel dismissal #2844 👈 (View in Graphite)
• fix(studio): timeline ruler seek, retime rounding, and pointer target sizes #2843
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[james-russo-rames-d-jusso]

Reviewed at 7be28e4.

Both fixes are tightly scoped and the abstractions are the right ones. The hex-draft split (updateColorDraft(value, source) with the hex branch skipping the canonical stamp-back, plus the new resolveColorGestureValue guarding gesture emits behind ^#[0-9a-f]{6}$) is a clean way to keep the hex input as the sole author of its own text while editing — the raw hex string flows through the gesture transaction as latest, the picker path still gets its RGB round-trip, and alpha is preserved from draftColorRef.current.alpha on the hex path so partial hex edits don't drop alpha.

Swapping the hand-rolled ShortcutsPanel dismiss for useContextMenuDismiss is the right consolidation — it joins the three existing consumers (CanvasContextMenu, KeyframeDiamondContextMenu, TrackGapContextMenu) so the panel now inherits the same capture-phase pointerdown + preventDefault-on-canvas-gesture handling that the rest of the editor already has. role="dialog" without aria-modal, with an inline comment explaining why (non-modal disclosure, focus not trapped), matches WAI-ARIA authoring practices exactly. The 6 new ShortcutsPanel tests cover all three focus positions on Escape + the preventDefault-on-pointerdown edge case + no-re-render-on-idle-Escape — thorough.

Concerns

• The "incomplete pending hex on outside-click" test only asserts onCommit isn't called — not that the input value reverts. propertyPanelColor.test.tsx:129 covers the commit no-op but not the visible-state restoration. The restoration path does work in the code (settle() at useInspectorGestureTransaction.ts:35 calls previewRef.current(active.before) before the commit; that runs resolveColorGestureValue(before) → picker source → updateColorDraft(before, "picker") → setHexDraft(toHexColor(...).toUpperCase()), so hexDraft reverts). But the test doesn't assert it. This mirrors the Escape test at :146 which explicitly re-opens the panel and asserts openHexInput(host).value === "#224466". Same assertion would harden the outside-click case against a future silent regression where the input keeps #12AB3 on-screen while the color model reverts. Trivial to add.

Nits

• The #22CC66 → #2222CC regression case at propertyPanelColor.test.tsx:112 is exactly the shape of production defect that specification-only tests miss. Nice catch to include it.
• useContextMenuDismiss.ts docblock is load-bearing — it's the kind of comment that saves future readers from wondering why we listen on capture-phase pointerdown AND mousedown. Keep it.

What I didn't verify

• Manual testing of the two shortcuts-panel-during-canvas-gesture failure modes (marquee-start with panel open, then outside click that gets preventDefault'd), and the hex-field alpha preservation via the picker's alpha slider (propertyPanelColor.tsx:190-196 looks correct, but I didn't repro).
• Sibling PR #2843 interactions — reviewed the two as a stacked pair.

— Review by Rames D Jusso

[vanceingalls]

Reviewed at 7be28e4.

Six real bugs, six tight fixes. The hex-draft split, the useContextMenuDismiss swap, and the role="dialog" without aria-modal are all the right shape. Alpha preservation through draftColorRef.current.alpha on the hex path (propertyPanelColor.tsx:203), the source-normalized sourceValue: formatCssColor(colorFromCss(value)) at :220 so Object.is comparisons in the gesture transaction don't spuriously fire from string-form drift, and the capture-phase pointerdown listener that survives the canvas's preventDefault — those choices are careful. I read Rames's review before writing this and won't repeat it; my focus is the one class of user input that the new resolver silently drops.

Concerns

• P1 — 3-digit hex shorthand (#F00, #000, #FFF) is silently rejected. resolveColorGestureValue at propertyPanelColor.tsx:196 uses ^#[0-9a-f]{6}$ — six hex digits only. But parseCssColor at colorValue.ts:36-45 explicitly accepts the 3-digit shorthand (shortHex branch). So typing #F00 walks this path:

  1. handleHexChange("#F00") → setHexDraft("#F00"), previewColorGesture("#F00").
  2. Resolver: source=hex, regex fails on 3 chars → returns null → outer onPreview no-ops. Swatch stays black.
  3. Outside-click → settleColorGesture. Latest="#F00", before="rgb(0, 0, 0)". Rollback preview restores hexDraft to "#000000" (via the source=picker branch). onCommit runs with "#F00" → resolver rejects → no commit.
  4. Net: user typed a valid CSS shorthand, saw it appear in the input, clicked outside expecting red, got nothing. No error surface.

  Same outcome on Tab-blur, which is a behavioural loss vs the OLD onBlur path (which called parseCssColor(normalized) directly and would have committed the 3-digit form). The existing "backspace to #3" test at propertyPanelColor.test.tsx:104-110 iterates through "#33333" → "#3" and asserts the input keeps each intermediate string — which is what fixes the snap-back, good — but the 3-digit form here is only ever an intermediate on the way to fewer chars, never a terminal commit state. There's no test that asserts #F00 (or #333 as a completed shorthand) commits as rgb(255, 0, 0) (or rgb(51, 51, 51)).

  Fix (single line): broaden the gate to accept both hex forms parsed by parseCssColor:

  if (source === "hex" && !/^#([0-9a-f]{3}|[0-9a-f]{6})$/i.test(nextValue)) return null;

  Then add a paired test covering the shorthand commit-on-blur path — the 3-digit case is genuinely common designer input (#000, #FFF, primaries), and CSS supports it.

Nits

• useContextMenuDismiss attaches its Escape listener to document unconditionally in useContextMenuDismiss.ts:777, so every Escape press throughout the editor invokes closeShortcuts = setShowShortcuts(false). React's Object.is short-circuits the re-render (the Profiler-based "does not re-render when Escape is pressed while closed" test at ShortcutsPanel.test.tsx:145-158 proves this — nice test), so it's functionally fine, but the old effect gated on showShortcuts and avoided the callback entirely. Minor: not blocking, worth a comment if you want future readers to know the deduplication is load-bearing.
• aria-controls={shortcutsPanelId} at ShortcutsPanel.tsx:554 is always set on the trigger, but the referenced element only exists in the DOM when showShortcuts=true. Screen readers handle a dangling reference gracefully in practice; a stricter reading of the ARIA spec would toggle the attribute conditionally. Not worth blocking on.
• The role="dialog" without aria-modal on a non-modal disclosure is a defensible-but-debatable ARIA choice. The inline comment at ShortcutsPanel.tsx:575-578 documents the intent well enough that any future reader who wants to argue for role="menu" or no role at all has the receipts. Good.

What I didn't verify

• Manual repro of the #F00 shorthand path in a running studio — traced through code, but a live check would confirm no other path picks up the shorthand later (e.g. the picker's own opening event doesn't retroactively re-parse hexDraft and commit).
• Alpha preservation on the hex path when the current alpha is < 1 and the user types a fresh 6-digit hex — read as correct at propertyPanelColor.tsx:200-204, not exercised at runtime.

Verdict

COMMENTED — B+. The three shipped fixes are correctly designed and the test suite covers the key regressions; the 3-digit hex gate is one line and worth landing before merge. Not a blocker to the enterprise/holdout gate — feature is opt-in on studio side and the shorthand loss is a UX degrade, not a crash.

— Review by Via

[james-russo-rames-d-jusso]

Re-reviewed at 86f633f (R1 was 7be28e4).

Delta

One new commit new to this PR: fix(studio): accept 3-digit hex shorthand in the colour field. (The R2 diff also carries #2843's roomForFullTarget commit downstack — noted, reviewed on #2843 already.)

R1 → R2 verdict

• R1 outside-click restoration test gap → RESOLVED. propertyPanelColor.test.tsx:151 "does not commit an incomplete pending hex on outside-click" now asserts openHexInput(host).value === "#224466" after the outside click — mirrors the Escape test's shape and closes the assertion gap. Also added a targeted commits a 3-digit hex shorthand on outside-click test at :147 covering the new #F00 path. Trace verified: settle() → previewRef.current(active.before) reverts hexDraft via the picker-source updateColorDraft, commitRef.current(active.latest) no-ops because the resolver returns null on #12AB3.

New — 3-digit hex shorthand acceptance

Well-caught side-bug. parseCssColor at colorValue.ts:39-48 already accepts shorthand (#F00 → RGB via parseInt(r+r, 16) per channel), so the old ^#[0-9a-f]{6}$ gate in resolveColorGestureValue was silently dropping ordinary designer input while the pre-refactor onBlur used to accept it. The regex update to ^#([0-9a-f]{3}|[0-9a-f]{6})$ is minimal, and the "live-previews only a hex length that parses, 3 or 6 digits" test now covers all four states (3, 4-and-5-suppressed, 6, revert-to-3).

Mid-typing states like #F0 / #F000 correctly stay silent through the resolver. Alpha preservation via draftColorRef.current.alpha is unchanged, so a shorthand-typed value keeps whatever alpha the picker held.

Clean second round.

— Review by Rames D Jusso

[vanceingalls]

R2 — verified at 86f633f9

Delta since R1 (aa281164 → 86f633f9)

Four commits, one of which is the direct R1 response:

• 7f0cadcb — drops aria-modal from the shortcuts popup (correct for a non-modal disclosure).
• 86f633f9 — accepts 3-digit hex shorthand in the gesture resolver.

propertyPanelColor.test.tsx grew from a two-case file to a full hex-editing suite (10 cases), and ShortcutsPanel.test.tsx is a new 158-line file.

R1 findings — verified

• P1: 3-digit hex silently dropped on outside-click / Tab-blur → RESOLVED. resolveColorGestureValue at packages/studio/src/components/editor/propertyPanelColor.tsx:192 now gates on /^#([0-9a-f]{3}|[0-9a-f]{6})$/i, which is exactly the accept-set of parseCssColor in colorValue.ts:41-59 (shortHex + hex, no 4-/8-digit alpha branch). Direct tests: "commits a 3-digit hex shorthand on outside-click" (#F00 → rgb(255, 0, 0)) and "live-previews only a hex length that parses, 3 or 6 digits" (asserts 4/5 digits stay silent, 3 and 6 preview once). The commit message explicitly cites the pre-existing onBlur path as evidence this was a behavioural loss rather than a pre-existing gap — that framing matches what I flagged.

• Nit: useContextMenuDismiss Escape fires unconditionally; React bail-out is load-bearing → ADDRESSED via test. ShortcutsPanel.test.tsx: "does not re-render when Escape is pressed while closed" uses <Profiler> to assert onRender is called exactly once after a closed-state Escape. The bail-out is now guarded against a future refactor that would break identity of setShowShortcuts(false).

• Nit: aria-controls={shortcutsPanelId} on the trigger references an id that only exists when showShortcuts=true → UNCHANGED. Still the case at ShortcutsPanel.tsx:152. This is a widely tolerated ARIA pattern (aria-controls may point to a target that appears when the control is invoked) and the new "connects the trigger to the dialog with aria-controls" test only asserts the open state. Not blocking — flagging so it stays visible.

Rames' R1 ask for a stronger outside-click restoration assertion is also satisfied: "does not commit an incomplete pending hex on outside-click" reopens the input and asserts input.value === "#224466" (the original), not merely onCommit not called.

Fresh adversarial pass at head

• parseCssColor at head accepts only transparent, 3-digit hex, 6-digit hex, and rgb()/rgba(). The new regex covers both hex forms. No 4-/8-digit alpha branch to leak.
• sourceValue: formatCssColor(colorFromCss(value)) re-normalizes on every render; assumes the transaction hook snapshots at gesture-start rather than tracking identity every render. Behaviour tests pass, so the assumption holds; noting the coupling for future changes to useInspectorGestureTransaction.
• useContextMenuDismiss listeners are registered document-wide for the lifetime of the mounted ShortcutsPanel (open or closed) — capture-phase pointerdown + mousedown + keydown. All three fire setShowShortcuts(false) unconditionally on outside events; correctness rides on React's Object.is bail-out. Cheap in practice, and now covered by the Profiler re-render test above.
• The Escape branch in useContextMenuDismiss does not preventDefault / stopPropagation, so pressing Escape while another surface owns the intent (e.g. text-field edit) still lets that surface see the event. Good.

No P0/P1 at head.

Verdict

APPROVED, grade A. R1 P1 is fixed at the parser boundary rather than papered over, the fix ships with directly-named regression tests including the 4-/5-digit silence guard, and both nits are either mechanically addressed or now protected by explicit assertions.

— Review by Via

The base branch was changed.