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

Update side panel focus trapping #9408

Conversation

marcellamaki
Copy link
Member

Summary

This PR uses a slightly modified version of the existing FocusTrap pattern's functions to ensure the focus shifts to the side panel correctly within the Content display and that a user can tab through each element of the side panel.

References

Fixes #9178
Fixes #9096
Fixes #9175
Possibly fixes #9193 ? (A few extra tabs are required vs. in Chrome and Safari, but all elements are accessible via tab key when correct settings are enabled... I will see if I can continue to improve this in follow up, but I think the initial improvements are at least a step in the right direction since the content is now available even if it is not the best experience)

Reviewer guidance

Use the tab key to navigate within various pieces of content (videos, exercises, etc.)
Opening the side panel to view information or to view additional resources should:

  • shift the focus to the side panel
  • cycle through the focusable elements - including links or buttons as well as the main close button
  • should work in Safari and Chrome. Improvements tbd in Firefox...

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

@marcellamaki marcellamaki added TODO: needs review Waiting for review TAG: a11y Affecting accessibility labels May 5, 2022
@radinamatic
Copy link
Member

Definitely a step in the right direction! 💯

Tested in Firefox and Chrome on Linux, it's possible to Tab-navigate correctly in and out the side panel, and focus & interact with all the elements except downloading the resources: the Download button itself gets the focus, and opens the available options to download, but those options (resources) cannot be selected (focused) by keyboard.

I'd say it's OK to deal with the additional Tab-presses in Firefox in the following PR, but let's fix this last issue before we merge. Thank you!

Was it intentional that the focus outline is rectangular in Chrome but rounded in Firefox? 🤔

@marcellamaki
Copy link
Member Author

Good catch @radinamatic -- thank you! I will look into the download button issue.

As for the focus outline... it was not intentional, but I also didn't try to ensure they were the same 😄

@marcellamaki
Copy link
Member Author

@radinamatic -- past Marcella has come back to haunt me. There is an open issue in KDS that I created, assigned to myself, and didn't finish, to address this very problem 🤦‍♀️ I will return to that PR to try to wrap it up and then after we release an updated KDS, if all goes well, hopefully that will help me fix this here.

Copy link
Contributor

@sairina sairina left a comment

Choose a reason for hiding this comment

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

Code looks good. Manual testing needs to be done to be sure.

@radinamatic
Copy link
Member

Ok, if I understand correctly, the unfocusability of the options in KDropdown is tracked in KDS (I just upped the priority 😈), so we can approve and merge this.

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

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

Manual QA passes, pending issue to be resolved in KDS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TAG: a11y Affecting accessibility TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants