fix(web): keep mobile sidebar keyboard focus in the visible drawer - #534
Merged
Merged
Conversation
tt-a1i
approved these changes
Sep 12, 2026
tt-a1i
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed current head 439ff4e after updating to the Effect cohort fix. Standards: no actionable findings. Spec: the narrow-screen drawer now owns focus, traps keyboard traversal, restores focus to the trigger, and makes the background inert; native dialogs retain their own focus ownership. Regression coverage exercises keyboard and responsive boundaries. No P0/P1 found. Merge remains conditional on all required current-head CI.
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
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.
Problem
Refs #350; this is the mobile sidebar keyboard/focus slice, not completion of the accessibility/mobile roadmap.
On
main@5bf2fe29e52801d79826c2eb573be403f53285e5, the closed narrow sidebar is translated off-screen but remains in the Tab order. Opening it does not transfer focus, keyboard navigation continues behind the scrim, and Escape does not dismiss it.Value
Keyboard users should be able to open, operate, and leave the same drawer they can see, without navigating invisible controls or the obscured main pane.
Approach
web/distartifacts.Validation
Windows; Node 24.19.0; Bun 1.3.14; locked Pi 0.85.1; Chrome 152.0.7977.76. Candidate:
fed827091b734ddf868e9518196447e1020c2ef3, based on5bf2fe29e52801d79826c2eb573be403f53285e5. Runtime proof was captured forc4201e2c50a2260d240bddda222aa0dee2811629; the follow-up changes only three test lines, and the production sources/generated asset hashes remain identical. No runtime proof is attributed to changed product code.Real behavior proof
Bounded RED/GREEN receipts, runtime provenance and final asset hashes. The README distinguishes unchanged-main baseline, intermediate review regressions and final-candidate proof. These are selected DOM/network observations, not raw Session or agent transcripts.
The comparison used the actual
bin/openpi.js webproduction WebHost and committed-styleweb/dist, not Vite or intercepted API responses.pi listreported only the tested checkout in an isolated Pi agent directory. Fresh automation browser contexts were used; no personal Sessions, model credentials, or paid calls. Navigation-only probes observed zero non-GET requests. Archive/restore is a separate scenario using explicitly synthetic, native-format header-only Session fixtures in that isolated environment and real UI/Host/API operations. These are not captured model conversations; only scoped test-data writes are expected.git HEADand the uniquepi listsource.Automated checks and limits
bun run check: passed, including regenerated production assets, format, lint and typecheck.fed8270: the first official Web E2E run passed 14/15, but the 390px test sent two Enter presses only 9.642ms apart without waiting for the menu's next-frame focus transfer. Its failure screenshot shows the closed menu and focus still on the trigger, consistent with the dependency's asynchronous focus path. Added the same menu-itemtoBeFocused()assertion already used on the first opening; retained keyboard activation and all dialog assertions, with no sleep, retry or timeout increase. Both widths repeated 10 times each: 20/20 passed with zero retries; then all four focused cases andbun run checkpassed again. Original CI failure/trace.fed8270passed 15/15 in 36.6s, including the formerly failing 390px case. This supersedes the original browser CI failure.ETARGETforeffect@^4.0.0-rc.114. This dependency-install gate remains unresolved; dependency/lock/CI files are unchanged and were not folded into this UI PR. Other workflow jobs remain separate gates, so no all-green or merge-ready claim is made.canonicalWorkspace.split('/')and waits for a full backslash path on Windows while the UI displays its basename. This separate test-portability issue was not folded into the sidebar fix. That full run predates the review-driven focus fixes; the finalcheck, component tests, focused browser run and real-product receipts cover the final candidate.bun run test: failed locally, 1,416 passed / 2 failed / 10 skipped. Failures are in unchanged shared/setup-config concurrent updater (Windows libuv file-watcher assertion) and workflows/narrator log-flood test (1s sandbox timeout). The runner stops before subsequent groups; those are not claimed passed here.Review closeout
Two scoped review findings about BODY focus after noninteractive clicks/search close and after archive/restore were reproduced and fixed in the same sidebar owner. A third review's sole finding alleged that resizing with a native dialog open would hide the modal. That premise was not reproduced: Chromium's top-layer dialog UA rule explicitly sets
visibility: visible, and the actual dialog remains visible and dismissible despite its sidebar ancestor being hidden. This finding is consciously rejected, not patched speculatively. The final helper exited nonzero for that finding; the closeout is no accepted in-scope blockers, not a clean-helper claim. There was one explicit same-owner scope rebaseline as necessary regression coverage grew; no new runtime subsystem or product policy was added.The later three-line test synchronization delta received its own exact-diff review: helper exit 0, no findings. No production code or review scope was expanded to address the CI test race.
Impact