WEBDEV-8458 Drive the item navigator's panels from their own state - #70
Merged
iisa merged 10 commits intoAug 27, 2026
Merged
Conversation
The panel's open/close was spread across two components, so the code worked around the animation rather than the behaviour that caused it — most visibly a 350ms timer standing in for 'the list is ready'. The navigator now owns whether the drawer and a panel are open, and the slider reports what the user did and renders what comes back. That retires a second copy of the selection that only agreed with the first by coincidence, plus two properties that never did anything: the slider's own open flag, frozen true by a static attribute, and animateMenuOpen, which nothing set. Because the state is now in one place it can be stated in the markup: aria-expanded reflects whether a surface is actually open rather than being hardcoded false, the drawer and panel carry roles and names, the panel is named by its own heading, and closed panels are inert so the tab order and the accessibility tree agree with the screen. Opening a surface moves focus into it and closing returns focus to whatever opened it — including from the shortcut rail, which used to hide the focused button and drop focus to the document. Closing the drawer also closes the panel inside it, so a stale panel can't reappear. The scroll follows the same rule: it runs when the list or the selection changes, so there is nothing to wait for. Menu buttons also carry an aria-label, keeping their name from depending on a styling variable a consumer might not set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Every glyph is square and all eight call sites set width and height together, so the separate width and height knobs only doubled the API. They also disagreed: the header icon defaulted to 2em in the slider and 18px in the sort button, and both render in the same panel header — the close icon came out 20px next to an 18px sort icon. One knob per icon group can only carry one default, so the two can no longer drift. The header settles on 2em, which the close buttons already used and which follows the em-against-10px sizing the rest of the component uses. That exposed why the sort button had been pinned to a pixel value: buttons don't inherit font-size, so it was sizing its glyph against the UA default rather than the component base. It now resets font the same way the close button beside it does. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Each component spelled out var(--item-navigator-base-font-size, 10px) where it needed the base, so the default lived in ten places. Every other knob in the component declares a private alias once and reads that, which is what keeps a default from drifting between files — the habit that was missing when the header icon knob ended up 2em in the slider and 18px in the sort button. Same public name, same 10px default, no visual change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Opening straight to a panel from a shortcut looked a beat slow. The panel is nested inside the drawer, so their transforms compose: the panel covered 640px in the time the drawer covered 320px, staying clipped for the first quarter of the animation and then arriving at double speed. Opening the same panel from a menu button was fine because the drawer is already in place and the panel travels its own 320px. Opening straight to a panel is one movement, so the panel now holds still and lets the drawer carry it in. CSS cannot tell the two apart on its own — both paths end that frame with the drawer and the panel open — so openShortcut marks the render where the drawer was actually closed, and the navigator hands the slider a transition value through an inherited custom property. Every other path clears the mark, rather than waiting on the drawer's transitionend: a zero-duration transition never fires one, so with animations turned off the flag would have stranded on and suppressed the panel's animation for good. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Every menu icon sat at z-index 2, above the panel at z-index 1, so opening or closing a panel showed its contents travelling behind the icon column — the inactive icons have no background of their own, so there was nothing to hide it. Only the open entry's icon needs to be above: it borrows the panel's background and rounds into it, and the two have to read as one shape. The rest now sit below the panel and are simply covered as it moves. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The panel's movement had no tests, so the recent fixes to it rested on manual checks. These pin the vocabulary: the panel slides on the edges between empty and open, switching between panels swaps contents without re-running the slide, and the host can suppress the slide so the drawer carries the panel in. Also covers the marker's lifecycle — set only when the drawer was actually closed, and cleared by selecting another channel, closing the panel or closing the drawer — and that only the open entry's icon lifts above the panel, which is what kept the panel from sliding visibly behind the others. Positions are read with animations disabled, since a computed transform mid-transition reports wherever the animation has reached rather than where it is headed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CustomTheaterInterface described a host that feeds menu providers and shortcuts to the navigator, but nothing has ever been that host. It had a single reference in the repo — its own declaration — and no implementer, importer or consumer in elements, offshoot, bookreader or petabox. It was already dead in iaux-item-navigator and came across verbatim when the component moved out of labs. Eight of its twelve members appear nowhere in the navigator; the four that do are properties the navigator declares on itself, sharing only a name. Its real seam with a theater is the header and main slots plus the shortcuts event, which stay deliberately untyped because the navigator is a shell. An interface nothing implements cannot be checked, so it drifts: this one carried a modal member until that turned out to be unused too. Also drops a binding in the slider tests that stopped being read when those assertions moved to the rendered box. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The demo drew every viewable file from a pile of cat items, which covered images, PDFs and a video but left the navigator's other theaters unshown. It now runs on Dolly Parton across six real, public items: two images, a video, an album, a scanned book and a magazine PDF. The album and the book are lending items, so they embed as samples and preview pages rather than the whole work — which is what a consumer hits in production, and worth having in the demo for that reason. Media types grow by two to carry audio and book; both take the standard embed path, so only PDFs still load their file directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…em-nav-open-close-semantics
The navigator is a shell that doesn't know what it is hosting, so it shouldn't decide what loading looks like either. It shipped an animated book-and-ring glyph and, in fullscreen, an 'Internet Archive' caption — branding and motion a consumer may already have its own version of, with no way to replace or suppress it. The loaded property stays and still gates the reader, so consumers keep the hook they use today; what changes is that the frame is simply empty until loaded rather than showing our spinner. A consumer that wants an indicator can slot its own, which is the same seam the theater already uses. This also retires the last inline SVG in the component — it stayed inline through the mask-image migration because a mask can't animate the spinning ring on its own. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #64, targeting that branch so it reviews on its own.
The problem
@jbuckner asked whether the 350ms
setTimeoutbefore scrolling the active file into view could be more deterministic. It can — but the timer was a symptom, not the cause.Nothing modelled the panel's open/close as state. The navigator held
menuOpened/openMenu; the slider held its ownselectedMenu, plusopenandanimateMenuOpen. BecausemenuTypeSelectedis composed, selecting a channel toggled both copies independently, then the navigator overwrote the slider's — they agreed only because the two toggle expressions happened to match. A static attribute setslider.openonce, so it stayedtrueforever. Nothing ever setanimateMenuOpen, so the slider's.animaterule never fired.With no single fact to react to, the scroll had to guess when layout would settle. Hence 350ms, picked to outlast a 200ms transition.
What changed
The navigator now owns whether the drawer and a panel are open. The slider reports what the user did and renders what comes back — one copy of the state instead of two.
slider.open,animateMenuOpenandmanuallyHandleCloseare gone.The rest follows from that:
firstUpdatedfired only on the first open; offshoot reuses it on a file click, so the active row could move without the panel following.inert, so the tab order and the accessibility tree match the screen, andaria-expandedreports what's actually open instead of a hardcodedfalse.<body>.aria-label, so their name no longer depends on a styling variable.The transitions themselves don't change. They just follow the state flip now instead of being something to schedule around.
Also here: two CSS custom-property commits
Auditing the knobs while in this code turned up the same class of problem — a value duplicated per file rather than declared once.
6078c78— one knob per icon size.--item-navigator-icon-width/-heightand--item-navigator-header-icon-width/-heightalways moved together, since every glyph is square. Each pair collapses to one knob, taking the public surface from 18 to 16. That also fixed a live defect: the header pair declared2emin the slider but18pxin the sort button, so the close and sort icons rendered 20×20 next to 18×18 in the same header. One knob can only carry one default.057362e— declare the base font size like every other knob. Each component spelled outvar(--item-navigator-base-font-size, 10px)where it needed the base, putting the default in ten places. It now uses the private-alias pattern the other knobs already follow. Same public name, same10px, no visual change.Verifying
The check that matters is running with the animation off. If behaviour holds at
--item-navigator-animation-timing: 0ms, nothing is timing against it. It holds.aria-expandedfalse → true, panel still inert, focus on the first menu button<body>The closed panel's close button takes no focus. No console errors.
For the knob commits: icons measure 20/20/20 in the header and 24 in the drawer, and setting
--item-navigator-base-font-size: 20pxdoubles all of them — so the aliases are wired, not merely declared.279 tests pass with 100% coverage on the component; build and lint clean. The two scroll tests that each slept 400ms and only asserted "didn't throw" now mount the panel in a real scrolling box and assert the active row lands in view. The slider's tests now assert it reports selections rather than deciding them.
Decisions
role="group", notdialog. It shouldn't claim to be modal when it isn't.aria-controlsonly on the toggle. ARIA IDREFs can't cross shadow roots, so the shortcuts and menu buttons can't point at the panel. A reference that resolves beats one that dangles.manageSideMenuEvents('toggle')— an explicit instruction rather than a user action, so I left it alone.