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 Introduction modal dialog a11y. #19607

Merged

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Jan 10, 2023

Context

Fixes the New Settings Introduction modal dialog accessibility.

Please note it's a while I'm not contributing to Yoast SEO, I'm not sure whether there's anything to do with stories and tests.

Summary

This PR can be summarized in the following changelog entry:

  • Improves accessibility of the Settings Introduction modal dialog.
  • [@yoast/ui-library] Uses forwardRef for the Modal component to get a references of the Dialog DOM node, necessary to move keyboard focus on it.

Relevant technical choices:

  • Hides content of the 'out of view' slides so that assistive technologies don't perceive extraneous content. Also avoids invisible tab stops.
    • Note: this is done by setting visibility: hidden on the inactive slides so that their dimension is preserved and the animation still works as expected.
  • Makes sure aria-describedby on the dialog only references one slide description at a time. Note: keeping Modal.Description is good as it automatically sets aria-describedby. We shouldn't remove it as done in DUPP-884-improve-the-discard-all-changes-dialog-a-11-y #19489. However, we should use it only for the active slide.
  • Uses forwardRef on the Modal component to allow passing a ref to the actual DOM element.
  • Avoids focus losses by moving focus to the modal dialog when pressing Deny, Next, and Back.
  • Changes the h2 headings to h1: the modal is the only perceivable content in the page so it worths a h1.
  • Hides the navigation three 'dots' from assistive technologies *this was read out as 'group', as it's a list with empty items).

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

  • Use a clean database or reset your user meta, specifically the one with meta key _yoast_settings_introduction
  • Go to Yoast SEO > Settings.
  • The introduction modal dialog opens.
  • Check initial focus is on the Play button (the preview image) and that there is a visible focus style.
  • Press Tab multiple times and check focus cycles only through visible elements in the current slide.
  • Tab to the play button (the image) and press Enter: the Deny/Allow permission panel appears.
  • Tab to Deny and press Enter: the Deny/Allow permission panel disappears and the Play button (image) appears again.
  • Check keyboard focus is on the modal wrapper: just press Tab and check focus goes to the Play button (image).
  • Press Enter: the Deny/Allow permission panel appears.
  • Tab to Allow and press Enter: the video loads and starts playing.
  • Tab to Next and press Enter: the next slide appears.
  • Check the sliding animation works as expected.
  • Check keyboard focus is on the modal wrapper: just press Tab and check focus goes to the Play button (image).
  • Check the video does not play automatically and the play button (image) is shown instead.
  • Play the video.
  • Tab to Back and press Enter: the previous slide appears.
  • Check the sliding animation works as expected.
  • Check keyboard focus is on the modal wrapper: just press Tab and check focus goes to the Play button (image).
  • Check the video does not play automatically and the play button (image) is shown instead.
  • Check the 'Skip' and 'Got it' buttons work as expected. After this, you may need to reset your user meta to make the modal dialog appear again (after refreshing the page).

Specific a11y checks to be tested by inspecting the source with the browser dev tools:

  • Check the modal dialog has an aria-label="Introduction to settings" attribute. Note: this was already fixed in DUPP-884-improve-the-discard-all-changes-dialog-a-11-y #19489
  • Check the modal dialog aria-describedby attribute references only one description ID at a time. When navigating to the next slide, the aria-describedby value updates to the ID of the next slide description.
  • Check the images within the play button have an alt="Play video" attribute.
  • Check the list with the 3 'dots' visually representing the navigation steps has an aria-hidden="true" attribute.

Relevant test scenarios

  • Changes should be tested with the browser console open
  • Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
  • Changes should be tested on different editors (Block/Classic/Elementor/other)
  • Changes should be tested on different browsers
  • Changes should be tested on multisite

Test instructions for QA when the code is in the RC

  • QA should use the same steps as above.

QA can test this PR by following these steps:

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

  • UI library

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Other environments

  • This PR also affects Shopify. I have added a changelog entry starting with [shopify-seo], added test instructions for Shopify and attached the Shopify label to this PR.

Documentation

  • I have written documentation for this change.

Quality assurance

  • I have tested this code to the best of my abilities
  • I have added unit tests to verify the code works as intended
  • If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
  • I have written this PR in accordance with my team's definition of done.

Innovation

  • No innovation project is applicable for this PR.
  • This PR falls under an innovation project. I have attached the innovation label and noted the work hours.

Fixes https://yoast.atlassian.net/browse/DUPP-900

@afercia afercia added changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog Yoast: Accessibility Yoast Feature labels Jan 10, 2023
@afercia afercia added this to the 20.0 milestone Jan 10, 2023
@afercia afercia requested a review from a team as a code owner January 10, 2023 15:16
@afercia
Copy link
Contributor Author

afercia commented Jan 11, 2023

TODO: even though the videos don't have audio content, they need transcripts to describe the visual information.

@marijnyoast
Copy link
Contributor

Thanks for creating this PR @afercia , highly appreciated!

@afercia
Copy link
Contributor Author

afercia commented Jan 16, 2023

YW. Last commit fixes a failing test by using displayName for the forwardRef Modal component.

@vraja-pro vraja-pro merged commit 2d11cb2 into release/20.0 Jan 17, 2023
@vraja-pro vraja-pro deleted the DUPP-900-introduction-modal-dialog-accessibility branch January 17, 2023 13:34
@vraja-pro vraja-pro restored the DUPP-900-introduction-modal-dialog-accessibility branch January 17, 2023 13:41
@vraja-pro
Copy link
Contributor

CR & AC ✅

@vraja-pro vraja-pro deleted the DUPP-900-introduction-modal-dialog-accessibility branch January 17, 2023 14:01
@vraja-pro vraja-pro restored the DUPP-900-introduction-modal-dialog-accessibility branch January 17, 2023 14:47
@vraja-pro vraja-pro deleted the DUPP-900-introduction-modal-dialog-accessibility branch January 17, 2023 15:04
@vraja-pro vraja-pro restored the DUPP-900-introduction-modal-dialog-accessibility branch January 17, 2023 15:06
@vraja-pro vraja-pro deleted the DUPP-900-introduction-modal-dialog-accessibility branch January 17, 2023 15:13
@vraja-pro vraja-pro restored the DUPP-900-introduction-modal-dialog-accessibility branch January 17, 2023 15:13
@vraja-pro vraja-pro deleted the DUPP-900-introduction-modal-dialog-accessibility branch January 17, 2023 15:19
d-claassen added a commit that referenced this pull request Jan 25, 2023
d-claassen added a commit that referenced this pull request Jan 30, 2023
This change was reverted, and was later introduced again in #19652.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog Yoast: Accessibility Yoast Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants