ItsJonQ / g2

✨ An experimental reimagining of WordPress components
http://g2-components.com/
MIT License
105 stars 12 forks source link

SelectDropdown: Re-enable Portal support #250

Closed ItsJonQ closed 3 years ago

ItsJonQ commented 3 years ago
Screen Shot 2021-01-29 at 3 10 09 PM

This update re-enables Portal support for the SelectDropdown component. This was previous refactored out to fix the a11y issues.

This update has been retested with voiceover + Chrome/Firefox to ensure that focus is working correctly.

Screenshot of FontSizeControl collapsed in Firefox:

Screen Shot 2021-01-29 at 3 11 34 PM

Screenshot of FontSizeControl opened in Firefox:

Screen Shot 2021-01-29 at 3 10 51 PM

Story for testing: https://g2-git-update-select-dropdown-portal.itsjonq.vercel.app/iframe.html?id=components-selectdropdown--preview&viewMode=story

vercel[bot] commented 3 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/itsjonq/g2/ezut6sepq
✅ Preview: https://g2-git-update-select-dropdown-portal.itsjonq.vercel.app

ItsJonQ commented 3 years ago
Screen Shot 2021-02-01 at 9 17 12 AM

@diegohaz Haiii ❤️!! We're noticing some oddities with Safari + Voiceover 🤔

The voiceover highlight box doesn't seem to surround the correct UI. It works okay in Firefox and Chrome.

I think this happens regardless of whether or not we use Portal.

Functionally, I think it's working.

Would you have any insight/experience with this for Safari?

Thank you!

The UI in the screenshot can be tested here:

https://g2-git-update-select-dropdown-portal.itsjonq.vercel.app/iframe.html?id=components-selectdropdown--inline-rendering&viewMode=story

diegohaz commented 3 years ago

The voiceover highlight box doesn't seem to surround the correct UI.

Yeah! It happens sometimes. There's no problem with that from what I can tell.

What's the reason for using Portal here though? It doesn't work well on iOS VoiceOver. Sometimes the listbox item will not get focused automatically, and since it's a portal and is placed at the end of the DOM, the user can't reach it by swiping to the right.

Here's how it works on main:

https://user-images.githubusercontent.com/3068563/106492789-1b8d0c80-6497-11eb-9670-a7ac821b6bdd.mov

And on this branch:

https://user-images.githubusercontent.com/3068563/106492764-129c3b00-6497-11eb-836a-1d748ab99b45.mov

(edit: sorry, the names of the files are inverted)

I think this is the same issue: https://youtu.be/tUlB13vUcac?t=342

Even on main, it looks like there's a focus trap that tries to focus on the dropdown button when the popup closes. Sometimes this causes an issue where the focus goes back to the first dropdown button when you focus on the second one.

ItsJonQ commented 3 years ago

@diegohaz Thank you for your help!

What's the reason for using Portal here though?

There's rendering issues for the Dropdown content if it's contained within a box with overflow:hidden. I provided an example in the initial issue: https://github.com/ItsJonQ/g2/issues/249

Do you feel like this PR is 👍 for now (at least for Desktop)?


From your YouTube video:

I actually think all dropdown menus like this one should be replaced with dialogs on mobile.

I agree 🙏 . That could be a cool enhancement we can add to SelectDropdown and Dropdown to render something like an Action Sheet for mobile:

I think we'd still need to use Portal to achieve a dialog like UI for mobile (like the screenshot above). Would we experience the same voiceover focus issues?

diegohaz commented 3 years ago

It works well on desktop 👍

I think we'd still need to use Portal to achieve a dialog like UI for mobile (like the screenshot above). Would we experience the same voiceover focus issues?

I didn't experience this issue with role="dialog" elements, only with listbox and menu popups. Needs testing, but that should work better on mobile than the current solution.

ItsJonQ commented 3 years ago

I didn't experience this issue with role="dialog" elements

That's really good to know!!! 🙏