Skip to content

fix(web): keep mobile sidebar keyboard focus in the visible drawer - #534

Merged
tt-a1i merged 4 commits into
openpi-dev:mainfrom
ooiuuii:fix/web-accessibility-350
Sep 12, 2026
Merged

tt-a1i merged 4 commits into
openpi-dev:mainfrom
ooiuuii:fix/web-accessibility-350

Conversation

@ooiuuii

@ooiuuii ooiuuii commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

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

  • Synchronize narrow-sidebar visibility with keyboard reachability, including previously collapsed desktop state.
  • Use mobile dialog semantics and an inert main pane; focus the close button on entry, contain forward/reverse Tab, and restore the trigger on dismissal.
  • Keep focus within the drawer after clicking its brand/padding, and return focus to the search toggle when its close control disappears.
  • After a render removes the focused row, recover BODY focus to a surviving drawer control, only while mobile-open and with no native modal active. Archive/restore share this single fallback; their runtime behavior is unchanged.
  • Preserve nested menu/native-dialog ownership and clear mobile isolation when returning to desktop.
  • Improve timestamp contrast using the existing row text color. No new palette, theme setting, focus framework, or dependency.
  • Add four production-browser regressions and regenerate the required web/dist artifacts.

Validation

Windows; Node 24.19.0; Bun 1.3.14; locked Pi 0.85.1; Chrome 152.0.7977.76. Candidate: fed827091b734ddf868e9518196447e1020c2ef3, based on 5bf2fe29e52801d79826c2eb573be403f53285e5. Runtime proof was captured for c4201e2c50a2260d240bddda222aa0dee2811629; 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 web production WebHost and committed-style web/dist, not Vite or intercepted API responses. pi list reported 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.

  1. Launch the production CLI with an isolated Pi agent directory; verify git HEAD and the unique pi list source.
  2. At a 390px viewport, press Tab from the document start, open the sidebar, cycle Tab/Shift+Tab, open a workspace menu and rename dialog, then dismiss each layer with Escape.
  3. Repeat at 320px, with light/dark colors, close-button/scrim dismissal, and mobile-to-desktop resizing.
  4. Seed a clearly marked native-format header-only Session fixture in the isolated agent directory. Through the real UI, archive then restore a non-current Session; wait for its row to disappear and verify focus, Tab and Escape recovery. No fake assistant response is added.
Observation Before After
Initial Tab order while closed First five stops enter the off-screen sidebar; close button x=-45/right=-15 Focus starts on the visible hamburger; closed controls are unreachable
Open drawer Focus stays on the background hamburger; Tab enters obscured main content Focus enters the close button; forward/reverse Tab stays in the drawer
Escape Drawer remains open Appropriate inner layer closes first; drawer dismissal restores the hamburger, including after padding clicks and closing search
Archive/restore removes the focused row Focus can fall to BODY and bypass descendant key handling Focus returns to a surviving drawer control; Tab and Escape remain usable
Narrow light/dark accessibility Existing timestamp contrast failed the new focused scan Both scoped Axe scans have zero violations

Automated checks and limits

bun run check
bun x --no-install vitest run tests/web/app-render.spec.ts
bun x --no-install playwright test --config tests/web/playwright.config.ts --grep 'mobile sidebar'
bun x --no-install playwright test --config tests/web/playwright.config.ts --grep 'mobile sidebar contains keyboard focus' --repeat-each=10 --retries=0
bun run test:web:e2e
bun run test
  • bun run check: passed, including regenerated production assets, format, lint and typecheck.
  • Existing component tests: 15/15 passed. New browser regressions: 4/4 passed, including nested menu Tab/Escape, rename-dialog focus/viewport bounds, visible outlines, collapsed-desktop transitions, and focus fallback after noninteractive clicks, closing search, and archive/restore row removal. The fallback scenarios also have real production RED/GREEN receipts.
  • CI follow-up 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-item toBeFocused() 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 and bun run check passed again. Original CI failure/trace.
  • The subsequent official Web E2E job on fed8270 passed 15/15 in 36.6s, including the formerly failing 390px case. This supersedes the original browser CI failure.
  • On the same head, Node 24 CI passed check, generated-dist verification, all 1,525 Node tests (one skipped), 130 Vitest tests and the first two package smoke checks. Its later packed Web CLI/locked-host install failed with npm ETARGET for effect@^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.
  • Earlier full browser run: 13/14 passed, not claimed all green. The unchanged bug(web): 空会话切换工作区仅改显示,发送仍命中原仓库 #467 workspace-selection fixture uses 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 final check, component tests, focused browser run and real-product receipts cover the final candidate.
  • Official 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.
  • No physical mobile device or screen-reader validation, provider smoke, or complete feat(web): add persistent theme/language preferences and accessibility/mobile hardening #350 audit is claimed. Official GitHub CI is a separate gate.

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

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tt-a1i
tt-a1i merged commit 7b797ab into openpi-dev:main Sep 12, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants