-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Changed Drop to add onFocusOutside #6078
base: master
Are you sure you want to change the base?
Conversation
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.
I think this PR is summarizing well the expected inline behavior of drop within the Select. And though it doesn't match the html select accessibility behavior 101, I do see how it significantly improves the axe ux of the component.
I would say though that the users that have concerns about the current accessibility behavior of Select, will need to fix the component locally and manually (by adding the inline prop to the Select's drop props) instead of getting the component fixed out of the box from the library (unless we can change Select to default to inline drop but that may not be backward compatible). IMHO, the default Select behavior should behave correctly without the additional prop, so maybe we can think about how to allow that.
A quick bug from running the Select "All" example:
If nothing is selected, the drop opens using Space, and the user presses Tab, the focus moves to the next interactive element as expected.
Though, if the Select does have an option 'Two' selected and drop was just opened using Space, the user will need to press Tab twice in order to exit the drop and move to the next interactive element.
Similarly, if the selected option is 'One', the user will only need to Tab once to exit the Select, which overall makes the behavior inconsistent.
THANK YOU! and I'll take a deeper look into the functionality tomorrow.
As I noted in the description above:
So, I am fully anticipating a conversation on "when" to do this and "how" to make it a default. I was really struck by the comments that different browsers have different default behavior for the native element. Firefox's default behavior for Thoughts? |
I'm summarizing some thoughts from our conversation today so the team can chime in as well. Since the select's native behavior varies between browsers, we should be mindful to pick a default tab behavior per user's browser. Since we want the users to have consistent behavior of different instances of Select across the app, we would leverage the theming capabilities to state the desired tab behavior as follows:
Potential values for
I think we can start with the options above, and enhance them as the need arises. Few more points to consider:
Please let us know what you think. |
I think if we are defaulting the theme to the native browser approach and aligning the tab behavior to how the browser would handle a native html select, I see this as a fix. We have done previous accessibility work aligning our components with how the native html component is working so I don't see a backwards compatibility issue with this. I see it as fixing the component to align with how the browser would handle the native html version.
Yes I do think we should allow the theme to control the overall drop behavior for the application, not just the Select component. To me it makes sense to set the tab behavior for drop's across the whole application. |
What does this PR do?
Changed Drop to add onFocusOutside.
This allows callers to combine
dropProps: { inline: true, trapFocus: false, onFocusOutside: () => {} }
to get Firefox-like behavior for a Drop. Namely, when the Drop is open and the user changes the focus to an element outside the Drop, the Drop can be closed.For this pull request, the Select in the AllComponents story has been changed to specify
dropProps={{ inline: true, trapFocus: false }}
and DropButton detects theinline
and automatically wires up theonFocusOutside
to close the Drop.This is meant to explore correctness of the behavior. I expect that the exact API signature and any theme configuration will need additional contemplation.
Where should the reviewer start?
Interact with the Select component in the All Components story to test the behavior.
Then, take a look at DropButton, then inside DropContainer for the real meat.
What testing has been done on this PR?
All Components story
How should this be manually tested?
All Components story
Do Jest tests follow these best practices?
no unit tests yet, tricky where focus is concerned
What are the relevant issues?
#5896
Do the grommet docs need to be updated?
yes
Should this PR be mentioned in the release notes?
yes
Is this change backwards compatible or is it a breaking change?
backwards compatible