-
Notifications
You must be signed in to change notification settings - Fork 355
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(Select/Dropdown): updated autofocus to false by default in v6 #11115
Conversation
Preview: https://patternfly-react-pr-11115.surge.sh A11y report: https://patternfly-react-pr-11115-a11y.surge.sh |
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.
Thanks for doing this update!
I have some feedback which can be done as a followup:
-
Noticed that all of the descriptions of Typeahead examples have duplicated sentences (in the
examples/Select.md
file), possibly caused when merging v5 to v6 -
Now after the change, the menu items are not easily accessible with keyboard. I know that's the reason why autofocus was introduced in the first place, and although the autofocus now defaulting to false is a great visual update, I think we should still support some kind of keyboard accessibility.
I made an update in this commit to the basic Select example to react on arrow up / down press, when the toggle is focused. (I don't know if it is making too many assumptions about what to do on arrow up/down event, but it is just an example). Here is an example of its behaviour:
Screen.Recording.2024-10-22.at.15.49.01.mov
It would require us to update all other examples though, but I think (given that e.g. the typeahead examples already have a lot of code in them) we can do that and make it a part of the Select (and Dropdown) Templates.
I know there is a followup to investigate, so let's say the commit could be one of the possible approaches (or maybe not and the logic should be encapsulated within the Select itself, this is just an idea)
As as addon to Adam's comment, we could also remove |
@adamviktora I think I did something similar locally at some point when tinkering around, though it was done as part of the base Menu/Select/Dropdown logic (rather than in each example code). May depend whether we would want to at least show a way consumers could handle focus management themselves in the mean time (since if a consumer opts to not autofocus for a non-typeahead, they'll have to do handle it themselves), or if we want to wait and see what potentially refactoring the logic internally may look like. @kmcfaul one issue with that may be that it depends where the menu is appended; with the document body appending then Tab may not go into the menu itself |
@thatblindgeye I prefer doing it as part of the base Menu logic. Consumers will probably expect it to be an out-of-the-box behaviour, so it will be better if it is part of the base component.
I prefer this, but it depends how long will it be, so we don't ship a half-working version now. But if the followup to fix it is a high priority, then I am absolutely ok with it. |
The current plan is:
|
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.
LGTM
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Towards #10393
We have a followup to investigate improving the focus logic/styling overall post-v6
Additional issues: