Skip to content

Conversation

@cdrini
Copy link
Contributor

@cdrini cdrini commented Jan 8, 2026

QA: https://deploy-preview-1480--ia-bookreader.netlify.app/bookreaderdemo/demo-internetarchive?ocaid=theworksofplato01platiala#page/198/mode/2up

Fixes 3 points from the audit:

  • Inappropriate label "Toggle fullscreen" is specified for the "Enter fullscreen / Exit fullscreen" button
  • On activating "Flip left" and "Flip right" buttons present in the "Bookreader Item Preview", the current page and the total number of pages gets updated visually but not aurally
  • The slider is not labelled with role=slider
  • Sidebar buttons missing aria-expanded ; will include once it's merged: Add aria-expanded to BR sidebar menus iaux-item-navigator#24

And a few other small points:

  • Screenreaders were having difficulty reading the text layer, because they were ignoring the elements that contained the spaces! Implemented a bit of hack there, but it worked well in NVDA; would be nice to test JAWS.

A few considerations:

  • I had to rejigger the slider quite a bit to make it report updates at the correct time and avoid some odd behaviour. This:
    • Fixed an issue where if you focussed the slider and pressed/held right, it would jitter and behave oddly, but
    • Introduce an odd experience where the keyboard shortcuts when the slider has focus are different than the shortcuts when it doesn't. Without focus, left/right goes back/forward, and up/down goes back/forward, and pgup/pgdown move by a single page. But since the slider is now handling its keyboard shortcuts, up/down move in the opposite direction, and pgup/pgdown move in jumps. Note sure which is better here, but I think it could be confusing if a user click the slider, is then given focus, and then the keyboard shortcuts change.
      • Discussed with @pezvi , and we decided removing up/down to flip was the best option. Folks can still use left/right of course! And this also frees up/down for scrolling across all modes.
  • In order to get the page change announcement working, I did a bigger rejiggering of the jumpToIndex method to take an optional ariaLive parameter so that we can decide in what contexts we do indeed want the new page to be announced. Otherwise the page change was announced/interrupted the read aloud experience, and also was duplicated when using the slider. This resulted in a bigger refactor with a BREAKING CHANGE to the jumpToIndex method's interface, from:

BookReader.jumpToIndex(index: PageIndex, pageX = 0, pageY = 0, noAnimate = false) to BookReader.jumpToIndex(indexOrDirection: PageIndex | 'left' | 'right' | 'next' | 'prev', {pageX = 0, pageY = 0, noAnimate = false, flipSpeed = null, ariaLive = false} = {}

These other parameters weren't really functional / used, so I don't think it'll break any usecases.

@codecov
Copy link

codecov bot commented Jan 8, 2026

Codecov Report

❌ Patch coverage is 58.51064% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.71%. Comparing base (fefbb8a) to head (80e3c45).

Files with missing lines Patch % Lines
src/BookReader/Navbar/Navbar.js 48.14% 14 Missing ⚠️
src/BookReader/ModeAbstract.js 47.05% 9 Missing ⚠️
src/BookReader.js 74.07% 7 Missing ⚠️
src/BookReader/Mode2UpLit.js 28.57% 5 Missing ⚠️
src/plugins/plugin.chapters.js 0.00% 2 Missing ⚠️
src/BookReader/Mode2Up.js 85.71% 1 Missing ⚠️
src/BookReader/ModeThumb.js 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1480      +/-   ##
==========================================
+ Coverage   69.43%   69.71%   +0.28%     
==========================================
  Files          62       63       +1     
  Lines        5330     5353      +23     
  Branches     1156     1172      +16     
==========================================
+ Hits         3701     3732      +31     
+ Misses       1594     1588       -6     
+ Partials       35       33       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This gets confusing when combined with slider focus, where the shortcut does potentially the opposite of what it appears to do.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant