WEBDEV-8458 Drive the item navigator's panels from their own state - #70
Open
iisa wants to merge 1 commit into
Open
WEBDEV-8458 Drive the item navigator's panels from their own state#70iisa wants to merge 1 commit into
iisa wants to merge 1 commit into
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>
|
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 the change reviews on its own.
Why
@jbuckner asked whether the 350ms
setTimeoutguarding a scroll could be more deterministic. Looking into it, the timer was a symptom: the panel's open/close wasn't modelled as state, so the code worked around the animation instead of the behaviour that caused it.Two things were true underneath:
The state was in two places. The navigator held
menuOpened/openMenu; the slider held its ownselectedMenu, plusopenandanimateMenuOpen. BecausemenuTypeSelectedis composed, selecting a channel toggled both copies independently and the navigator then wrote its value back over the slider's — they agreed only because the two toggle expressions happened to be equivalent.slider.openwas set as a static attribute, so Lit wrote it once and it stayedtrueforever;animateMenuOpenwas never set by anything, so the slider's.animaterule never applied. Closing the drawer left the panel open underneath it.The semantics didn't describe any of it.
aria-expanded="false"was a hardcoded string on the toggle and every shortcut — announcing "collapsed" while open. The drawer was an unlabelleddiv. The closed panel was only translated off-canvas, so its close button stayed in the tab order and the accessibility tree while invisible. Both close buttons were named "Close this menu" but closed different things. Opening from the shortcut rail hid the focused button in the same frame and dropped focus to<body>.What changed
The navigator owns whether the drawer and a panel are open. The slider reports what the user did and renders what comes back, so there's one copy of the state instead of two.
slider.open,animateMenuOpenandmanuallyHandleCloseare gone.With the state in one place it can be stated in the markup:
aria-expandedreflects what's actually open, the drawer and panel carry roles and names, the panel is named by its own heading, and closed panels areinertso the tab order and the accessibility tree agree with the screen.Focus follows the same rule — opening a surface moves focus into it, closing returns focus to whatever opened it. Closing the drawer now also closes the panel inside it.
The scroll follows it too: it runs when the file list or the selection changes. That also fixes two gaps the timer was hiding — bookreader reuses one panel element, so
firstUpdatedonly ever fired on the first open, and offshoot reuses the element on a file click, so the active row could move without the panel following it.Menu buttons now carry an
aria-label, so their accessible name doesn't depend on--item-navigator-menu-button-label-displaybeing set. Worth knowing for the migration: both consumers currently set the upstream name for that variable (--menuButtonLabelDisplay), which our port renamed — so the labels would otherwise have quietly disappeared. Tracked in WEBDEV-8856/8857.Verifying
The useful check is running it with the animation switched off — if the behaviour is identical at
--item-navigator-animation-timing: 0ms, nothing is timing against it. It is:aria-expandedfalse → true, panel still inert, focus on the first menu button<body>The closed panel's close button is not focusable. The rail is a real
ul/liwithrole="list". No console errors.279 tests pass with 100% coverage on the component; build and lint clean. Tests that pinned the removed state were rewritten rather than dropped — the slider's tests now assert that it reports selections instead of deciding them, and the two scroll tests that slept 400ms each and only asserted "didn't throw" now mount the panel in a real scrolling box and check the active row is actually in view.
Worth a second opinion
role="group"rather thandialog, so it doesn't claim to be something it isn't — happy to make it genuinely modal instead if you'd prefer.aria-controlsonly where it resolves. IDREFs can't cross shadow roots, so it's on the toggle (same root as#menu) but not on the shortcuts or menu buttons, which live in different roots from the panel. Better than shipping references that point at nothing.manageSideMenuEvents('toggle'). That's an explicit instruction rather than a user action, so I left it — and its test still passes.