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

Changed Drop to add onFocusOutside #6078

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ericsoderberghp
Copy link
Member

@ericsoderberghp ericsoderberghp commented Apr 20, 2022

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 the inline and automatically wires up the onFocusOutside 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

@ericsoderberghp ericsoderberghp marked this pull request as draft April 20, 2022 00:20
Copy link
Member

@ShimiSun ShimiSun left a 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.

src/js/components/Drop/DropContainer.js Show resolved Hide resolved
src/js/components/Drop/index.d.ts Show resolved Hide resolved
src/js/components/DropButton/DropButton.js Show resolved Hide resolved
@ericsoderberghp
Copy link
Member Author

As I noted in the description above:

This is meant to explore correctness of the behavior. I expect that the exact API signature and any theme configuration will need additional contemplation.

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 <select /> is as this pull request does it, where tab closes the drop and moves to the next focusable inline element. I was also compelled by the comment about the un-naturalness of not matching the default browser behavior. I'm wondering if we should offer two fundamental modes for grommet's Select that align with the browser default behaviors and then allow a theme configuration that indicates whether the default should be a specific one or whether it should match the browser's default behavior for the native element. It would be easy to cover the top browsers and default to one of the modes for all others.

Thoughts?

@ShimiSun
Copy link
Member

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.
Enabling inline will allow us to support browsers such as FF.

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:

   select: {
      keyboard: {
        tab: <value>,
      },
      ...
   }

Potential values for <value>:

  1. "native" - would default to the user's browser behavior.
  2. "closeAndForward" - would close the drop and move up to the next interactive element (inline will probably need to be injected into the drop).
  3. "ignore" - would ignore the tab when the Select is opened (similar to Chrome native select that swallows the tab event).

I think we can start with the options above, and enhance them as the need arises.

Few more points to consider:

  1. Grommet would like to provide a Select that will have a reasonable tab behavior out of the box, without additional configuration, one way to do it is to default the theme to the "native" approach. Can you think of any backward compatibility issues that this change may result in?
  2. Should we support tab configuration up high on the Drop component theming, so it can allow consistency between different grommet components that uses drop (such as Select, DateInput, Menu...). This alternative will be opt-in since it may cause backward compatibility issues when changing its defaults. But I'm wondering what are your thoughts on this.

Please let us know what you think.

@jcfilben
Copy link
Collaborator

  1. Grommet would like to provide a Select that will have a reasonable tab behavior out of the box, without additional configuration, one way to do it is to default the theme to the "native" approach. Can you think of any backward compatibility issues that this change may result in?

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.

  1. Should we support tab configuration up high on the Drop component theming, so it can allow consistency between different grommet components that uses drop (such as Select, DateInput, Menu...). This alternative will be opt-in since it may cause backward compatibility issues when changing its defaults. But I'm wondering what are your thoughts on this.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants