Learn searchbar a11y updates - #15267
Conversation
github-actions
Bot
added
APP: Learn
npm Package VersionsWarning The following packages have changed files but no version bump:
If these changes affect published code, consider bumping the version. |
🟡 Waiting for changesLast updated: 2026-09-14 14:00 UTC |
Build Artifacts
Smoke test screenshot |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15267 — the checkbox conversion and the opt-in skip target both do what the issue asks; Chrome's accessibility tree confirms the group + checkbox semantics and axe (AA) found no violations on the new markup.
One blocking bug: the index-based v-for key lets Vue skip the :checked DOM patch when entries reorders on toggle, so a pill can announce the opposite of its applied state — reproduced in a unit test and in live QA, including a pill left checked after "Clear all". Stable keys fix it.
CI: Frontend tests passing. Manual QA ran across mouse, keyboard, loading, RTL, 412px and axe AA; every other violation on those routes is pre-existing and outside the diff.
Findings inline: 1 blocking, 6 suggestions, 3 nitpicks.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| <legend class="visuallyhidden">{{ appliedFiltersGroupLabel$() }}</legend> | ||
| <label | ||
| v-for="(entry, index) in entries" | ||
| :key="`${entry.type}-${index}`" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: The index-based key desyncs the new checkbox's checked state from the applied filter.
It was harmless while the pill was a stateless KButton. Now the pill owns DOM state the browser mutates on click, and entries reorders on every toggle (applied filters are prepended). When a node is reused for a different entry, Vue 2's updateDOMProps compares the new :checked against the old vnode's value — not the DOM — and skips the write when they match, so the property the user just flipped is never corrected.
QA repro, from a clean /en/learn/#/library: check Explore, check Listen, uncheck Explore. Listen is the only applied filter, is painted active with a close icon, and is the pill you must activate to clear it — but its checkbox reports checked === false, and the a11y snapshot agrees ([checkbox] value="on", no checked state). Worst case is "Clear all": after checking Read (index 3) and clearing, the reused node under Reflect is still exposed as checked with nothing applied.
Also reproduced in a unit test on this branch:
● keeps checked in sync after unapplying the first of two applied activities
expect(element).toBeChecked()
Received element is not checked:
<input class="visuallyhidden" type="checkbox" />
Keying by the identity entries already computes makes it pass:
- v-for="(entry, index) in entries"
- :key="`${entry.type}-${index}`"
+ v-for="entry in entries"
+ :key="`${entry.termKey}:${entry.value}`"@click.prevent on the input also closes it, by keeping the DOM state purely derived from the binding — but the stable key is worth having either way (it also stops focus landing on a reused input whose label changed underneath it).
| // lay out as direct flex children of .filter-pills, same as before grouping. | ||
| .filters-fieldset { | ||
| all: unset; | ||
| display: contents; |
There was a problem hiding this comment.
suggestion: display: contents (and :focus-within on line 224) sit below the repo's declared browser floor.
packages/browserslist-config-kolibri/index.js declares Chrome >= 49, ChromeAndroid >= 49, Samsung >= 5, iOS >= 10. display: contents needs Chrome 65 / Samsung 9.2 / iOS 11.1; :focus-within needs Chrome 60 / Samsung 8. Neither is polyfillable by postcss.
Verified the fallback in-page (all: unset alone computes to display: block), measured at 900px:
| pill gaps | .filter-pills height |
"All filters" | |
|---|---|---|---|
display: contents |
8px | 82px | same row |
| fallback | 0px | 118px | own row |
The pill borders butt into one run-on strip. And with :focus-within unsupported, the focusable element is the 1px clipped input, so keyboard focus becomes invisible (WCAG 2.4.7) — KButton used :focus on the button itself, which has no floor.
Both alternatives avoid the property: put role="group" + aria-label on the existing .filter-pills flex container (it holds only filter controls), or keep the fieldset and give it the flex layout itself. Separately, display: contents on a grouping element is the case browsers historically dropped from the AX tree — it holds in Chrome (verified via CDP), but worth a Safari check, and getByRole('group') can't detect it since Testing Library never reads applied CSS.
| } | ||
|
|
||
| .filter-pills .pill { | ||
| display: inline-flex; |
There was a problem hiding this comment.
suggestion: Dropping KButton also dropped its .button base styles, and one of them was bounding the pill width.
.button (kolibri-design-system/lib/buttons-and-links/buttons.scss) supplied max-width: 100%; overflow: hidden; user-select: none and the colour transition. With white-space: nowrap kept but nothing bounding the width, a long keyword pill now pushes the whole document sideways. Measured at 412×915:
?keywords=how%20to%20prepare%20for%20the%20national%20mathematics%20exam
→ pill width 464px; documentElement.scrollWidth 488 vs clientWidth 412
The trailing close icon scrolls off with it, so the filter can only be removed via "Clear all". On develop the pill was clipped to the container. Re-adding max-width: 100%; overflow: hidden restores the bound.
Two smaller losses in the same row, visible next to the unchanged "All filters" KButton: pill text is now selectable (user-select: auto), and the rgba(0,0,0,.1) hover snaps in while its neighbour still fades. The label also renders 36px against "All filters"'s 38px — KButton wraps its icon in a 20px span.icon-container and the label renders the 18px <svg> directly, so min-height: 0 lets it shrink; min-height: 20px here brings them back level (verified in-page).
There was a problem hiding this comment.
Yeah, for long keywords, we don't handle overflow properly.
| type="checkbox" | ||
| class="visuallyhidden" | ||
| :checked="isFilterActive(entry.termKey, entry.value)" | ||
| :disabled="loading" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Disabling the checkbox that currently has focus blurs it, so every toggle drops focus to <body>.
Event log around one Space press:
["focusout INPUT:Explore[disabled] -> null", "change INPUT:Explore[disabled] checked=true"]
document.activeElement -> BODY
Focus never returns once loading clears, and because the checkbox is disabled at the moment its change fires, the new checked state isn't announced at all. The :disabled="loading" binding is pre-existing (develop's KButton had the same), but it's the thing preventing the announcement this PR exists to add. Leaving the input enabled and gating in toggleFilter, or restoring focus after the reload, avoids it.
Related, same mechanism: applying a filter re-scopes entries (11 checkboxes → 3 on a narrowed search) with nothing announced and no live region anywhere in LibraryPage. A polite live region carrying the new result count would cover both this and the equivalent silence after a search submit.
| // Override this by marking the element where the focus should land instead (e.g. | ||
| // the `main` landmark itself, especially if DOM content preceeds the first H1). | ||
| const target = | ||
| mainEl.querySelector('[data-skip-nav-target]') || mainEl.querySelector('h1') || mainEl; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The new fallback lands on the unlabelled #main wrapper, skipping the role="main" landmark that is its direct child.
On Learn Home (no h1 inside #main) the chain falls through to mainEl, which is AppBarPage's <div id="main" class="main-wrapper"> — no role, no accessible name. Measured after Tab → Enter on /en/learn/#/home:
active : DIV#main.main-wrapper tabindex="-1" role=null aria-label=null
landmarks: [{tag:"DIV", role:"main", parent:"main"}] <- direct child of #main
Focus moves (an improvement over develop's no-op) but nothing is announced and no ring is drawn. One selector fixes it:
mainEl.querySelector('[data-skip-nav-target]') ||
mainEl.querySelector('h1') ||
mainEl.querySelector('main, [role="main"]') ||
mainEl;Verified this disturbs nothing: Device, Facility and Coach have no main/[role=main] inside #main at all, so they keep landing on their <h1>.
Two more on this line: querySelector matches descendants only, so a page marking #main itself — the case the comment above offers as the example — silently falls through to the h1 (mainEl.matches('[data-skip-nav-target]') ? mainEl : … covers it). And this resolution order is the actual behaviour change in the PR, in a packages/kolibri component every page uses, and it has no spec — the two tests added to LibraryPage.spec.js assert that the template declares the attributes, so they pass unchanged if the order here regresses. It's plain DOM: render the component against a #main fixture, click, assert document.activeElement for each of the three branches.
| const { allFilters$ } = searchAndFilterStrings; | ||
| // for a11y, the pill is a semantic checkbox and label | ||
| // but visually styled to match KButton for sighted users | ||
| function pillPseudoStylesFor() { |
There was a problem hiding this comment.
nitpick: pillPseudoStylesFor() takes no arguments and returns the same object for every pill, so it's a computed, not a per-entry function — the For suffix invites a future caller to pass one. It's also invoked once per pill per render and $computedClass keys its cache on JSON.stringify(value), so the stringify runs each time. Only :focus-within needs to be dynamic; :hover, cursor and the loading opacity/pointer-events are static and belong in the scoped <style> block.
| v-if="iconAfterFor(entry)" | ||
| #iconAfter | ||
| > | ||
| <span class="link-text">{{ entry.label }}</span> |
There was a problem hiding this comment.
nitpick: .link-text styles nothing here — KDS scopes it as .link .link-text (buttons-and-links/buttons.scss:47) and this label has class pill. It carried over from KButton's internal markup.
The branch also newly owns this span, which makes dir="auto" possible for the first time: keyword pills carry the learner's own search text and language pills carry catalog lang_name values, neither of which goes through the translation layer.
| handleClickSkipLink() { | ||
| // Every page where this is supposed to work needs to have a top-level | ||
| // element with 'role' and 'id' attribute equal to 'main' and 'tabindex= -1'. | ||
| // element with 'role' and 'id' attribute equal to 'main'. |
There was a problem hiding this comment.
nitpick: "preceeds" → "precedes" (line 37). Also, no page sets role="main" on #main, so the role half of this contract has been untrue for a while — good moment to drop it. The one non-AppBarPage host, TopicsContentPage.vue, has it backwards: <div role="main" tabindex="-1"> with no id, so getElementById('main') returns null and the skip link there re-focuses itself (verified on /en/learn/#/topics/c/<id>). Pre-existing and outside this diff, but adding id="main" to that div would fix it, and with tabindex now set on the fly the hardcoded one becomes redundant.
| message: 'All filters', | ||
| context: 'Label for the button and side panel showing all available search filters', | ||
| }, | ||
| appliedFiltersGroupLabel: { |
There was a problem hiding this comment.
nitpick: The key and its context both call this the applied filters, but the fieldset wraps every offered pill — on a cleared Library that is 11 checkboxes, none applied. filtersGroupLabel matches what translators will see.
praise: the context itself is exactly the level of detail a translator needs — what the pills are, that the label is SR-only, and why it isn't visible.
…hem to be a semantic checkbox (that looks like a button) for a11y
…lows the itself to be the focus, but continues to default to the page H1. Adds this for the Learn > Library page since is ahead of the h1 in the DOM
764ee86 to
2ef07d6
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15267 — delta 764ee86..2ef07d6: 4/12 resolved, 1 acknowledged, 7 open, 1 new. CI green bar assets; QA did not run.
Prior-finding status
RESOLVED — Pills:19 — key
RESOLVED — Pills:19 — blur
RESOLVED — SkipNav:38 — landmark
RESOLVED — SkipNav:51 — offset
RESOLVED — Pills:14 — praise
ACKNOWLEDGED — LibraryPage:40 — landmark
UNADDRESSED — Pills:277 — display: contents
UNADDRESSED — Pills:281 — max-width
UNADDRESSED — SkipNav:51 — untested
UNADDRESSED — Pills:230 — pillPseudoStylesFor()
UNADDRESSED — Pills:27 — .link-text
UNADDRESSED — SkipNav:38 — "preceeds"
UNADDRESSED — searchAndFilterStrings:59 — label
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| // Need to set the tabindex attribute on the fly to get tab behavior | ||
| target.setAttribute('tabindex', -1); | ||
| // The fixed app bar would otherwise cover the target when it scrolls into view | ||
| const header = document.querySelector('.scrolling-header'); |
There was a problem hiding this comment.
suggestion: .scrolling-header is scoped to ScrollingHeader.vue:4; renaming it silently breaks this. Use $el.closest(), or appBarHeight (AppBarPage/index.vue:226).
| mainEl.focus(); | ||
| target.style.scrollMarginTop = `${header.offsetHeight}px`; | ||
| } | ||
| target.focus(); |
There was a problem hiding this comment.
suggestion: Still untested; LibraryPage.spec.js only checks template attributes.
| // lay out as direct flex children of .filter-pills, same as before grouping. | ||
| .filters-fieldset { | ||
| all: unset; | ||
| display: contents; |
There was a problem hiding this comment.
suggestion: Unchanged: display: contents and :focus-within (224) below the browser floor; KButton's max-width: 100% no longer bounds the pill (281).
| v-if="iconAfterFor(entry)" | ||
| #iconAfter | ||
| > | ||
| <span class="link-text">{{ entry.label }}</span> |
| #icon | ||
| <fieldset class="filters-fieldset"> | ||
| <legend class="visuallyhidden">{{ appliedFiltersGroupLabel$() }}</legend> | ||
| <label |
| } | ||
|
|
||
| .filter-pills .pill { | ||
| display: inline-flex; |
There was a problem hiding this comment.
Yeah, for long keywords, we don't handle overflow properly.
| header.focus(); | ||
| } else { | ||
| mainEl.focus(); | ||
| target.style.scrollMarginTop = `${header.offsetHeight}px`; |
There was a problem hiding this comment.
I don't see any difference with or without this 😅, what is it for?
3 participants
Summary
Addresses 2 of the 3 items in #15024
fieldsetmainelement itself can be focused with an aria-label. The specific case for this is so that on the Learn > Library page, the new search bar is not entirely skipped.role=searchto the search area, to make it easier to identify and find using landmarks for screenreader users in generalReferences
Partially fixes: #15024
Note the reason for breaking it apart is that I think managing the other search results screen output updates (in progress) would make this PR really large. Splitting will make the code review more manageable.
Reviewer guidance
QA team:
spacenot onenter. Is this okay? I would imagine the sighted keyboard user might figure this out, but it seems a bit... not ideal. However I think it might be the best we can do.skip to mainlands on the library page, and if the landmarks are navigable and well-labeled. (Happy to update strings and output further)skip to mainand ensure that on other pages, it hasn't landed us in an unlabeledmainarea without further context. I think since we retained the default behavior, this shouldn't happen, but some "spot-checking" would be helpful just to confirm in a few places.AI usage
Claude did the actual code generation, but highly guided by a very opinionated me of exactly what should happen and how it should happen. Comprehensive code review including tests, and manual a11y review using Mac Voiceover