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

[Vue] - Tabs not working properly when conditionally updating the selected-index prop from the change event #1858

Closed
klausXR opened this issue Sep 16, 2022 · 5 comments · Fixed by #1887
Assignees

Comments

@klausXR
Copy link

klausXR commented Sep 16, 2022

What package within Headless UI are you using?

@headlessui/vue

What version of that package are you using?

v1.7.1

What browser are you using?

Chrome Latest

Reproduction URL

https://codesandbox.io/s/awesome-thompson-ytuh1o?file=/src/App.vue

Describe your issue

I have forms within TabPanel and I want to implement a "Discard changes" confirmation before switching to another tab, if there are unsaved changes.

I tried implementing this with a custom modal and the built in one, but it doesn't work properly.

When I press Cancel, instead of getting back to the browser, the click event triggers again and again and it enters a permanent loop.

What I'm trying to use is a controlled component, I'm using the change event to conditionally determine whether or not the user should move to the new tab.

@klausXR klausXR changed the title [Vue] - Tabs not working properly when requiring confirmation before switching [Vue] - Tabs not working properly when conditionally updating the selected-index prop from the change event Sep 16, 2022
@RobinMalfait RobinMalfait self-assigned this Sep 16, 2022
@RobinMalfait
Copy link
Collaborator

Hey! Thank you for your bug report!
Much appreciated! 🙏

This is an interesting "bug". The issue is not that there is some infinite loop, however the issue is the moment you press cancel, the browser will focus the element you last clicked again. The tabs are implemented in a way so that the moment you focus a tab, that this tab will become the selected one and an onChange will be fired.

The tricky part is that there is no proper way of solving this in Headless UI itself without making assumptions. We can't just focus "nothing" or focus the first tab because that will also trigger the onChange. We can't also change the behaviour of the confirm method (I don't think at least) to not focus the button you clicked on cancel.

One thing you can do is if you implement it with a custom dialog where you have full control, then you can choose what to do when you cancel (e.g.: focus the field that is not filled in yet or something).

Was your goal to use window.confirm or was this as an initial test and you ran into this roadblock?

Either way, do you think?

@klausXR
Copy link
Author

klausXR commented Sep 16, 2022

I see. Kinda tired today, gonna see if I have some time to look at the source code tomorrow and come up with a proposition.

I tried using a custom modal, but it restores focus to the last element, so it causes the same problem.

Why do you switch tabs on focus?

If I remember correctly the proper ways to implement tabs is to have them switch on left/right and mouse clicks, or have them switch on enter/space when the buttons are focusable.

If following these guidelines, the button receiving focus, but the parent not updating the model value should not be an issue

See here

https://www.w3.org/WAI/ARIA/apg/example-index/tabs/tabs-automatic

This one works similarly to what you have implemented in the default mode, it works on left/right and clicks. You can, however, simulate a focus by clicking on the tab, the dragging your mouse outside of the button and release the click. This causes the button to receive focus, but it doesn't switch the open tab.

Edit: just noticed that they actually use negative tabindex on the inactive buttons so you can't focus them, knew I should wait until tomorrow before replying ☹️

Anyway, let me know what you were thinking about when designing the keyboard navigation and we'll see if we can figure something out.

@gecklund
Copy link

I just ran into this on the React side of things and the best way I found to get it working was to use the manual option for the Tab.Group. This seems to work in your Vue example even though it leaves focus on the wrong tab in the end. That seems like an easier problem to solve though..

@klausXR
Copy link
Author

klausXR commented Sep 23, 2022

Yeah, this is how I ended up solving this. I'm still not sure why the tab tries to switch on focus

RobinMalfait added a commit that referenced this issue Sep 30, 2022
The "change on focus" was an incorrect implementation detail that made
it a bit easier but this causes a problem as seen in #1858.

If you want to conditionally check if you want to change the tab or note
(e.g. by using `window.confirm`) then the focus is lost while the popup
is shown. Regardless of your choice, the browser will re-focus the Tab
therefore asking you *again* what you want to do.

This fixes that by only activating the tab if needed while using arrow
keys or when you click the tab (not when it is focused).
RobinMalfait added a commit that referenced this issue Sep 30, 2022
* rework Tabs so that they don't change on focus

The "change on focus" was an incorrect implementation detail that made
it a bit easier but this causes a problem as seen in #1858.

If you want to conditionally check if you want to change the tab or note
(e.g. by using `window.confirm`) then the focus is lost while the popup
is shown. Regardless of your choice, the browser will re-focus the Tab
therefore asking you *again* what you want to do.

This fixes that by only activating the tab if needed while using arrow
keys or when you click the tab (not when it is focused).

* update changelog
@RobinMalfait
Copy link
Collaborator

Hey!

I reworked the internals of the Tab component a bit so that it only changes tabs when you use your arrow keys or when you actually click it. Not when you are just "focusing" it. This was an (incorrect) implementation detail that should now be fixed.

This should be fixed by #1887, and will be available in the next release.

You can already try it using:

  • npm install @headlessui/react@insiders.
  • npm install @headlessui/vue@insiders.

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 a pull request may close this issue.

3 participants