-
Notifications
You must be signed in to change notification settings - Fork 55
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(dropdown): fix mobile view and add backdrop #1027
Conversation
👋 Thanks for creating a pull request! 🚀 Checkout the storybook we've created for it: |
Since `gds-popover` is now modal, focus can't be outside until it is closed
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 is a very good approach. It mimics the behaviour that the native select element has! You have to close it before you do can click or tab to anything else.
One big thing that I feel that it's lacking right now is keyboard navigation!
Issues:
Action: When i open the dropdown and then press arrows, it scrolls the page.
Expected behaviour: I should move between list box items and select them if i press space or enter.
Action: When i open the dropdown and then tab i end up in the browser adress window
Expected behaviour: When dropdown is open tabbing should be disabled until I make a choice with space or close with escape (looking at select behaviour)
A11y
Regarding a11y behaviour I would look at select for general dropdown behaviour.
For multiselect I would have the same navigation but space should toggle selected items and I guess that you need to close it with escape to gain access to tabbing again?
For search I would look at datalist and just mimic their behaviour. I guess one question that I can think of is: Do you open the dropdown when being tabbed to and then focusing the input field or does the user need to open manually.
Otherwise great work as always :)
Looking at the React version with Core of this it seems to work better, maybe storybooks in PRs don't get updated with later commits? 😮 |
Need a better solution for this.
@vsjolander Yeah, I think the move to using the I've gone by MDNs recommendations for keyboard interactivity for ComboBox and Listbox, although some aspects still remains to be implemented. I'll have a look at native datalist for the search behaviour, sounds like a good idea to mimic that! |
@vsjolander This one is a little tricky, because we still need the tab-key in Also, I notice that One argument for keeping the default Either way, I feel like this is outside the scope of this PR (the main fix is to add backdrop in the mobile viewport), so maybe we can open a separate issue where this discussion can continue? Edit: For now, I agree that it's a bit akward to end up in the address bar when pressing tab, so I'm gonna go with a compromise:
|
Having it at least 1rem should prevent auto-zoomin on iOS
in mobile viewport
Related: #862
This PR corrects the mobile layout of the popover and adds a backdrop.
Since the mobile version is designed to work as a modal, I figured that the native modal
<dialog>
would be suitable here. It also gives us access to the:backdrop
pseudo element, and makes use of the#top-layer
stacking context.A caveat is that it makes the view structure of the popover different in mobile view, meaning that we have to rely on media query events to update the view accordingly, which feels a bit "hacky". But since it is a small and simple component (the
gds-popover
), I think we can live with it.Other options that could simplify it would be:
Use the modal approach for larger (desktop) view as well (not visually, only semantically). This would make the rest of the page inaccessible while the popover is open, forcing the user to abort or make a choice before they do anything else. Focus would be trapped in the popover.
Get rid of the mobile-specific layout and just use the regular popover in all viewports, and keep it always non-modal.
I'm not sure what the most a11y-friendly solution is. There are trade-offs either way. For now though, it should at least be an improvement over the current implementation, and we can always change it later.