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

[SelectMenu] Broken focus/tab index after selecting #2350

Open
OrbisK opened this issue Oct 9, 2024 · 9 comments
Open

[SelectMenu] Broken focus/tab index after selecting #2350

OrbisK opened this issue Oct 9, 2024 · 9 comments
Labels
bug Something isn't working radix-vue v3 #1289

Comments

@OrbisK
Copy link
Contributor

OrbisK commented Oct 9, 2024

Environment

System:
OS: Linux 6.5 Ubuntu 23.10 23.10 (Mantic Minotaur)
CPU: (8) x64 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz
Memory: 44.48 GB / 62.57 GB
Container: Yes
Shell: 5.9 - /usr/bin/zsh
Browsers:
Chrome: 126.0.6478.126

Version

v3.0.0-alpha.6

Reproduction

https://ui3.nuxt.dev/components/select-menu

Description

  1. open docs
  2. open any select menu
  3. select any option
  4. tab to next field
  5. tab index is reset

Additional context

No response

Logs

No response

@OrbisK OrbisK added bug Something isn't working triage v3 #1289 labels Oct 9, 2024
Copy link
Member

I'm sorry but I don't understand what the issue is. Would you mind sharing a video maybe?

@OrbisK
Copy link
Contributor Author

OrbisK commented Oct 10, 2024

select-tab-reset.mp4

at 0:04 i am opening the select menu
at 0:05 i ve selected the option
now i press tab to jump tho the next index.
next index is not at type?: "label" | "separator" | "item" (it should be see pre 0:04)

Copy link
Member

You're right, the focus should come back on the button after the menu closes. I'll investigate!

@benjamincanac benjamincanac removed the triage label Oct 10, 2024 — with Volta.net
@benjamincanac benjamincanac changed the title [Select-Menu] broken focus/tab index after selecting [SelectMenu] Broken focus/tab index after selecting Oct 10, 2024
Copy link
Member

benjamincanac commented Oct 10, 2024

Not sure I can fix this one, the Combobox is designed to focus the ComboboxInput when it closes. However, I've put the ComboboxInput inside the menu to make the SelectMenu.

https://github.com/unovue/radix-vue/blob/main/packages/radix-vue/src/Combobox/ComboboxRoot.vue#L143

@OrbisK
Copy link
Contributor Author

OrbisK commented Oct 11, 2024

Not sure I can fix this one, the Combobox is designed to focus the ComboboxInput when it closes. However, I've put the ComboboxInput inside the menu to make the SelectMenu.

https://github.com/unovue/radix-vue/blob/main/packages/radix-vue/src/Combobox/ComboboxRoot.vue#L143

Yes, because the search is in the content. Crap. It's probably also difficult to simply refocus the trigger when closing :/

Maybe it's a bit hacky but you could do something with the provides of the root. https://github.com/unovue/radix-vue/blob/main/packages/radix-vue/src/Combobox/ComboboxRoot.vue#L249

If you want I can test a bit and see how it would be possible. Is it already possible to develop nuxt/ui@v3 locally?

Copy link
Member

benjamincanac commented Oct 11, 2024

Of course, you just have to checkout the v3 branch, run pnpm install and pnpm run dev:prepare. Then you can run the playground: pnpm run dev or the docs: pnpm run docs.

@OrbisK
Copy link
Contributor Author

OrbisK commented Oct 11, 2024

So. the ComboboxInput is very strongly linked to the focusmanagment and keyboards.

I think there are only 2-3 ways to proceed:

  1. get the search input out of the content (input has to be there, but input can be readonly)
  2. separate the input and the search more strongly in radix-vue

(3.) A: I can probably manage to implement it as planned with a few small hacks. I just don't know how strong these hacks are.

PoC of the hack

<ComboboxRoot v-model:open="open">
  <ComboboxAnchor>
          <ComboboxInput
            as="div"
            tabindex="0"
            @keydown="open=true"
          >My real value</ComboboxInput>
          <ComboboxTrigger>
<!-- ... -->
          </ComboboxTrigger>
        </ComboboxAnchor>
        <ComboboxContent>
            <WrapperComponent>
                <ComboboxInput></ComboboxInput>
            </WrapperComponent>
<!-- ... -->
        </ComboboxContent>
</ComboboxRoot>

WrapperComponent has to inject and reprovide the root context but rewrite the onInputElementChange and onInputNavigation to prevent overriding the main inputElement and handle navigation

B: Second hack option might be to try refocusing the trigger when an option is selected and fiddeling the rest (keyboard controls (if no search), multiple selection,...) around that.

In the long term, it would probably be better to create a feature request in radix-vue and until then probably proceed with option 1 or 3. I can also go into more detail for option 3 about how it could work.

I thought i'd just ask you for your opinion before i dive right in.

I hope my explanations make sense

Copy link
Member

Thanks for the explanation. Not sure I want to start hacking this new version like we did with Headless UI back then. I'm gonna migrate to reka-ui (v2 of radix-vue) once their alpha-2 is released and there are breaking changes on the Combobox component so I would wait to try this.

Once it's done we can submit an issue on Radix Vue to make this work.

@OrbisK
Copy link
Contributor Author

OrbisK commented Oct 11, 2024

Thanks for the explanation. Not sure I want to start hacking this new version like we did with Headless UI back then. I'm gonna migrate to reka-ui (v2 of radix-vue) once their alpha-2 is released and there are breaking changes on the Combobox component so I would wait to try this.

Once it's done we can submit an issue on Radix Vue to make this work.

Ahh exciting. Yes, with v2 there's definitely a lot different in terms of keyboard navigation. Apparently, from v2 onwards, the focus for navigation will be directly on the items. As it looks so far, the input component will probably still be the one that gets the focus after the blur. Maybe you could simply ask here with a mini feature request whether you can simply deactivate this with a prop (e.g. passive) so that the component does not register as “main input”.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working radix-vue v3 #1289
Projects
None yet
Development

No branches or pull requests

2 participants