fix(a11y): heading hierarchy, error page h1, cover-modal alt, live region (bookshelf-vqy9.19) #738

Merged

zombor merged 2 commits from bd-bookshelf-vqy9.19 into main 2026-06-24 02:56:24 +00:00

Conversation 2 Commits 2 Files changed 6 +172 -10

zombor commented 2026-06-23 20:09:29 +00:00

Owner

Copy link

Summary

Four minor a11y fixes from the vqy9.2 audit (M-4, M-7, M-8, N-4):

• M-4 (settings_shell.html): 7 panel headings changed from <h1 class="page-title"> to <h2> — they live inside role="tabpanel" sections that are already below the page-level <h1> in base.html, so <h1> was creating a duplicate top-level heading (WCAG 1.3.1).
• M-7 (error.html): Error status promoted from <p class="error-page-status"> to <h1> so screen-reader users can jump to the error message via heading navigation shortcuts.
• M-8 (books_show.html): Cover-modal <img> had alt="" (correct only for decorative images). Replaced with alt="Cover of {{.Book.DisplayTitle}}" since the enlarged cover is informational.
• N-4 (books_show.html): metadata-refetch-status span lacked role="status" aria-live="polite", so async fetch-result text was never announced to AT.

No inline style= added. No canonical class changes.

Test plan

• 8 new assertions in internal/tmpl/a11y_render_test.go covering each change
• make test — all pass
• npm test — 2533 JS tests pass
• make lint — 0 issues in this worktree

Closes bead bookshelf-vqy9.19 on merge.

## Summary Four minor a11y fixes from the vqy9.2 audit (M-4, M-7, M-8, N-4): - **M-4** (`settings_shell.html`): 7 panel headings changed from `<h1 class="page-title">` to `<h2>` — they live inside `role="tabpanel"` sections that are already below the page-level `<h1>` in `base.html`, so `<h1>` was creating a duplicate top-level heading (WCAG 1.3.1). - **M-7** (`error.html`): Error status promoted from `<p class="error-page-status">` to `<h1>` so screen-reader users can jump to the error message via heading navigation shortcuts. - **M-8** (`books_show.html`): Cover-modal `<img>` had `alt=""` (correct only for decorative images). Replaced with `alt="Cover of {{.Book.DisplayTitle}}"` since the enlarged cover is informational. - **N-4** (`books_show.html`): `metadata-refetch-status` span lacked `role="status" aria-live="polite"`, so async fetch-result text was never announced to AT. No inline `style=` added. No canonical class changes. ## Test plan - [x] 8 new assertions in `internal/tmpl/a11y_render_test.go` covering each change - [x] `make test` — all pass - [x] `npm test` — 2533 JS tests pass - [x] `make lint` — 0 issues in this worktree Closes bead bookshelf-vqy9.19 on merge.

zombor added 1 commit 2026-06-23 20:09:30 +00:00

fix(a11y): heading hierarchy, error heading, cover-modal alt, live region (bookshelf-vqy9.19) ... ea3f385c82

- M-4: settings_shell.html — panel headings changed from h1 to h2 (7 occurrences)
  so they sit correctly below the page-level h1 in base layout
- M-7: error.html — error-page-status promoted from <p> to <h1> so
  screen-reader users can navigate to the error via heading shortcuts
- M-8: books_show.html cover-modal — empty alt="" replaced with
  alt="Cover of {{.Book.DisplayTitle}}" so enlarged cover is described
- N-4: books_show.html metadata-refetch-status — added role="status"
  aria-live="polite" so async fetch completion is announced to AT

Tests: 8 new assertions in internal/tmpl/a11y_render_test.go covering each
change with either real-template render or static source checks.

zombor force-pushed bd-bookshelf-vqy9.19 from ea3f385c82 to 0caf08182b 2026-06-23 20:17:07 +00:00 Compare

zombor commented 2026-06-23 20:24:10 +00:00

Author

Owner

Copy link

CODE REVIEW: NOT APPROVED

Phase 0: DEMO Verification

No DEMO block is required for a pure a11y/template PR — CI acts as the behavioral gate. Proceeding to Phase 1.

---

Phase 1: Spec Compliance

All four stated changes are present in the diff:

• settings_shell.html: 7 h1→h2 conversions
• error.html: status p→h1
• books_show.html: cover-modal alt text + live-region attributes
• internal/tmpl/a11y_render_test.go: 8 new assertions

---

Phase 2: Code Quality

[MAJOR] internal/tmpl/a11y_render_test.go — live-region tests are tautological (zero regression protection)

The two It blocks for the metadata-refetch-status live region assert:

Expect(body, readErr).To(ContainSubstring(`role="status"`))
Expect(body, readErr).To(ContainSubstring(`aria-live="polite"`))

Both of these strings already appear in books_show.html before this PR — on the flash <div> elements at lines 33 and 42 of the template:

<div class="flash flash--success" role="status" aria-live="polite" ...>

The test reads the entire raw file and uses ContainSubstring. It would have passed unchanged on origin/main — before the metadata-refetch span had any role or aria-live attribute. This is the same tautological-test pattern caught in #736: the test passes because of a pre-existing element, not because the fix is in place. Removing the new attributes from the <span> would not cause these tests to fail.

Fix: scope the assertion to the metadata-refetch-status element, e.g.:

Expect(body, readErr).To(MatchRegexp(`metadata-refetch-status[^>]+role="status"`))
Expect(body, readErr).To(MatchRegexp(`metadata-refetch-status[^>]+aria-live="polite"`))

[MAJOR] templates/pages/settings_shell.html — heading hierarchy is now broken (no h1 on the page)

Before this PR, each visible settings tab panel had an <h1 class="page-title"> — one h1 visible at a time (hidden panels are hidden). That was a valid heading structure.

After this PR, the panel headings are <h2> but there is no h1 anywhere on the page. The base.html layout only renders its <h1 class="top-bar-title">{{.PageTitle}}</h1> when PageTitle is non-empty, and internal/settings/shell_handler.go:254 sets base.Title = "Settings" but never sets base.PageTitle. The settings_shell.html template itself contains no <h1> at all.

Result: screen-reader users navigate to "Email Settings" as an h2 with no h1 parent — a skipped heading level, which WCAG 2.1 SC 1.3.1 treats as worse than the single-h1-per-tab approach that existed before.

The fix should either (a) set base.PageTitle = "Settings" in the shell handler so the top-bar h1 renders, or (b) add an explicit visually-hidden <h1>Settings</h1> inside settings_shell.html.

The test for this (does not contain any h1 with class page-title) correctly asserts the panel headings changed, but it doesn't verify that any h1 exists on the page — so it doesn't catch the broken hierarchy.

---

Minor issues (do not block)

[MINOR] internal/tmpl/a11y_render_test.go — the settings_shell test (does not contain any h1 with class page-title / renders panel headings as h2) correctly confirms the panel-heading element change but does not assert a h1 exists anywhere on the rendered page, leaving the broken-hierarchy scenario undetected by tests.

---

Verdict

REVIEW VERDICT: 0 blocker, 2 major, 1 minor

## CODE REVIEW: NOT APPROVED ### Phase 0: DEMO Verification No DEMO block is required for a pure a11y/template PR — CI acts as the behavioral gate. Proceeding to Phase 1. --- ### Phase 1: Spec Compliance All four stated changes are present in the diff: - `settings_shell.html`: 7 h1→h2 conversions - `error.html`: status p→h1 - `books_show.html`: cover-modal alt text + live-region attributes - `internal/tmpl/a11y_render_test.go`: 8 new assertions --- ### Phase 2: Code Quality **[MAJOR] internal/tmpl/a11y_render_test.go — live-region tests are tautological (zero regression protection)** The two `It` blocks for the metadata-refetch-status live region assert: ```go Expect(body, readErr).To(ContainSubstring(`role="status"`)) Expect(body, readErr).To(ContainSubstring(`aria-live="polite"`)) ``` Both of these strings already appear in `books_show.html` **before this PR** — on the flash `<div>` elements at lines 33 and 42 of the template: ```html <div class="flash flash--success" role="status" aria-live="polite" ...> ``` The test reads the entire raw file and uses `ContainSubstring`. It would have **passed unchanged on `origin/main`** — before the metadata-refetch span had any `role` or `aria-live` attribute. This is the same tautological-test pattern caught in #736: the test passes because of a pre-existing element, not because the fix is in place. Removing the new attributes from the `<span>` would not cause these tests to fail. Fix: scope the assertion to the `metadata-refetch-status` element, e.g.: ```go Expect(body, readErr).To(MatchRegexp(`metadata-refetch-status[^>]+role="status"`)) Expect(body, readErr).To(MatchRegexp(`metadata-refetch-status[^>]+aria-live="polite"`)) ``` **[MAJOR] templates/pages/settings_shell.html — heading hierarchy is now broken (no h1 on the page)** Before this PR, each visible settings tab panel had an `<h1 class="page-title">` — one h1 visible at a time (hidden panels are hidden). That was a valid heading structure. After this PR, the panel headings are `<h2>` but there is **no h1 anywhere on the page**. The `base.html` layout only renders its `<h1 class="top-bar-title">{{.PageTitle}}</h1>` when `PageTitle` is non-empty, and `internal/settings/shell_handler.go:254` sets `base.Title = "Settings"` but never sets `base.PageTitle`. The settings_shell.html template itself contains no `<h1>` at all. Result: screen-reader users navigate to "Email Settings" as an h2 with no h1 parent — a skipped heading level, which WCAG 2.1 SC 1.3.1 treats as worse than the single-h1-per-tab approach that existed before. The fix should either (a) set `base.PageTitle = "Settings"` in the shell handler so the top-bar h1 renders, or (b) add an explicit visually-hidden `<h1>Settings</h1>` inside `settings_shell.html`. The test for this (`does not contain any h1 with class page-title`) correctly asserts the panel headings changed, but it doesn't verify that **any** h1 exists on the page — so it doesn't catch the broken hierarchy. --- ### Minor issues (do not block) [MINOR] `internal/tmpl/a11y_render_test.go` — the settings_shell test (`does not contain any h1 with class page-title` / `renders panel headings as h2`) correctly confirms the panel-heading element change but does not assert a h1 exists anywhere on the rendered page, leaving the broken-hierarchy scenario undetected by tests. --- ### Verdict REVIEW VERDICT: 0 blocker, 2 major, 1 minor

zombor added 1 commit 2026-06-23 20:33:37 +00:00

fix(a11y): restore settings h1, scope live-region test assertions ... 2179607830

[MAJOR] shell_handler.go: set base.PageTitle = "Settings" so the real
base.html top-bar renders an <h1> — the h1→h2 change on panel headings
had left the settings page with no h1 at all (broken hierarchy regression).

[MAJOR] a11y_render_test.go: replace tautological ContainSubstring
assertions on role="status"/aria-live="polite" with MatchRegexp scoped
to the metadata-refetch-status element, preventing false positives from
the flash divs that also carry those attributes.

[MINOR] shell_handler_test.go: add regression-guard test that drives the
handler through a base-layout stub with the PageTitle→h1 block; the test
fails when PageTitle is not set, so this class of regression can't recur.

Delete-confirm-fail verified for all three assertions (see PR comment).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-06-23 20:41:34 +00:00

Author

Owner

Copy link

Review fix: 2 MAJOR + 1 MINOR applied

[MAJOR] h1 hierarchy regression (PageTitle):
Added base.PageTitle = "Settings" in internal/settings/shell_handler.go:254 so the base.html top-bar renders <h1 class="top-bar-title">Settings</h1>. Without this the settings page had no h1 after the panel headings were changed to h2.

Delete-confirm-fail (h1): Temporarily removed base.PageTitle = "Settings" from the handler → the new regression-guard test in shell_handler_test.go (GetSettingsShellHandler: heading hierarchy) immediately FAILED with Expected `<h1 class="top-bar-title">Settings</h1>`. Restored → test PASSES. ✓

[MINOR] h1 regression-guard test:
Added a new Describe("GetSettingsShellHandler: heading hierarchy (regression: PageTitle→h1)") block in internal/settings/shell_handler_test.go that drives the handler through a base-layout stub rendering the same {{if .PageTitle}}<h1>{{end}} guard as the real base.html. If anyone removes base.PageTitle from the handler, the test will catch it.

[MAJOR] Tautological live-region assertions:
Replaced both ContainSubstring(\role="status"`)/ContainSubstring(`aria-live="polite"`)withMatchRegexp(`metadata-refetch-status[^>]+role="status"`)andMatchRegexp(`metadata-refetch-status[^>]+aria-live="polite"`)ininternal/tmpl/a11y_render_test.go`. The old assertions passed even when the span had neither attribute (flash divs in books_show.html also carry those strings). The new assertions are scoped to the element under test.

Delete-confirm-fail (live-region role): Temporarily removed role="status" from the metadata-refetch-status span in books_show.html → test FAILED: Expected metadata-refetch-status[^>]+role="status". The old ContainSubstring would have PASSED (flash divs still had the attribute). Restored → test PASSES. ✓

Delete-confirm-fail (live-region aria-live): Removed aria-live="polite" from the span (leaving role intact) → test FAILED: Expected metadata-refetch-status[^>]+aria-live="polite". Restored → test PASSES. ✓

CI: green ✓

## Review fix: 2 MAJOR + 1 MINOR applied **[MAJOR] h1 hierarchy regression (PageTitle):** Added `base.PageTitle = "Settings"` in `internal/settings/shell_handler.go:254` so the base.html top-bar renders `<h1 class="top-bar-title">Settings</h1>`. Without this the settings page had no h1 after the panel headings were changed to h2. **Delete-confirm-fail (h1):** Temporarily removed `base.PageTitle = "Settings"` from the handler → the new regression-guard test in `shell_handler_test.go` (`GetSettingsShellHandler: heading hierarchy`) immediately FAILED with _Expected \`\<h1 class="top-bar-title">Settings\</h1>\`_. Restored → test PASSES. ✓ **[MINOR] h1 regression-guard test:** Added a new `Describe("GetSettingsShellHandler: heading hierarchy (regression: PageTitle→h1)")` block in `internal/settings/shell_handler_test.go` that drives the handler through a base-layout stub rendering the same `{{if .PageTitle}}<h1>{{end}}` guard as the real `base.html`. If anyone removes `base.PageTitle` from the handler, the test will catch it. **[MAJOR] Tautological live-region assertions:** Replaced both `ContainSubstring(\`role="status"\`)` / `ContainSubstring(\`aria-live="polite"\`)` with `MatchRegexp(\`metadata-refetch-status[^>]+role="status"\`)` and `MatchRegexp(\`metadata-refetch-status[^>]+aria-live="polite"\`)` in `internal/tmpl/a11y_render_test.go`. The old assertions passed even when the span had neither attribute (flash divs in books_show.html also carry those strings). The new assertions are scoped to the element under test. **Delete-confirm-fail (live-region role):** Temporarily removed `role="status"` from the `metadata-refetch-status` span in books_show.html → test FAILED: _Expected `metadata-refetch-status[^>]+role="status"`_. The old `ContainSubstring` would have PASSED (flash divs still had the attribute). Restored → test PASSES. ✓ **Delete-confirm-fail (live-region aria-live):** Removed `aria-live="polite"` from the span (leaving role intact) → test FAILED: _Expected `metadata-refetch-status[^>]+aria-live="polite"`_. Restored → test PASSES. ✓ **CI:** green ✓

zombor force-pushed bd-bookshelf-vqy9.19 from 2179607830 to 423fc0a721 2026-06-24 02:45:12 +00:00 Compare

zombor merged commit 1ed565621d into main 2026-06-24 02:56:24 +00:00

zombor referenced this pull request from a commit 2026-06-24 02:56:26 +00:00

Merge pull request 'fix(a11y): heading hierarchy, error page h1, cover-modal alt, live region (bookshelf-vqy9.19)' (#738) from bd-bookshelf-vqy9.19 into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!738

No description provided.