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

Fix page changes not being announced by assistive technology when navigating using the client-side router #5288

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

JeffersonBledsoe
Copy link
Member

@JeffersonBledsoe JeffersonBledsoe commented Oct 7, 2023

Create a mutation observer on client-side load that tracks when the <title> element contents changes and updates an aria live region to announce the page change. Needs cross-browser and cross-screen reader testing

Fixes #4724

TODO

  • How should we handle focus after page change?
  • History API isn't listened to.

@netlify
Copy link

netlify bot commented Oct 7, 2023

Deploy Preview for volto ready!

Name Link
🔨 Latest commit 317d5d7
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/65216b58341e5600089c34e6
😎 Deploy Preview https://deploy-preview-5288--volto.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@JeffersonBledsoe JeffersonBledsoe changed the title WIP: title change announcements using MutationObserver (needs testing) Fix page changes not being announced by assistive technology when navigating using the client-side router Feb 14, 2024
Copy link

netlify bot commented Jul 2, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit f284734
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/67115631676f4000086e5cdf

@JeffersonBledsoe JeffersonBledsoe marked this pull request as ready for review July 2, 2024 10:33
@JeffersonBledsoe
Copy link
Member Author

@plone/volto-accessibility Could we test this / discuss the implementation?

@ichim-david
Copy link
Member

hey @JeffersonBledsoe I don't have time until the wk busy with some bugs :( I added a calendar entry so I don't forget about it :)

@ichim-david ichim-david added the 99 tag: UX Accessibility Accessibility issues label Jul 21, 2024
@ichim-david ichim-david self-requested a review July 21, 2024 07:01
@JeffersonBledsoe JeffersonBledsoe self-assigned this Sep 24, 2024
@ichim-david
Copy link
Member

Status report on testing the feature as part of the Salamina sprint:

  1. We've tested https://volto.demo.plone.org/ navigating different urls using the keyboard
    and notice the lack of announcement that we have changed url.
    This was using VoiceOver on Mac and NVDA on Windows
  2. We've tested the change on Windows and Mac and heard the announcement of the title
    which worked until we played with the browser history navigation that Jeff handled today in
    86d5b6b
  3. We tested https://www.gov.uk where we heard the location change although it was challenging at times
    due to the combination of browsers or screen reader technology, it was easier to hear on NVDA then VoiceOver

Tomorrow we will hopefully talk with the A11y expert that Red Turtle has asked to participate and hopefully get some
feedback also from him on this proposal.

@JeffersonBledsoe
Copy link
Member Author

After further discussion with an accessibility consultant at the Salamina sprint, it was concluded that the current behaviour is acceptable. Only with more user testing can we concretely decide what to do with focus and so leaving the focus where it is on page change is an acceptable behaviour for now.

@JeffersonBledsoe
Copy link
Member Author

@ichim-david This is finally ready! Not sure if we need review from a non-a11y perspective, I'll leave that up to you

Copy link
Member

@ichim-david ichim-david left a comment

Choose a reason for hiding this comment

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

@JeffersonBledsoe you have locking changes that doesn't belong to this pull request and you also set it.only on locking.js file

@ichim-david
Copy link
Member

@JeffersonBledsoe you have locking changes that doesn't belong to this pull request and you also set it.only on locking.js file

@JeffersonBledsoe you probably needed to change the selector to be more specific, but it.only shouldn't be there still

@JeffersonBledsoe
Copy link
Member Author

Good catch on the .only! The other changes are relevant. There are multiple alert on the page at times now so the tests that assumed there was only one need updated

@JeffersonBledsoe
Copy link
Member Author

JeffersonBledsoe commented Sep 27, 2024

@ichim-david I applied the fix we discussed at the sprint. It doesn't seem to impact anything and the new fixed behaviour is good but is a good safeguard anyway incase somebody does end up with focus on it somehow

observer.disconnect();
window.removeEventListener('popstate', handlePop);
};
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

By just looking at the code I would say that eslint should complain that this array should at least contain the updatePage function. Would it be possible to move it inside the useEffect next to handlePop?

Copy link
Member Author

Choose a reason for hiding this comment

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

@pnicolli The linters don't seem to complain, but I've moved it within regardless

@ichim-david ichim-david requested a review from a team October 15, 2024 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
99 tag: UX Accessibility Accessibility issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Page changes are not announced over Screen reader (WCAG 2.4.2 failure)
4 participants