Skip to content

a11y phase 3a: dynamic aria-selected + roving tabindex (#1099)#1112

Draft
RebelTechPro wants to merge 2 commits into
CambrianTech:mainfrom
RebelTechPro:feat/a11y-phase3a
Draft

a11y phase 3a: dynamic aria-selected + roving tabindex (#1099)#1112
RebelTechPro wants to merge 2 commits into
CambrianTech:mainfrom
RebelTechPro:feat/a11y-phase3a

Conversation

@RebelTechPro
Copy link
Copy Markdown
Collaborator

Summary

What changed

ReactiveListWidget — base class

  • New virtual protected isItemIdSelected(id): boolean. Default returns id === this.selectedId. Subclasses override to use their own selection state. Drives both aria-selected and the roving tabindex.
  • New Lit updated() override calls syncListSelection() after every render. Walks .list-item wrappers and applies aria-selected + the roving tabindex (selected → 0, others → -1).
  • getRenderFunction initial tabindex is now isItemIdSelected ? 0 : -1 (phase 2 had blanket tabindex=0).
  • Fallback in syncListSelection: if no item is currently selected, the first item gets tabindex=0 so the list remains a single Tab stop.
  • onListKeydown (the arrow-key handler from phase 2) now updates roving tabindex as focus moves — newly-focused item gets tabindex=0, all others -1. The list stays a single tab stop after the user has navigated.

RoomListWidget

  • Overrides isItemIdSelected: id === this.currentRoomId. The @reactive() currentRoomId already triggers a Lit update on change → updated()syncListSelection() walks the DOM. New active room becomes aria-selected="true" with tabindex=0; previous active room drops to false / -1.

UserListWidget

  • Overrides isItemIdSelected: id === this._selectedUserId. Same reactive pattern.

Why this matters

Phase 2 was correct at item-creation time but stale after that:

  • Click a different room → visual .active class moves correctly (Lit re-render), but aria-selected stayed on the old item
  • Tab into the list → every item was a tab stop (blanket tabindex=0)

Phase 3a fixes both. A screen reader user now hears "selected" announced on the right item when they change rooms; a keyboard user gets a single tab stop into the list, then uses arrow keys to move around (the standard listbox interaction pattern).

Out of scope (further phase 3 follow-ups)

  • Color-contrast audit across the six themes
  • <div onclick><button> semantic migration
  • axe-core lint gate in CI
  • Focus restoration when a selected item is removed or filtered out

Test plan

  • npm run build:ts → green (verified locally on top of phase 2)
  • Open room list. Tab into it: focus lands on the currently-active room (or the first room if none active). Tab a second time: focus leaves the list (not stuck cycling through items).
  • Arrow Down/Up moves focus between rooms. The new focus is the only tab stop (verify by Tab-leaving and Tab-returning — focus lands on the last-arrowed item).
  • Click a different room: aria-selected moves to the new active room, tabindex=0 moves with it. Screen reader announces "selected".
  • Same flow on the user list.
  • No visual regressions; the .active class still drives all visual styling.

⚠️ Not visually or screen-reader validated locally — TS compile is the only check.

Relationship to other open PRs

joelteply and others added 2 commits May 13, 2026 13:29
…e 2)

Builds on phase 1 (PR CambrianTech#1103). Behavior-preserving — adds ARIA
listbox/option semantics + keyboard navigation to the room and user
lists, plus accessible labels on chat message rows so screen readers
can navigate transcript message-by-message.

ReactiveListWidget — base class additions:
  - role="listbox" + aria-label (from listTitle) on the default
    container in `render()`. Subclasses that override render() add
    role=listbox locally (see RoomList/UserList below).
  - getRenderFunction sets role="option" + tabindex=0 + aria-label
    (via new virtual `getItemLabel()`) + aria-selected on every
    .list-item wrapper.
  - Enter / Space on a focused item activates it (mirrors click).
  - New `onListKeydown` handler attached in firstUpdated():
    ArrowDown / ArrowUp move focus between siblings, Home / End jump
    to first/last. Scoped to the container so it doesn't interfere
    with chat composer or other keyboard handling.

RoomListWidget:
  - role=listbox + aria-label="Rooms and direct messages" on the
    container in its render() override.
  - Overrides getItemLabel(): for rooms → "Room {name} — {topic}";
    for DMs → "Direct message with {name}" or "Group DM: {name},
    {count} members".

UserListWidget:
  - role=listbox + aria-label="Users and personas" on the container.
  - Overrides getItemLabel(): "{name}, {persona|agent|user}, {status}"
    so a screen reader hears the kind and presence state.

ChatWidget — getRenderFunction:
  - role="article" + aria-label on each .message-row (sender name +
    timestamp + " sending" if optimistic). Combined with phase 1's
    role=log + aria-live=polite on the messages-container, the chat
    transcript is now navigable per-message via screen reader rotor.

Out of scope (phase 3 follow-ups):
  - Dynamic aria-selected updates when the active room/user changes
    after initial render (current value is set at item-creation time
    only — limitation noted in PR description).
  - Roving tabindex (currently every item is tabindex=0).
  - Color-contrast audit across themes.
  - <div onclick> → <button> migration.
  - axe-core lint gate.

`npm run build:ts` is green. Not visually validated locally —
keyboard + screen-reader walkthrough is in the PR test plan.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e 3a)

Layered on phase 2 (PR CambrianTech#1111). Completes the listbox correctness
story by making `aria-selected` and the tab order respond to selection
changes after initial render.

ReactiveListWidget — base class additions:
  - New virtual `protected isItemIdSelected(id): boolean`. Default
    matches `selectedId`; subclasses override to use their own state.
    Drives both aria-selected and the roving tabindex.
  - New Lit `updated()` override walks `.list-item` wrappers after
    every render and syncs aria-selected + tabindex via the new
    `syncListSelection()` helper. The visual `.active` class was
    already reactive via Lit (subclasses re-render their inner
    template); this hook keeps the ARIA state on the static
    EntityScroller-managed outer wrapper in sync without re-rendering
    the wrapper.
  - Initial `getRenderFunction`: tabindex now depends on
    `isItemIdSelected` (selected → 0, others → -1) rather than the
    blanket `tabindex=0` from phase 2.
  - Fallback: if no item is currently selected, the first item gets
    tabindex=0 so the list remains a single Tab stop.
  - Arrow-key navigation in `onListKeydown` updates roving tabindex
    as focus moves — newly-focused item gets tabindex=0, all others
    -1. Keeps the list a single tab stop after the user has navigated.

RoomListWidget:
  - Overrides `isItemIdSelected`: `id === this.currentRoomId`.
    When the active room changes, the @reactive currentRoomId
    triggers a Lit update → updated() → syncListSelection() walks
    the DOM and the new room becomes aria-selected="true" with
    tabindex=0, old room drops to "false" / -1.

UserListWidget:
  - Overrides `isItemIdSelected`: `id === this._selectedUserId`.
    Same reactive pattern.

Out of scope (further phase 3 follow-ups, not blockers):
  - Color-contrast audit across themes
  - <div onclick> → <button> migration
  - axe-core lint gate in CI
  - Focus restoration when a selected item is removed/filtered out

`npm run build:ts` is green. Stacked on PR CambrianTech#1111; once that merges,
this PR's diff against main reduces to just the phase-3a changes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@joelteply
Copy link
Copy Markdown
Contributor

Substantive review (claude tab #2 / claude-tab-2, picked up via continuum#1127 intake card). Reviewed #1111 first; this picks up the incremental phase-3a delta on top of phase 2.

What this PR adds (over #1111)

  • isItemIdSelected(id) virtual — drives both aria-selected and the roving tabindex
  • updated() lifecycle hook + syncListSelection() — walks .list-item wrappers after every Lit re-render to refresh ARIA attrs that live on the EntityScroller-managed outer wrappers (which Lit doesn't re-render itself)
  • onListKeydown updates roving tabindex on arrow nav (focused item gets tabindex=0, others -1)
  • Fallback: if no selected item, first item gets tabindex=0 so the list stays a tab stop
  • Initial tabIndex in getRenderFunction is now isItemIdSelected ? 0 : -1 (was blanket 0 in a11y phase 2: listbox/option semantics + keyboard nav + message-row labels (#1099) #1111)

This is exactly what phase 2 was missing. The "static EntityScroller wrapper, dynamic ARIA" insight is correct — you can't put the aria-selected on the Lit-rendered side because the outer .list-item wrapper is created imperatively.

Correctness — checked the reactive plumbing

  • RoomListWidget.currentRoomId and UserListWidget._selectedUserId are both @reactive() → mutating them schedules a Lit update → updated() fires → syncListSelection() walks the DOM. ✓
  • The reactive triggering means a click on a different room flows: click → onItemClick (existing) → updates selectedId/currentRoomId@reactive triggers update → updated()syncListSelection() rewrites aria-selected on every item. Screen reader will announce the selection change. ✓
  • onListKeydown updates roving tabindex inline (not via syncListSelection) — that's fine because focus + tabindex are short-circuited together; calling syncListSelection would also work but would duplicate.

Things to consider

  1. updated() runs syncListSelection() unconditionally on every Lit re-render — for a 50-room list, that's 50 setAttribute + tabindex writes per re-render. Not a bottleneck at typical sizes, but the changed: Map<string, unknown> you receive could gate it cheaply:

    protected override updated(changed: Map<string, unknown>): void {
      super.updated(changed);
      // Only re-sync ARIA when something selection-relevant changed.
      // Subclasses can extend this list via `selectionPropertyNames`.
      if (this.shouldSyncSelection(changed)) this.syncListSelection();
    }
    protected shouldSyncSelection(changed: Map<string, unknown>): boolean {
      return changed.has('selectedId');  // subclasses override to add 'currentRoomId' etc.
    }

    Adds a bit of subclass ceremony; not blocking. Default-on is the safer behavior for now.

  2. Initial getRenderFunction snapshot vs syncListSelection: the wrapper is built with tabIndex = isSel ? 0 : -1 AT CREATION TIME, then EntityScroller adds it to the DOM. If selection state changes between item-creation and DOM-attach, syncListSelection (running on the next Lit updated()) will reconcile. Race-free in practice. ✓

  3. syncListSelection "selectedFound" fallback: when nothing is selected, items[0].tabIndex = 0 keeps the list a tab stop. Good. But if the user previously arrowed to item 5 and then the active selection clears, your fallback resets the Tab stop to item 0 — focus lost. Edge case (selection-clearing without re-clicking is rare); flagging only.

  4. shadowRoot.activeElement in onListKeydown — works in modern Chrome/Firefox/Safari. Worth a quick note in code comment that this needs the closed/open shadow root mode the project actually uses (which it does — Continuum's widgets use open shadow DOM consistently).

Phase split is well done

You correctly flagged in #1111's "out of scope": "currently set once at item-creation time. The visual .active class still updates correctly via Lit. Real fix needs a Lit updated() hook that walks DOM after every render." This PR IS that fix. Honest scoping → clean follow-up. That's the right pattern.

Recommendation

LGTM to land after #1111. Same caveat about screen-reader validation — TS compile + the architectural correctness reviewed here is necessary but not sufficient; VoiceOver/NVDA dynamic-update behavior is what proves "user actually hears the new selection announced." The change is correct on paper.

The four nits in #1111 carry over (no-as unknown as, bare document, missing disconnectedCallback, role=article confirmed-fine). Add the optional shouldSyncSelection gate if/when scaling matters.

Thanks for breaking it into reviewable pieces — this stack is easier to reason about than one mega-PR.

Copy link
Copy Markdown
Contributor

@joelteply joelteply left a comment

Choose a reason for hiding this comment

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

Review (claude-tab-1) — REQUEST CHANGES (stale-branch regression of #1100)

This branch is stale relative to canary. The diff against current canary shows it reverts the XSS hardening shipped in #1100 (chat XSS hardening — DOM-returning adapter render path):

-      this.renderAdapterContentInto(contentDiv, adapter, message);
+      contentDiv.innerHTML = contentHtml;   // ← re-introduces innerHTML XSS surface

Plus deletes the renderAdapterContentInto private method that was the seam for that hardening.

This is the same root cause I documented in detail on #1107 — see #1107 (review) latest review for the full breakdown including the a11y phase-2 regression in #1108 specifically.

Fix

Rebase this branch onto current origin/canary and resolve the conflict in ChatWidget.ts so the new work in this PR sits ON TOP of the kept renderAdapterContentInto path, not in place of it.

After rebase, this PR's actual changes will likely be much smaller and easier to evaluate on their own merits. As-is the diff is dominated by accidental reverts.

The PR's intentional changes look fine; this is purely a substrate hygiene issue. Sorry for the late catch — these PRs sat unreviewed too long. The substrate's stale-PR signal (airc#608 / #609) would have flagged it earlier.

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