-
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
"Dropdown" component - Fix issues with focus - Part 1 #259
Conversation
…pdown::ListItem::Interactive`
🦋 Changeset detectedLatest commit: c2a8f70 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 ↗︎
|
This fixes the issue with the first item being in a strange state in Chrome.
@MelSumner can you have a look at this PR? Thanks |
@didoo This appears to resolve all of the issues I noted in #258. I did see a new issue popup in Firefox (but not in Chrome) though. I was able to tab to the ToggleButton, open the menu, and then tab to the first list item using the keyboard, but I couldn't tab to any other items in the list. Focus remained on that first list item. |
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.
This seems to resolve the concerns we were having previously. Tested on macOS with Chrome, Safari and Firefox and was able to confirm that we are seeing what we expect.
I am not entirely sure about the shouldSelfFocus
change in addition to the CSS change (I was expecting one or the other but probably not both) but I am not concerned enough to object at this time.
Leaving a comment to check my understanding. With this PR we're trying to address the 3 issues flagged in #258 as follows:
Do we need a changelog entry for these fixes? |
I checked in my local env and it works for me (in Firefox). Maybe you have to enable the tabbing on all the "control" elements, as per @MelSumner suggested link ? |
Yes, just pushed one. |
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.
@MelSumner's suggested fix worked to resolve what I was seeing in Firefox. I'm seeing things as expected now. Thanks.
📌 Summary
This is a possible fix for part of the problems related to focus states in
Dropdown::ListItem::Interactive
(see #258).There will be a second round of fix, in which we want to review all the focus management for the components that rely on the
Disclosure
component (with the intent to fix also the bug seen in https://github.com/hashicorp/cloud-ui/pull/2362#issuecomment-1097909966).🛠️ Detailed description
In this PR I have:
active+focus
states🔗 Links
👀 How to review
👉 Review by files changed
Reviewer's checklist:
💬 Please consider using conventional comments when reviewing this PR.