Skip to content

fix(frontend): make sidebar plain links and move focus on navigation - #1371

Merged
IAmJSD merged 9 commits into
LeanerCloud:mainfrom
IAmJSD:ui/issue-730
Oct 24, 2026
Merged

IAmJSD merged 9 commits into
LeanerCloud:mainfrom
IAmJSD:ui/issue-730

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator

The sidebar announced itself to screen readers as a tab list ("tab, 2 of 6") but none of the tab keys worked, the Inventory and Admin sub-tabs had the same gap, and after picking a page focus stayed on the sidebar link with nothing announcing the new page. This PR turns the sidebar into plain navigation links, moves focus to the new page on navigation, and gives the remaining in-page sub-tabs the keyboard model their role promises.

Closes #730

What changed

  • frontend/src/index.html: the sidebar <nav> drops role=tablist/aria-orientation; its links drop role=tab/aria-selected/aria-controls, and the current one gets aria-current="page". The six top-level page containers drop role=tabpanel/aria-labelledby, and each gets a visually hidden <h2 class="sr-only page-heading" tabindex="-1"> (Home, Opportunities, Plans, Purchases, Inventory & Coverage, Admin). The Inventory and Admin sub-tabs get a static roving tabindex.
  • frontend/src/navigation.ts: setActiveNav toggles aria-current. switchTab takes a new focusContent option that focuses the page heading; the sidebar click handler (app.ts) and the popstate router set it, programmatic switches (deep links etc.) don't. renderNoAccess keeps the heading.
  • frontend/src/app.ts: a link click in the mobile drawer only returns focus to the hamburger if focus is still inside the drawer, so it no longer pulls focus back from the new page heading.
  • New frontend/src/lib/tablist.ts: the APG keyboard model for the Inventory and Admin tab lists. Roving tabindex (kept in sync by switchSettingsSubTab and switchInventorySubSection), Left/Right with wrap-around, Home/End, hidden admin-only tabs skipped, manual activation with Enter/Space.
  • Tests: new __tests__/tablist.test.ts, __tests__/route-focus.test.ts and tests-e2e/tab-keyboard.spec.ts; drawer-focus cases in mobile-nav.test.ts; sidebar-anchors, app, recommendations.spec.ts and mobile-header-drawer.spec.ts updated to query role=link/role=navigation instead of tab/tablist.

Commits: ee49c2ab (fix), addedbbe (tests).

Why this approach

This follows the issue's suggested fix. The sidebar is page navigation with real hrefs (open-in-new-tab, see #462), so links with aria-current are the right semantics and arrow keys would be wrong there. The sub-tabs switch in-page panels, so they keep the tab pattern and get its keyboard model. Activation is manual rather than automatic because every sub-tab selection fetches data and pushes a history entry; automatic activation would fire a load and a history entry for each tab arrowed across. Focus goes to a hidden per-page heading because Home and Opportunities have no visible heading at the top (their first content renders dynamically), and focusing the first visible h2 would scroll halfway down Home. The hidden heading gives every page the same target and announces the page name.

Screenshots

Before is main (a14dc8f2), after is this branch. Both are headless Chromium at 1440x900 against the repo's mocked e2e API. Focus on a hidden heading can't be seen, so a black caption painted by the screenshot script shows document.activeElement and the relevant ARIA attributes.

Before After
sidebar-enter-focus before sidebar-enter-focus after
Focus on the Opportunities sidebar link, then Enter: the URL changes but focus stays on the link, and the nav still claims role=tablist / role=tab aria-selected. Same keys: focus is on the hidden "Opportunities" page heading, which a screen reader announces. The sidebar is a plain <nav> and the link has aria-current="page".
admin-subtab-arrows before admin-subtab-arrows after
ArrowRight on the focused "General" Admin sub-tab does nothing; all four tabs are separate Tab stops. ArrowRight moves focus to "Purchasing policies" while General stays selected (manual activation). Only the selected tab is a Tab stop (tabindex 0/-1/-1/-1).

Verification

Run on this branch's head addedbbe on top of main a14dc8f2, from frontend/:

  • npm run lint: 0 errors (142 warnings already on main, none in changed files). Typecheck and build pass.
  • Jest: 111 suites, 3508 passed, 1 skipped. The new and changed suites also pass with --randomize (44 tests).
  • Playwright (e2e-local.sh): 161 passed, including the 4 tests in the new tab-keyboard.spec.ts.
  • Secrets scan clean (planted key flagged); scripts/test-git-secrets-allowlist.sh: 137 PASS.

Notes for review

  • The page headings are visually hidden (sr-only), so after a sidebar click sighted keyboard users see no focus ring; their next Tab continues from the top of the page content. If visible page titles are preferred, the same h2s can drop the sr-only class (a visual change).
  • The new headings are h2; the topbar brand stays the only h1.
  • Sub-tab activation is manual (arrows move focus, Enter/Space selects), not the APG default of automatic; see above.
  • Re-clicking the current sidebar item also moves focus to its heading.
  • Left alone: the status chip filters in history.ts (Purchases) and settings.ts (Accounts) also use role=tablist/tab with no tabpanel. They are filter toggles and would fit better as role=group with aria-pressed buttons; that can be a follow-up.
  • The "Failed to load settings: r is not iterable" banner in the Admin screenshots comes from the shared /api/config e2e mock and appears in both shots; it is not caused by this change.

Overlaps

No textual conflicts with the other UI branches. Branches touching the same files on other lines: #729 (index.html, mobile-nav.test.ts), #52, #183, #189, #213, #676, #739 and #749 (index.html), and #434, #707 and #711 (app.ts). Related: #216 covers mobile drawer focus; this branch touches the drawer only so it stops taking focus back from the new page heading after a link click.

Note for the UI owner

This changes UI code that @cristim owns. IAmJSD reviewed the before/after screenshots and asked for it to be merged; any design point you disagree with can be a follow-up.

🤖 Generated with Claude Code

IAmJSD and others added 2 commits October 23, 2026 23:22
…ge, add sub-tab keys

The sidebar declared the ARIA tab pattern (role=tablist/tab, aria-selected)
without its keyboard model, so screen readers announced "tab, 2 of 6" and
arrow keys did nothing. It is page navigation, so it is now plain links
with aria-current="page", and the top-level page containers drop their
tabpanel role.

Activating a sidebar link, or going back/forward, left focus on the link
and announced nothing. switchTab now takes a focusContent option, set by
the sidebar click handler and the popstate router, which focuses a visually
hidden <h2 tabindex="-1"> page heading at the top of each page. The
no-access placeholder keeps that heading. The mobile drawer no longer pulls
focus back to the hamburger when a link click has already moved it.

The Inventory and Admin sub-tab lists keep role=tablist, so they now get
its keyboard model (lib/tablist.ts): a roving tabindex kept in sync by the
sub-tab switchers, Left/Right with wrap-around, Home/End, hidden admin-only
tabs skipped. Activation is manual (Enter/Space) because each selection
fetches data and pushes a history entry.

Refs LeanerCloud#730

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…antics

Jest tests for lib/tablist.ts (roving tabindex, arrows with wrap, Home/End,
vertical orientation, hidden admin-only tabs, manual activation) and for
index.html's tab semantics, plus a Playwright spec that drives the real
keys in Chromium: Enter on a sidebar link focuses the new page heading,
Back restores focus, and the Admin and Inventory sub-tabs move focus with
the arrow keys and select with Enter/Space.

Refs LeanerCloud#730

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged urgency/this-quarter Within the quarter impact/all-users Affects every user effort/m Days type/bug Defect labels Oct 24, 2026
@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Caution

Review failed

The pull request is closed.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

IAmJSD and others added 2 commits October 24, 2026 16:49
Conflicts:
- frontend/src/app.ts: import block. Kept this branch's
  wireTablistKeyboard import and main's renderInitError import (LeanerCloud#1338).
- frontend/src/index.html: Inventory sub-tab buttons. Main added a
  Trends sub-tab (LeanerCloud#1317); this branch added a static roving tabindex.
  Kept all four tabs in main's order (Active commitments, Coverage,
  Trends, RI Exchange) with tabindex 0 on the selected tab and -1 on
  the rest. syncTabStops and the tablist keyboard model are generic over
  the buttons, so Trends gets arrow/Home/End handling with no code change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main added a fourth Inventory sub-tab (Trends, LeanerCloud#1317), so index.html now
has 8 role=tab buttons (4 Inventory + 4 Admin). The merge gave Trends
the same roving tabindex as the others; update the expected count.

Refs LeanerCloud#730

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD and others added 5 commits October 24, 2026 19:31
Conflicts:
- frontend/src/app.ts: import block; kept this branch's wireTablistKeyboard
  import and main's initPurchasingNav import (LeanerCloud#1336).
- frontend/src/inventory.ts: import block; kept this branch's syncTabStops
  import and main's ownValue import (LeanerCloud#1344).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eyboard e2e

The sidebar entries are links now (LeanerCloud#730), so the spec main added in
LeanerCloud#1388 can no longer find them by role=tab.

Refs LeanerCloud#730

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts:
- frontend/src/app.ts: main (LeanerCloud#1340) moved setupMobileNav and
  syncHeaderPlacement into frontend/src/nav/mobile-nav.ts. Kept both
  imports (wireTablistKeyboard from this branch, setupMobileNav from
  main) and took main's removal of the inline drawer code.
- frontend/src/nav/mobile-nav.ts (merged cleanly, edited by hand):
  ported this branch's drawer change into the moved module.
  closeDrawer takes returnFocus 'always' | 'if-inside', and a sidebar
  link click closes with 'if-inside' so focus stays on the new page
  heading. The MobileNavHandle.close() main added wraps
  closeDrawer() so it keeps its always-return-focus behavior.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts:
- frontend/src/app.ts: adjacent imports; kept both this branch's
  wireTablistKeyboard import and main's setOpenAccountsHandler import
  (LeanerCloud#1389 first-run empty state).
- frontend/src/index.html: #home-tab keeps this branch's markup (no
  role=tabpanel/aria-labelledby, hidden page heading as the first child)
  and main's #home-onboarding section (LeanerCloud#1389) right after the heading,
  before #summary.

The new "open Accounts" handler from main switches pages programmatically
(no focusContent), the same as the existing RI Exchange settings shortcut.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): APPROVE bdfdbcc32e602494e2692aa4a77ba7c2de516860

Blocking:

  • none
    Non-blocking:
  • frontend/src/lib/tablist.ts:23-45 with app.ts:158-160: a cold load at /admin/users or /admin/accounts runs switchSettingsSubTab and then wireTablistKeyboard, so syncTabStops runs twice. Both runs happen before updateUserUI() adds .admin-only.visible, so isAvailable() drops the selected admin-only tab, and General gets tabindex=0 while the selected "Users"/"Accounts" tab gets -1. Nothing re-syncs after updateUserUI. The result: an admin who deep-links to /admin/users and Tabs into the Admin tablist lands on "General, tab, not selected" instead of the selected tab. This breaks the "only the selected tab is a Tab stop" promise until the first sub-tab change. Fix: call syncTabStops on both tablists at the end of updateUserUI, or have syncTabStops keep the selected tab as the stop regardless of computed display. The e2e only covers /admin/general.
  • frontend/src/app.ts:243-246: main's first-run "open Accounts" handler (fix(frontend): show a first-run empty state instead of "no rows match" #1389) switches pages without focusContent. Focus stays on a button inside the now-hidden Home/Opportunities panel and falls to . This is user-initiated navigation that the new focusPageHeading would fit. It is pre-existing behaviour on main and not a regression.
  • frontend/src/tests/route-focus.test.ts:33: the ../inventory mock's isValidInventorySubSection list lacks 'trends' (main feat(frontend): add a Trends sub-tab for the analytics trends endpoint #1317). It is harmless for these tests but stale.
    Attacked: Read the full origin/main...HEAD diff (main 049c617 == merge base). Remerge-diffed f7dc82d, 8ac7a29, 01d426e and bdfdbcc (d930197 was clean). In f7dc82d, main's Trends sub-tab survived with tabindex=-1. In 01d426e, main's inline drawer removal was taken, and diffing nav/mobile-nav.ts against main shows only the closeDrawer(returnFocus) port. applyBreakpointState, syncHeaderPlacement and the MobileNavHandle.close semantics are intact. In bdfdbcc, #home-onboarding was kept after the new heading. Checked follow-ups 5c2edea (8 tabs) and 63e22ae (role=link). Grepped src and tests-e2e for leftover getByRole('tab')/aria-selected/tabpanel uses on the sidebar, and for CSS keyed on aria-selected; none remain. Verified listener order: the app.ts sidebar handler runs before mobile-nav's, so the drawer close does not steal heading focus. Also checked that confirmDialog restores focus before it resolves, so the dirty-guard path still ends on the heading. Ran tablist, route-focus, mobile-nav, sidebar-anchors, app, inventory and navigation jest suites in a scratch worktree: 214/214 passed.

@IAmJSD

IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at bdfdbcc3 merged onto current main (effe0baf; merge-tree 22a093f5; Linux, Node, headless Chromium):

  • This branch's own checks passed at this head: lint, typecheck, build, the full jest suite, the full Playwright e2e suite, and the git-secrets checks (see Verification in the PR body).
  • The combined tree of every UI PR in this merge wave (tree 22a093f5, main plus 5 PRs) passes npm ci, lint, typecheck, build, jest (1 skipped, 4374 passed, 4375 total), and the full Playwright suite (259 passed).
  • CI is green on bdfdbcc3 (CI Success, Run pre-commit hooks).
  • The independent review above approves this exact head.
  • Nothing was applied to any cloud.

@IAmJSD
IAmJSD marked this pull request as ready for review October 24, 2026 23:03
@IAmJSD
IAmJSD merged commit 10f2c8a into LeanerCloud:main Oct 24, 2026
29 checks passed
@IAmJSD

IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Marked ready and merged by IAmJSD's agent, at IAmJSD's request (IAmJSD reviewed the before/after screenshots; cristim is traveling), after the independent review and local verification above, all at bdfdbcc3.

IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 24, 2026
Conflicts resolved:
- frontend/src/toast.ts: kept this branch's hover/focus pause
  (startTimer/pauseTimer/resumeTimer and listeners) and main's LeanerCloud#742
  stack layout. After the toast is appended, call layoutToastStack()
  and startWatching() from main, then startTimer() from this branch;
  dropped the duplicate appendChild.
- frontend/src/__tests__/ladder.test.ts: kept both imports
  (refreshLadderTimeline from this branch, confirmDialog from main).
- frontend/tests-e2e/mobile-header-drawer.spec.ts: combined main's
  'navigation' role for Main navigation (LeanerCloud#1371) with this branch's
  "Log out" button name.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 24, 2026
Conflicts resolved:
- frontend/src/app.ts: import block only. Kept this branch's
  fanout-retry import and main's wireTablistKeyboard (LeanerCloud#730/LeanerCloud#1371) and
  setOpenAccountsHandler (LeanerCloud#1389) imports; the code using each merged
  cleanly.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/all-users Affects every user priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/bug Defect urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(frontend): ARIA tab pattern has no keyboard model and route changes keep focus

1 participant