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

[useSelect][useCombobox] Create prototype to have only useDownshift that covers both scenarios. #899

Closed
raunofreiberg opened this issue Jan 15, 2020 · 9 comments

Comments

@raunofreiberg
Copy link

  • downshift version: 4.0.5
  • node version: n/a
  • npm (or yarn) version: n/a

What happened:

When it's possible to scroll the document, opening the menu scrolls to the bottom of the page, when using useSelect with a portal.

Reproduction repository:

https://codesandbox.io/s/useselect-usage-0ifnq

@raunofreiberg
Copy link
Author

I guess a solution would be to always render the portal, regardless of the isOpen state. Although, is it really necessary to have X amount of portals rendered in the page?

@silviuaavram
Copy link
Collaborator

I think it's the fact that when we focus the menu, even though it's visually near the button, it's actually at the end of the page DOM-wise. So focus goes to the end of the page.

This is my initial thought.

@raunofreiberg
Copy link
Author

raunofreiberg commented Jan 15, 2020

If this is of any help, same example with the usage of Downshift render prop API does not scroll the page, though.

https://codesandbox.io/s/useselect-usage-pxxk4

@silviuaavram
Copy link
Collaborator

silviuaavram commented Jan 15, 2020

Yeah, the focus on the list is causing your issue. If you are using Downshift / useCombobox you should get the activeDescendant value from the inputProps and use that on the toggle button, for accessibility.

I will leave this issue open, as I am studying the possibility of having only one hook to cover both select and combobox. I split them due to the focus issue and some other things, but I think that I can keep focus on the toggle button and still be accessible, in both cases. Will follow up with a PR for a unified useDownshift prototype and see how it behaves.

Even though the hooks are used that's fine, I think that switching between them to useDownshift should be without any breaking changes. Maybe some renames in stateChangeTypes and TS typings of course. But other than that I aim to make it as painlessly as possible. We shall see.

@silviuaavram silviuaavram changed the title [useSelect] Opening menu inside portal scrolls the page [useSelect][useCombobox] Create prototype to have only useDownshift that covers both scenarios. Jan 15, 2020
@silviuaavram
Copy link
Collaborator

Closing in favour of moving forward with separate hooks. As part of #907 the list should not be focused anymore, functionality kept, and this bug should be fixed.

@silviuaavram
Copy link
Collaborator

We have experimented with keeping the focus on the button, but that breaks accessibility. If focus stays on the button, when menu is open, screen readers will not enter in Forms Mode. To conclude, the focus will stay on the list for now, as ARIA suggests in their documentation.

That being said, I don't think you need a portal for that list, you can achieve the same result with CSS. Or maybe I am missing something.

@raunofreiberg
Copy link
Author

That being said, I don't think you need a portal for that list, you can achieve the same result with CSS. Or maybe I am missing something.

Without portal, parent elements with overflow: hidden would clip the menu, which is why we want to render the menu in a portal.

@silviuaavram
Copy link
Collaborator

Closing this as I don't have a solution. Either use portal in scenarios scrolling at focus is not needed or try to fix it from css. Thanks for reporting it and sorry it cannot work in this case :(

@raunofreiberg
Copy link
Author

raunofreiberg commented Apr 25, 2020

We have experimented with keeping the focus on the button, but that breaks accessibility. If focus stays on the button, when menu is open, screen readers will not enter in Forms Mode. To conclude, the focus will stay on the list for now, as ARIA suggests in their documentation.

I'm just curious - does the render prop API of Downshift not keep the focus on the button then, seeing as it can work with portals without scrolling?

Example: https://codesandbox.io/s/useselect-usage-kkouj?file=/index.js

Thanks!

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

No branches or pull requests

2 participants