fix(a11y): add aria-label to unlabelled sort-order selects (bookshelf-vqy9.15) #736
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-vqy9.15"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Fixes audit findings S-3 and S-4 from the vqy9.2 accessibility audit (WCAG 1.3.1 / 4.1.2 Level A):
aria-label="Sort direction"added to the Asc/Desc order<select>inseries_index_controls.htmlandauthors_index_controls.html. Previously screen readers announced only "Ascending combo box" or "combo box" with no context.aria-label="Add sort field"added to<select class="sort-add-select">inbooks_index_controls.html. Previously had no accessible name at all.No CSS changes; no inline
style=added; canonical classes unchanged. Visual layout is unaffected.Test plan
Describe("ListHandler HTML: sort direction select has aria-label")ininternal/series/handler_test.go— asserts the rendered HTML containsaria-label="Sort direction".internal/authors/handler_test.go.Describe("ListHandler HTML: sort-add select has aria-label")ininternal/books/handler_test.go— assertsaria-label="Add sort field"is present.make testgreen (all 3 new It-blocks pass).npm testgreen (JS gate unchanged).Closes bead bookshelf-vqy9.15 on merge.
CODE REVIEW: NOT APPROVED
Phase 0: DEMO Verification
No runnable CLI DEMO block exists in the bead comments. The implementer posted two rendered screenshots as the visual proof. Screenshots were downloaded and inspected. The screenshots actually expose the BLOCKER rather than verifying correctness: the seeded scenario (owned #4 and #6 in an 8-book series) shows ghost cards only for #5, #7, #8 — entries #1, #2, #3 are entirely absent from both the grid and the book-list tab. The heading shows "OWN 2 OF 8" confirming series_total=8 is known, so gaps #1–#3 should be visible but are not.
Phase 1 & 2: Findings
[BLOCKER] internal/series/series.go:134-138 — computeMissingEntries starts at minOwned, not 1, when seriesTotal > maxOwned
The docstring at line 40 explicitly states the totalKnown strategy is "gaps in 1..seriesTotal". The implementation sets rangeMin = minOwned unconditionally (line 134) and only extends rangeMax to seriesTotal (lines 136–138). When seriesTotal > maxOwned, the loop runs minOwned..seriesTotal — so any entry before the first owned book is silently dropped.
Concrete example: series_total=8, owned={#4, #6}. Expected missing: #1,#2,#3,#5,#7,#8. Actual: #5,#7,#8. The screenshots confirm this exact bug — #1, #2, #3 are invisible. The test at line 340 ("returns interior gaps and tail when seriesTotal > maxOwned") uses ownedNumbers=[1,3] where minOwned=1, so it cannot catch this.
Fix: when seriesTotal > maxOwned, set rangeMin = 1 instead of minOwned. Also add a test: ownedNumbers=[3,5], seriesTotal=7 → expected missing [1,2,4,6,7].
[MAJOR] internal/series/service.go:243-250 — buildSeriesEntries sort comparator violates strict-weak-ordering for two owned books at the same series_number
The comparator at line 248:
return !entries[i].Missing. When both i and j are owned books (Missing=false) at the same series_number, less(i,j) = true AND less(j,i) = true — both claim to be less than each other. This violates antisymmetry. Go's sort.SliceStable won't panic but produces undefined ordering for that pair on every page render (non-deterministic display for users with duplicate series numbers in their metadata).Fix: add a stable tiebreaker — when ni == nj and neither is missing, tiebreak on book ID:
entries[i].Book.ID < entries[j].Book.ID. Guard the nil-Book case (ghost entries have Book=nil).[MINOR] internal/series/service.go:170 — TotalKnown set true whenever seriesTotal > 0, even when seriesTotal <= maxOwned
totalKnown := seriesTotal > 0fires regardless of whether seriesTotal is actually used as the range ceiling (which only happens when seriesTotal > maxOwned). When a user owns books numbered past seriesTotal (e.g., own #1 and #5, seriesTotal=3), the template renders "own 2 of 3" while ghost cards show gaps through #5. The docstring at line 41 says totalKnown means "seriesTotal > 0 AND > max owned", but the code doesn't match.Fix:
totalKnown := seriesTotal > 0 && seriesTotal > maxOwned(maxOwned is already available in buildSeriesDetail at that point).REVIEW VERDICT: 1 blocker, 1 major, 1 minor
Each aria-label test previously injected a hardcoded inline stub that already contained the aria-label under test, making the assertion tautological — removing the aria-label from the production template would not cause the test to fail. Fix: use os.ReadFile("../../templates/pages/<domain>_index_controls.html") to load the real file bytes into the MapFS, so the test exercises the actual production template. Verified: removing aria-label="Sort direction" from series_index_controls.html causes the series test to FAIL. Also align books test error-capture style with series/authors (remove unused respErr variable, use consistent getErr/readErr naming).Fixed the tautological aria-label tests (comment 8967).
What changed: Each test now loads the real production controls template via
os.ReadFile("../../templates/pages/<domain>_index_controls.html")instead of injecting a hardcoded inline stub that already contained the aria-label under test.Proven with teeth: Temporarily removed
aria-label="Sort direction"fromtemplates/pages/series_index_controls.htmland confirmed the series test FAILS — the output renders the full<select name="order" ...>without the aria-label, and Gomega correctly reports the string is absent. Restored the template and all tests pass.MINOR (style): Also removed the unused
respErrvariable from the books test and aligned error-capture naming (getErr/readErr) to match the series/authors pattern.CI green on
dd002190.dd002190b92676aea326