fix(a11y): dialog semantics, focus management, server-rendered tab state (#21)

[fsecada01]

Closes #21. Phase 2 of epic #19.

Three gaps that predate 0.1.0, flagged during the #6 review and deferred so a theme PR would not widen into an accessibility PR. This is the phase that unblocks the theme-expansion epic (#25): every fix here is a pattern the three new themes copy, so landing it against the two existing themes means writing it twice instead of five times.

---

1. Modal — dialog semantics and focus management

• role="dialog" and aria-modal="true" on all four modal templates
• aria-labelledby pointing at the header slot's content, with a documented aria-label fallback when there is no header
• Focus moved into the dialog on open
• Focus restored to the trigger on close
• Focus trapped while open
• Escape closes
• All focus behavior in cf_ui_alpine.js (cfModal / initModal()), not per template

Labelling picks exactly one of the two, never both — an aria-labelledby aimed at an empty element resolves to the empty string and announces nothing, which is worse than no labelling. The fallback is a new label prop defaulting to "Dialog".

The trigger is captured, not declared: show() records document.activeElement, so any opener works — a button, a link, Alpine.store('cf').modal.open(id) — and no template has to name it.

The <div> decision is recorded in docs/accessibility.md with its reasoning, per the issue, so it is not relitigated once the three new themes arrive.

tests/unit/test_accessibility.py::test_no_modal_template_implements_focus_itself asserts the "in Alpine, not per template" half structurally — no .focus() and no keydown in any modal partial. Five themes carrying five slightly-different focus traps is the failure mode it exists to catch.

2. Tabs — server-rendered active state

• Accept an active prop on CfTabs / <c-cf.tabs> and render the active class server-side
• Keep the Alpine binding for client-side switching
• aria-selected and aria-controls, server-rendered for the same reason
• Same treatment for CfPanel's open prop — it had the same gap

CfPanel did have it, and worse: open was declared in the {#def} and never rendered anywhere, so an open panel still emitted x-cloak and stayed hidden permanently without JS. It now renders its state, initPanel() seeds Alpine from it rather than resetting to closed, and it takes a new id prop for the aria-controls target.

3. Remaining interactive components

• CfPanel — aria-controls, aria-expanded (plus: the Bulma toggle was a <button> with no type, so it would submit an enclosing form)
• CfTabs — roving tabindex
• Anything else with an Alpine-only state binding and no server-rendered equivalent

Audited cfNavbar and cfNotification too. Both are Alpine-only by nature — a collapsed mobile menu and a dismissed toast have no meaningful server state — and both already carry correct ARIA. No change.

Roving tabindex uses manual activation: arrows move focus, Enter/Space activates. Automatic activation would fire an HTMX request on every arrow press. With no active prop the first tab holds tabindex="0" — these anchors carry no href, so a widget where every tab is -1 is unreachable by keyboard entirely.

Testing

• E2E in both js_on and js_off
• Assert focus location after open and after close, not attribute presence
• Assert focus does not escape the dialog on Tab from the last focusable element
• Strengthen the js_off tab assertion from "attached" to "exactly one tab carries the active class"
• just check, just test-js, and the full E2E tier green

Split along the line the issue draws. Markup claims — a role, an aria-* value, a server-rendered class — live in tests/unit/test_accessibility.py, run against all four template sets. Behavioral claims live in E2E.

test_jinja_tabs_activate was rewritten rather than extended: it asserted the first tab is not active, clicks it, asserts it is — which only held because no tab was ever active to begin with. The gallery now ships active="tab1", so it asserts a move, and the js_off half has something real to check.

The behavioral tests were mutation-tested

They were written after the implementation, so "they pass" proves nothing on its own. Disabling the focus management in cf_ui_alpine.js fails exactly the six tests that measure it. Disabling only the Tab branch — leaving focus-in, focus-restore, and Escape intact — fails exactly the two trap tests and nothing else, so each assertion is measuring the thing it names.

543 passed   pytest unit + integration
 80 passed   pytest e2e (12 skipped: js_off variants of JS-only behavior)
 65 pass     node --test

prek run --all-files clean, run twice per the ruff-pin recurrence noted in the repo's CLAUDE.md.

---

Scope notes

New id prop on CfPanel. aria-controls needs a target id. Two panels on one page now need distinct ids or they emit duplicate element ids. Called out in the changelog.

The cotton E2E pages carry no Alpine. The E2E Django server is a bare WSGIHandler and serves no static files. That half exists to prove the new props survive <c-vars> — which the unit tier structurally cannot do, since render_to_string bypasses the compiler — and focus behavior is asserted on the JinjaX galleries where cf_ui_alpine.js actually loads. Wiring StaticFilesHandler into the E2E server would let the cotton half assert behavior too; left out as scope.

Follow-ups, not folded in

1. A closed panel still cannot be opened without JS. Fixing it properly means <details>/<summary>, which is a different component shape, not an attribute change. Noted in docs/accessibility.md.
2. Pre-existing Alpine expression interpolation in the tabs partials. setActive('{{ tab.id }}') and :class="{ 'is-active': active === '{{ tab.id }}' }" splice a value into evaluated JS. Predates this phase, and the new active prop deliberately avoids it (data-cf-active + initTabs()) rather than joining it. Worth a ticket.
3. Bare ruff format . rewrites Python snippets inside markdown, and one rewrite is wrong — it turns an INSTALLED_APPS line in prompts/migrate-django.md into a tuple. Nothing in CI, just format, or the prek hooks touches those paths, so nothing is broken today. Carried over from Axis layer: enforce every guarantee it states #20; wants an extend-exclude.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NhqNRBg83czKfr8L6FF5xf

[fsecada01]

Adversarial review — PR #27

Reviewed the full diff (28 files, +1354/−61). Two findings, both confirmed by execution rather than by reading. Neither is a regression; both are places where the implementation is narrower than what the PR and its own docs claim.

---

Overview

Adds ARIA semantics and keyboard behavior to CfModal, CfTabs, and CfPanel across all four template sets, with the behavior centralized in cf_ui_alpine.js so the three themes in epic #25 inherit it. Structure is right: theme partials carry classes and static ARIA state, cf_ui_alpine.js carries every focus decision, and test_no_modal_template_implements_focus_itself enforces that split structurally instead of by convention.

The data-cf-active / data-cf-open + x-init route for passing server state into Alpine is the correct call. x-data="cfTabs('{{ active }}')" would splice a request-controlled value into an evaluated expression, where template escaping cannot help — an apostrophe alone breaks out. Good that the new code avoids it rather than matching the surrounding pre-existing interpolation.

---

Finding 1 — an unmatched active removes the whole tablist from the tab order [medium]

src/cf_ui/templates/{jinja/*/Tabs.jinja,cotton/_themes/*/tabs.html}, and tabIndexFor() in cf_ui_alpine.js.

The roving-tabindex fallback keys on not active, so it only fires when active is empty — not when it is set to a value no tab matches:

active=''                       tabindex=['0', '-1']   ← guarded
active='one'                    tabindex=['0', '-1']   ← correct
active='typo-or-stale-slug'     tabindex=['-1', '-1']  ← nothing is focusable

tabIndexFor() agrees (if (this.active) return this.active === id ? 0 : -1), so Alpine does not repair it client-side either. These anchors carry no href, so tabindex is the only thing making them focusable — every tab at -1 means the widget is unreachable by keyboard, with no visible symptom.

docs/accessibility.md names this exact failure mode and then states the guarantee unconditionally:

With no active prop, the first tab holds tabindex="0". It has to: […] a widget where every tab is -1 is unreachable by keyboard.

The trigger is plausible: a tab id renamed on one side of the call, or an active read from a URL param or stored preference that no longer matches. Not a regression — pre-PR the anchors had no tabindex at all and were never focusable — but it is an incomplete fix that reads as complete.

The fix is "fall back to the first tab when nothing matches", not "when active is empty", in all three places. Server-side that needs a precomputed does-any-tab-match flag, since neither Django nor Jinja can look ahead inside the loop.

test_{jinja,cotton}_tabs_stay_keyboard_reachable_with_no_active_tab covers only active="", which is why this got through.

Finding 2 — the initModal() comment overclaims focus restoration [low]

src/cf_ui/static/cf_ui/cf_ui_alpine.js:

// Watched rather than folded into show(), so a consumer that sets
// `open` directly still gets focus management.
this.$watch('open', (isOpen) => {
    if (isOpen) this.$nextTick(() => this._focusFirst());
});

True for opening, not for closing. Restoration lives in close(), which the watcher never calls, so setting open = false directly skips it. Measured in a browser against the gallery:

FOCUS INSIDE AFTER OPEN:            True
CLASS AFTER DIRECT CLOSE:           modal        (closed correctly)
ACTIVE ELEMENT AFTER DIRECT CLOSE:  BODY#        (not #open-modal)

Focus lands on <body> — the browser evicting it from a now-hidden subtree — rather than returning to the trigger. Nothing in CHANGELOG.md or docs/accessibility.md promises this, so the blast radius is the comment itself, and every path the components actually use (close(), the close button, backdrop click, Escape, cf-modal-close) is correct. Either move restoration into the watcher's else branch, or narrow the comment to say it covers opening only.

---

Checked and clean

• aria-selected / aria-expanded survive a falsy Alpine binding. Alpine whitelists aria-pressed/aria-checked/aria-expanded/aria-selected against its remove-when-falsy rule, so :aria-selected="false" renders aria-selected="false" rather than dropping the attribute. Confirmed in the E2E counts.
• x-init ordering. The root's x-init runs before children are walked, so :class / :tabindex / x-show on descendants read the seeded value. No flash, no reset to closed. Verified in E2E after _wait_for_alpine.
• Tailwind tree-shaking. class="tab {% if … %}tab-active{% endif %}" keeps tab-active as a whole literal token; test_no_daisy_class_name_is_assembled_at_render_time still passes on the rewritten partials.
• Django {% if %} precedence in tab.id == active or not active and forloop.first binds as (…) or ((not active) and forloop.first). Correct, and covered.
• this.$el inside Alpine.data() methods resolves to the x-data root, not the element the handler is bound to — so _tabs() and _trapTab() scope as intended even though @keydown sits on the tablist child.
• not_to_have_attribute("x-cloak", "") does detect a valueless present attribute (returns ""), so the cotton panel assertion is not vacuous.
• js_off coverage is real. Five of the new E2E tests run unconditionally in both modes rather than skipping; the 12 skips are all genuinely JS-only behavior.
• Mutation testing. Reproduced the PR's claim: disabling only the Tab branch fails exactly the two trap tests and nothing else.

Nit, not a finding

onKeydown's (current < 0 ? tabs[0] : next).focus() overrides the End case, so End with no tab focused would go to the first tab. Unreachable with the shipped markup — the handler sits on the tablist and the tabs are its only focusable descendants — so I could not construct a failing scenario. Mentioned only so it is not mistaken for intent later.

---

Recommendation: Finding 1 is worth fixing before merge — it is a silent keyboard-accessibility hole in an accessibility PR, and the same three-place fix will otherwise be copied into three more themes by epic #25. Finding 2 is a one-line comment or a one-line else; either is fine.

🤖 Generated with Claude Code

[fsecada01]

CI follow-up — now green on 3.11 / 3.12 / 3.13

Three js_on E2E tests failed on CI while passing locally on every run. Two commits, two distinct causes:

99dc44c — _focusFirst() now retries. focus() on an element that is not rendered yet is a silent no-op, on the first tabbable child and on the $el fallback alike. Alpine does not order the :class effect that reveals the dialog against the open watcher's $nextTick, so focus could be attempted while display: none still applied, with nothing to retry it. It now verifies the result and retries per animation frame, bounded by CF_FOCUS_ATTEMPTS, bailing out if the dialog closed meanwhile. This also covers a theme that reveals behind a transition. test_modal_takes_focus_even_when_the_reveal_is_late makes the race deterministic by pinning the dialog hidden with a rule that outranks is-active and lifting it two frames later; disabling the retry fails it with exactly the symptom CI reported.

7d16099 — the assertions now wait. That first commit did not fix CI, and the log said why: the dialog was already class="modal is-active" with focus still outside. So the reveal was not the problem — reading document.activeElement once, immediately after to_have_class returns, races the retry loop. A fast machine wins that race every time; a loaded runner does not. test_escape_closes_the_modal was downstream of the same thing: Escape is handled on the dialog, so a keypress sent while focus is still on <body> never reaches the handler. Both suites now go through _wait_for_modal_focus() before asserting focus or pressing a key the dialog handles.

Both changes are load-bearing — without the retry focus never lands at all, so waiting alone would not have helped either.

The two findings from the review comment above are untouched and still open for your call.