Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes accessibility issue when reading epubs #12183

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

nathanaelg16
Copy link
Contributor

Summary

These changes fix the issue for those using tabs to navigate through books using the epub viewer library. Previously, one could never reach the Right Navigation button due to the tabindex terminating with the epub viewer. This change sets the first tab item to be the epubviewer, then all other tab-accessible items will follow the DOM order.

BEFORE

before

AFTER

after

References

#10371

Reviewer guidance

  1. Open an epub book to read
  2. Press tab repeatedly until it reaches the Right navigation button
  3. Trigger the Right navigation button (press tab or enter)
  4. Press tab repeatedly again until it reaches the right navigation button

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

Copy link

ellipsis-dev bot commented May 17, 2024

Your free trial has expired. To keep using Ellipsis, sign up at https://app.ellipsis.dev for $20/seat/month or reach us at [email protected]

Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation seems correct to me! Thanks @nathanaelg16
Leaving final review to @radinamatic @pcenov after further accessibility QA is done!

@radinamatic
Copy link
Member

Thank you for diving into trying to fix this accessibility issue, @nathanaelg16!

However, positive tabIndex values are often flagged as errors, and are not the recommended way to approach the keyboard accessibility. I tested the asset from your PR on Firefox in Ubuntu 20.04, and while it may seem that it fixes the immediate issue of not being able to focus the right arrow for page navigation, this made me realize that we have deeper problems related to keyboard navigation for epub renderer that need some decisions first before trying to fix isolated issues. I'm almost leaning into thinking of the iframe as a sort of a modal window that should have the focus trap within for all the epub-specific features (page navigation, settings, search, etc.), and the defined way in- and out- of that epub-space, for the rest of the Kolibri features (folder, bookmarks, info, etc...)

But I digress... Bottom line is that I think we need some more general decision before approaching the fix for this specific issue. If this was more than one-off PR and you are interested to contribute more to improve the keyboard navigation in Kolibri epub renderer, please join our Kolibri #dev-community channel on Slack, where we can decide what would be the best approach. Thank you!

@nathanaelg16
Copy link
Contributor Author

Thank you @radinamatic for your feedback. I was debating whether to set the tabIndex to a positive or negative value, but the negative value didn't seem right either. I'd be happy to join the Slack channel and continue discussing this! Thanks

@MisRob
Copy link
Member

MisRob commented Jun 4, 2024

Thanks both. Just general FYI on the PR - this is already being discussed on Slack. Some decisions are needed and @radinamatic is planning to have a look at it at some point.

@MisRob
Copy link
Member

MisRob commented Sep 3, 2024

Leaving this PR open in case it's handy but note #10371 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: frontend DEV: renderers HTML5 apps, videos, exercises, etc. SIZE: very small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants