feat(studio): panel tabs take the decided Current design - #4266
Conversation
Dockview's tab strip uses one pill treatment, with a reserved close target and colours only from theme tokens. A paper host overrides those same token names.
fb48def to
fc6a9ff
Compare
oxfmt wraps the long dock selectors. The tab treatment is unchanged.
Replaces the pill, underline and close treatment with the Current design: panels are cards on the shell with one border each, the shown tab is a fill in the card colour that slides to the next tab, only the shown tab carries its type icon and close glyph, strip actions sit on the active group only, and narrow strips scroll with an edge fade.
miguel-heygen
changed the title
Also points the hovered-tab contrast pair at the lines that declare it.
Tighter tab padding and gaps, same font. The load smoke now fails when a tab strip clips a label at 1440x900.
CI renders the system font stack with a narrower Linux fallback, so a strip that just fits in CI could still clip on a Mac.
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
Reviewed at 8ed7ab69 for code correctness — UI verification is yours per the PR body.
Scope. 16 files, all under packages/studio/src/components/dock/ + nle/, theme.css, Menu.tsx (adds MenuCheckboxItem), and two script files (contrast-pairs.json, studio-runtime-smoke.mjs). No reach outside Studio components.
installTabFill (dockTabFill.ts, the new imperative DOM lane). Read/write split via the measureFill closure is correct — every strip is measured first, then all writes fire, so layout runs once per placeAll. fillOf uses :scope > .${TAB_FILL_CLASS} and list.prepend on miss, so a second fill can't accumulate. data-ready set on the next rAF gates the transition, so the first placement doesn't slide from (0, 0px) — matches the "placed once without motion; only later moves slide" comment. Disposer removes the capturing scroll listener, disposes all six dockview subscriptions (onDidActivePanelChange, onDidAddPanel, onDidRemovePanel, onDidMovePanel, onDidLayoutChange, onDidLayoutFromJSON), and disconnects the ResizeObserver. observe() prunes disconnected elements before re-adding, so the observed set doesn't leak across layout changes. Root wiring in Dock.tsx:157/193 pairs installTabFill with its disposer alongside installDockAccessibility.
Fill hides when there's no active tab. measureFill sets fill.hidden = !box, and .dv-tabs-container[data-clipped] mask + fade tokens are stable across the null case — no flash of stale translateX because the write path early-returns.
DockTab / DockStripActions. Both use useSyncExternalStore(subscribe, snapshot) correctly (title change / maximize change respectively). TAB_ICONS: Record<PanelId, Icon> gives compile-time coverage of the panel-id enum, so a future panel forgotten here is a typecheck failure not a runtime null. Close-glyph on the tab is role="button" + tabIndex={-1} with a pointerDown stopPropagation — matches dockview's stock (a tab cannot hold a focusable button), and the PR body's MINOR note documents the keyboard path (Tab → Panel menu → uncheck). Non-blocking, and consistent with what shipped before.
CSS surface. Every rule scoped under .hf-dock, no bleed. Mask on [data-clipped] only — matches the comment "A mask adds a compositor render pass; only a strip that is actually clipped needs one" and the timeline-viewport-gate table in the body (b20ff32 → 4130277 recovered p95 20ms). No literal colours — the contract test proves it (regex #[0-9a-f]{3,8}\b|rgba?\(|hsla?\(|oklch\( matches zero).
dockTabs.contract.test.ts is executable docs on the CSS. It reads dock.css at test-time, parses top-level rules, and asserts (a) exactly one bordered selector (.hf-dock .dv-groupview), (b) only .hf-dock-tab-fill[data-ready] transitions and only .hf-dock-tab-icon animates, (c) the reduced-motion block turns both off, (d) .hf-dock-tab centres by height:100%+align-items:center, (e) no hex/rgb/hsl/oklch literals. This is exactly the drift-locking pattern that catches "someone adds a hover transition later" — the RED-then-GREEN table in the body ("add a hover transition → 1 red") proves each assertion is non-vacuous.
DockTabs.test.tsx shim. happy-dom has no layout so the test stubs offsetLeft/offsetWidth (100px step / 90px width) via Object.defineProperties(HTMLElement.prototype, …, {configurable: true}) and restores them in afterAll. Same-file beforeAll loads dock.css into document.head so getComputedStyle(...).display in the shown-tab-only assertion is what a user actually sees. Prototype restoration is guarded (if (descriptor)) so a missing original doesn't wipe the property. Fake timers cleaned per-test.
Runtime smoke. studio-runtime-smoke.mjs now sets page.setViewport({width: 1440, height: 900}) before the strip-clip check (default layout is only meaningful at the design width). The check computes room = actions.left - strip.left and errors when strip.scrollWidth + MAC_FONT_ALLOWANCE_PX > room, with the 10px allowance derived in the PR body (measured macOS − Linux width per strip; left strip is the widest gap at 10px, so 10 covers all four). Fires under Studio: load smoke in real Chromium — green here.
Contrast pairs. Three new pairs (dock-tab-label, dock-shown-tab-label, dock-hovered-tab-label), each with background as an array (the hover case correctly composites over both --panel-strip-bg and --panel-tab-bg-hover). The line-number source cites (dock.css:13/15/12/14/100/102) match the token declarations in the updated .hf-dock block; drift on those specific lines would surface via the ratchet.
Menu addition (MenuCheckboxItem). Small, matches the sibling MenuRadioItem shape (base component + tick indicator span). Exported from the ui index. Consumed by DockStripActions. Nothing else in the codebase to break.
CI at head. Test, Test (studio), Test (runtime), Test (support), Studio: load smoke, Studio: timeline viewport gate, Studio and player captures, Preflight (lint + format), Format, Lint, Typecheck, Build, Fallow audit, File size check, player-perf, preview-regression, regression, regression-shards (shard-1..9), Tests on windows-latest{, : studio-core, : studio-engine-cli}, Render on windows-latest, Graphite / mergeability_check, CodeQL, Analyze (*), CLI smoke (required), Semantic PR title, Preview parity, Producer: unit tests, Producer: integration tests, SDK: unit + contract + smoke — all success at head. The ${{ matrix.lane }} and regression-shards template names showing "cancelled" are pre-expansion status artifacts; the individual matrix leaves all pass, and statusCheckRollup is SUCCESS.
PR body hygiene. Separate ## Before / ## After with dark + paper captures, plus a switching-tabs video and the "every label whole" strip captures with numbers (278/282px macOS, 268/282px Linux). The frame-cost regression table in ## Timeline viewport gate documents the b20ff32 → 4130277 recovery. The ## Tests table pairs each rule with its test and the RED break used to prove it. The ## Independent review note walks the SHA history with the one MAJOR fixed and the two accepted MINORs.
Approving on code correctness. Miguel owns the UI acceptance per the PR body's "For Miguel" note.
— tai
…ions The action slot was reserved at 72px on every strip, so inactive strips clipped tabs next to empty space. The load smoke now fails on a reserved empty slot and checks each strip while its group is active.
Focusing a panel draws its strip actions, which could hide the shown tab of a strip that only just fit. The load smoke checks move into their own function to keep the smoke runner simple.
A strip the user scrolled by hand no longer snaps back when another panel changes. The load smoke measures the focused group's strip directly.
Covers a strip back from hidden (another panel maximised) and a shown tab moved by a closed or moved neighbour; the user's own scroll still stays put.
A hidden strip keeps its scroll, so a strip is compared with its last visible shape and a strip that comes back unchanged is left where the user put it.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at 5fd6c7bd. I reviewed the five commits since Terence's approval at 8ed7ab69 (an ancestor of this head): dockTabFill.ts, dock.css, DockTabs.test.tsx and scripts/studio-runtime-smoke.mjs.
What changed, checked against the code
- Actions no longer reserve width.
.dv-right-actions-containerdrops its fixed 72px, andDockStripActionsreturnsnullfor an inactive group, so only the active strip gives up room. The trade-off is that a strip narrows when its group takes focus. The reveal logic handles that case: the strip is observed by theResizeObserver, so the narrowing reachesplaceAll. - When the strip scrolls to the shown tab.
reshaped()returns true only when the strip got narrower, or when the shown tab changed, moved or resized. A hand scroll changes none of those, so it is left alone. A hidden strip (clientWidth === 0) is neither compared nor stored, so it is compared with its last visible shape when it comes back. That matches the maximise and restore case in the tests. Reads still all happen before writes; thescrollLeftwrite sits inside the deferred write closure. - Smoke check. It now clicks each strip's shown tab so every group gets measured while active. It also fails if an inactive strip still holds width for actions, which pins the CSS change.
Evidence. After a clean bun install in a worktree at this head, vitest run src/components/dock passes: 6 files, 69 tests. I ran two mutations on reshaped():
- Never reveal on narrowing: 2 tests fail ("keeps the shown tab whole when its strip narrows", "reveals the shown tab when its strip comes back narrower").
- Always reveal: 3 tests fail (the three hand-scroll tests).
Both halves of the predicate are covered.
Nit, not blocking. In revealedScroll, if the shown tab is wider than the strip, left + size + 1 - width is greater than left. The strip then scrolls to the tab's right edge and cuts off the start of its label. Math.min(left, …) on that branch would keep the start in view. With labels kept whole this only happens on a very narrow strip.
— Rames
3 participants
Studio's panels become cards on a dark shell, and each panel's tabs take the "Current" design: the open tab is a fill in the card's colour that slides to the tab you pick, only the open tab shows its icon and close glyph, and a strip that is too narrow scrolls with a soft edge instead of hiding tabs behind a menu. This replaces the pill, underline and close treatment that was on this PR before.
Before
After
Switching tabs (the fill slides in about 140 ms and the new icon fades in over 80 ms; the panel body switches at once):
switch-after-1440.mp4
Every label whole, and the tabs use the whole strip (2x)
An earlier head clipped the left strip's fourth tab next to empty strip space. That space was a 72 px slot (plus 4 px) held on every strip for the strip actions, even on strips whose panel is not focused and so draws no actions. Dockview sizes that slot by its content; this PR had pinned it. Now the slot takes room only where the actions are drawn, so an unfocused strip gives its tabs the whole width. Tab padding is 6 px with no gap between tabs, so the tabs also fit next to the actions. Labels, icons and close glyphs sit on the tab's centre in every strip.
Left strip at 1440 x 900, tabs width against the room before the action slot:
At 1280 x 800 the left column is narrower, and the pinned slot clipped Catalog next to the empty space. Room for the tabs there went from 241 px to 317 px:
Captured at the PR head (5fd6c7b) from a fixture project (the
warm-grainregistry example) in headless Chrome on Linux, 2x, each state loaded fresh so Studio builds its default layout for that window. Before ismain; the strip Befores use the earlier heads'dock.css(8079c63 at 1440, 8ed7ab6 at 1280) on the same build. The macOS column was measured live in Chrome on macOS. The paper theme only re-themes the timeline today, so the panels look the same in both themes.What changed
--panel-shell-bg,--panel-card-bg,--panel-card-border,--panel-strip-bg,--panel-tab-bg-hover, with contrast pairs registered.Timeline viewport gate
The first head of this rebuild (b20ff32) cost the timeline gate one frame on CI: interaction p95 33 ms on every run (first attempt 100 ms on three runs), against main's 20 ms. Main-thread traces were identical; the extra cost was compositing, from a fade mask on every tab strip. Masking only a strip that is actually clipped brought it back:
From about 14:30Z
mainitself reads 33 ms on every run (three consecutivemainCI runs), and CI tests this PR merged with the currentmain, so the 6a5edd8 row matchesmain's level at that time.The af4f2b9 first attempt had three runs at main's level and two single-run spikes; the same commit's rerun passed 5/5, and the change is tab spacing only.
Tests
Each rule that could regress without anyone noticing has a test, and each test was proved by breaking the code once:
DockTabs.test.tsx(dock.css applied, computed display)DockTabs.test.tsxDockTabs.test.tsxdockTabs.contract.test.tsdockTabs.contract.test.tsdockTabs.contract.test.tsdockTabs.contract.test.tsscripts/studio-runtime-smoke.mjs(CI "Studio: load smoke", real Chrome)scripts/studio-runtime-smoke.mjsDockTabs.test.tsxDockTabs.test.tsxRun alone, each exits 0:
DockTabs.test.tsx5,dockTabs.contract.test.ts5,Dock.test.tsx14,dockAccessibility.test.ts10,PreviewPane.test.tsx1,Menu.test.tsx10,scripts/contrastRatchet.test.tspassing.Independent review
An adversarial review (separate agent, mandate to refute) at 4130277, eefaa83, 8079c63, af4f2b9 and 8ed7ab6: no blockers; one major, fixed.
MAJOR (af4f2b9), fixed in 8ed7ab6: CI renders Studio's system font stack with a narrower Linux fallback, so the clip check could pass while a Mac clips. The check now keeps 10 px of room for the macOS font, and a spacing that fits on Linux but clips on macOS turns it red. The 8ed7ab6 review asked whether 10 px holds for every strip, not just the left one it was measured on. Measured macOS minus Linux tab width at 1440 x 900: left 10 px, right 6 px, Preview and Timeline 2 px each, so the left strip is the widest gap and 10 px covers all four.
MINOR, left as is: the room check has no unit test of its own; it runs only in a real browser, and the smoke is its test (red on both broken spacings above).
MINOR: a panel cannot be closed from the focused tab itself by keyboard. The keyboard path is focused tab, Tab to "Panel menu", Enter, uncheck the panel (checked live: closes that one panel; checking it again reopens it). The tab's close glyph is not focusable, the same as dockview's stock tab on main, because a tab cannot hold a focusable button.
MINOR: the open tab is 16 px wider than on the design page (the close glyph's space is kept; see For Miguel).
Resolved: a contrast pair cited stale line numbers.
UX checklist
Job: someone editing a video moves between panels (Compositions, Assets, Code, Catalog; Design, Layers, Renders, Variables) many times an hour and needs to see which one is open and reach the next without hunting.
One action: click the tab of the panel you want.
Dock.Panel(Dock.tsxsetTitle)Choice: four tabs per side, one click each. The end: the panel body appears at once and the fill settles under the tab you picked.
Not exercised: a screen reader pass; touch input.
Traded off: hover-only strip actions (rows 2 and 12) and scroll-plus-fade instead of an overflow menu when a strip is narrowed are Miguel's decided design.
For Miguel