-
Notifications
You must be signed in to change notification settings - Fork 44
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
Change focus management in disclosure utility component #284
Conversation
π¦ Changeset detectedLatest commit: ba40988 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
53311b3
to
0922979
Compare
Thank you for your PR! Can you please add some more details to the PR, explaining why you want to make this change, and what the intent is. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alex-ju I've found an issue with Safari that I suspect is a blocker: when the dropdown is activated/opened via keyboard (tab to the toggle, open the toggle via spacebar/enter) everything works as expected; but if the dropdown is opened via mouse click, then nor the click outside nor the "esc" key close the dropdown.
other things I notice, that may be helpful in narrowing down the problem:
- if you add a
console.log
inside theonFocusOut
function, you will see that is logged in Chrome (when the toggle is clicked to open it) but not in Safari - in Safari, if you open the toggle via mouse click, but then hit
tab
and focus on the first item, then the click outside and the esc both trigger the close
I did some tests, but I am not sure what would be the cause root, if something in the logic or a bug in Safari. I'll let you investigate.
packages/components/tests/dummy/app/templates/utilities/disclosure.hbs
Outdated
Show resolved
Hide resolved
Sure, I should've been more clear on the why; I guess part of it was left in the comment on the issue. I have added a paragraph to the PR description trying to articulate the motivation behind the proposed change. |
Ah, interesting, it seems to be a bug in webkit, but I'll investigate and add a patch in a separate commit. |
@didoo I've added a patch for the inconsistency of WebKit not triggering |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good for me π
Is this a proposal or a foregone conclusion? I think I missed that part of the conversation entirely. Any documents that you can point me to? I don't think we should change the component without appropriate justification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: After looking through it, I really think we are losing some A11y-specific UX here, but it's also technically a WCAG-conformant solution. As such, I disagree with this change but cannot object to it.
Thank you for the review, Melanie. From an accessibility perspective, my concern with the current implementation is that assistive tech users are not being told that they've entered a region when they moved into the disclosed content and they're not being told that if they want to get out of the region they can use the escape key (some may figure it out, but it's not explicit), which may trap and frustrate some of them. That being said, we'll most probably need this focus-trap-based solution for the modal dialogue component. |
To avoid focus trapping and preserve the requirement that a click (or tap or tab) outside of the disclosure will close/deactivate the content
Preserve the requirements that Escape key should close/deactivate the content and when used, focus should return to button/toggle.
7e3152a
to
207a127
Compare
Our linter doesn't allow interactive events on standard elements complaining about 'keyup' being used on the main component element. As our disclosure component is interactive (jn a sense that expands and collapses a container) we need to add this exception to the template.
We explicitly test the functionality we introduced with this component, which is handling the escape key and the focus out. As a note, these two tests could be grouped under a 'when disclosure content is visible', but I would avoid introducing nested tests as we don't seem to have them anywhere else and we don't have many tests for this component anyway.
Webkit doesn't give a `focus` state to buttons when clicked (a behaviour that is different from all the other vendors; see https://bugs.webkit.org/show_bug.cgi?id=22261). To overcome this difference in behaviour we apply a focus state to the buttons used to toggle the disclosed content. Fixup! Patch button focus when clicked WebKit
207a127
to
ba40988
Compare
7f4b9b2
to
ba40988
Compare
π Summary
This PR is proposing an alternative approach to focus management in our disclosure utility component on the back of #280
π οΈ Detailed description
The main change is dropping the focus-trap mechanism and letting the focus flow through the component while preserving the rest of the requirements defined in the associated CRD. The solution is meant to avoid any change the API of the component.
I propose this change to fix the issues described in #280 (briefly: "fix focus-trap bug when the interactive opens a modal", and the "scrolling caused by focus returning to the previously active element"). It also addresses the WCAG 2.1 success criterion 2.1.2 No Keyboard Trap (Level A) by allowing the users to exit the disclosed content using the default keyboard shortcut (tab).
πΈ Screenshots
Note: the colors went way off on the above compressed gifs π±
π External links
Review of focus management in Disclosure (+ Dropdown/Breadcrumb) components
Fixes #267 and https://github.com/hashicorp/cloud-ui/pull/2362#issuecomment-1097778784
π How to review
π Review commit-by-commit
Reviewer's checklist:
π¬ Please consider using conventional comments when reviewing this PR.