Repository navigation
Conversation
Screen readers announced the raw slider value; they now read the same locale-formatted percentage shown next to the slider.
github-actions
Bot
added
DEV: renderers
|
👋 Hi @rishiraj38, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
📢✨ Before we assign a reviewer:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. 📎 We couldn't find an issue linked to this pull request. Please edit the description and link one under References, for example |
|
📢✨ Before we assign a reviewer, we'll turn on |
🟡 Waiting for changesLast updated: 2026-10-01 23:11 UTC |
Build Artifacts
Smoke test screenshot |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15431 meets both acceptance criteria: aria-valuetext reuses the visible label's $formatNumber call, and the spec asserts two values.
- suggestion: valuetext can disagree with the input's own value after a key step that lands on the current page (inline).
- nitpick: duplicated format expression (inline).
CI pending. Manual QA (page turns, Home/End/arrows, TOC/search jumps, resume, ar/fa/fr-fr): valuetext matched the visible label except in the case above. axe showed no slider violations. Its 2 other violations predate this PR.
@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
When a slider change lands the book on the page already shown, the range input keeps the stepped value but sliderValue does not change, so the input is not re-rendered and the thumb disagrees with the label and aria-valuetext. Count relocations in the renderer and pass the count to the bottom bar, which sets the input from sliderValue whenever the count changes. The input keeps the user's value while the jump is in flight. Also move the percentage formatting into a computed so the visible label and aria-valuetext share it.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15431: both prior findings resolved. No new findings. CI pending.
Prior-finding status
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/BottomBar.vue — valuetext disagrees with input value after a no-op key step
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/BottomBar.vue — duplicated format expression
@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
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
Changes make sense, will ask QA to verify the fix.
|
Thank you @rishiraj38 , good work, ebook reader progress bar now has the proper 
|
radinamatic
left a comment
radinamatic
left a comment
There was a problem hiding this comment.
Manual QA passes, good to go! 💯 
🚀
|
Thanks @rtibbles for the reviews and @radinamatic for testing it out. |
Summary
The EPUB reader's progress slider had no
aria-valuetext, so screen readers announced the raw range value (for example "23") instead of a reading position. This bindsaria-valuetextto the same locale-formatted percentage shown beside the slider, through a sharedprogressTextcomputed, so no new translation string is needed.It also fixes a related mismatch found in review: when a slider change landed the book on the page already shown, the range input kept the stepped value while
sliderValuedid not change, so the thumb and the announced percentage disagreed. The renderer now counts relocations and passes the count to the bottom bar, which sets the input fromsliderValuewhenever the count changes. The input keeps the user's value while the jump is in flight and is put back in step once the relocation settles, without recreating the element, so keyboard focus stays on the slider.References
Fixes #14623
Reviewer guidance
BottomBar.spec.jsassertsaria-valuetextforsliderValue23 ("23%") and 100 ("100%"), and, rendered through a parent as inEpubRendererIndex, that a changed input keeps its value untilrelocationCountchanges, is reset tosliderValuewhen the relocation leavessliderValueunchanged, and moves to a newsliderValuewhen the book moves. The reset case fails without the watcher.EpubRendererIndex.spec.jschecksrelocatedHandlerbumpsrelocationCounton every call, including one that leavessliderValueunchanged. All 88 epub_viewer Jest tests pass.AI usage
I used Claude Code to add the binding, prompting it to reuse the
$formatNumbercall the visible label already uses so screen readers announce the same value sighted users see. I had it write the failing spec cases first, reviewed them, and verified they pass with the change along with the epub_viewer tests and the prek hooks. I also used it to work through the review comments: tracing why the input kept its stepped value, adding the relocation count and its specs, which I reviewed and ran.