Ensure correct DOM node order when performing focus actions #1038
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When we are performing actions like `focusIn(list, Focus.First)` then we have to ensrue that we are working with the correct list that is properly sorted. It can happen that the list of DOM nodes is out of sync. This can happen if you have 3 Tabs, hide the second (which triggers an unmount and an `unregister` of the Tab), then re-add the second item in the middle. This will re-add the item to the end of the list instead of in the middle. We can solve this by always sorting items when we are adding / removing items, but this is a bit more error prone because it is easy to forget. Instead we will sort it when performing the actual keyboard action. If we didn't provide a list but an element, then we use a getFocusableElements(element) function, but this already gives you a correctly sorted list so we don't need to do that for this list.
It could still happen that this internal list is not ordered correctly but that's not really a problem we just have the list to keep track of things. For our tests we now use the position from the DOM directly.
|
This pull request is being automatically deployed with Vercel (learn more). headlessui-vue – ./packages/@headlessui-vue🔍 Inspect: https://vercel.com/tailwindlabs/headlessui-vue/7YwmKVo6y92FrXtqXoxAH3YDcvbQ headlessui-react – ./packages/@headlessui-react🔍 Inspect: https://vercel.com/tailwindlabs/headlessui-react/5oYoiWP36aRz62N5yDckZtAfbkSy |
RobinMalfait
changed the title
|
Somewhat related to this, I noticed that the Vue version of RadioGroup has some logic in the registerOption method: to not always assume that a newly-registered option is at the end. For my Svelte port I didn't see any reason to not use that same logic for registering Tab, Listbox, and Menu. I think it fixes issues like #994 |
|
@rgossiaux that's a valid point and thanks for bringing it to my attention! I was thinking on refactoring all of those things to be more like an automatic thing for all components so that we don't have to think about it all the time. |
RobinMalfait
added a commit
that referenced
this pull request
Jan 14, 2022
* placeholder for next release * Ensure portal root exists in the DOM (#950) * ensure that the portal root is always in the DOM When using NextJS, it happens that between page transitions the portal root gets removed form the DOM. We will check the DOM when the `target` updates, and if it doesn't exist anymore, then we will re-insert it in the DOM. * update changelog * Allow `Tabs` to be controllable (#970) * feat(react): Allow Tab Component to be controlled * fix falsy bug `selectedIndex || defaultIndex` would result in the `defaultIndex` if `selectedIndex` is set to 0. This means that if you have this code: ```js <Tab.Group selectedIndex={0} defaultIndex={2} /> ``` That you will never be able to see the very first tab, unless you provided a negative value like `-1`. `selectedIndex ?? defaultIndex` fixes this, since it purely checkes for `undefined` and `null`. * implemented controllable Tabs for Vue * add dedicated test to ensure changing the defaultIndex has no effect * update changelog Co-authored-by: ChiefORZ <seb.schaffernak@gmail.com> * Fix missing key binding in examples (#1036) Co-authored-by: superDragon <xkloveme@gmail.com> * Fix slice => splice typo in Vue Tabs component (#1037) Co-authored-by: Ryan Gossiaux <ryan.gossiaux@gmail.com> * update changelog * Ensure correct DOM node order when performing focus actions (#1038) * ensure that the order of DOM nodes is correct When we are performing actions like `focusIn(list, Focus.First)` then we have to ensrue that we are working with the correct list that is properly sorted. It can happen that the list of DOM nodes is out of sync. This can happen if you have 3 Tabs, hide the second (which triggers an unmount and an `unregister` of the Tab), then re-add the second item in the middle. This will re-add the item to the end of the list instead of in the middle. We can solve this by always sorting items when we are adding / removing items, but this is a bit more error prone because it is easy to forget. Instead we will sort it when performing the actual keyboard action. If we didn't provide a list but an element, then we use a getFocusableElements(element) function, but this already gives you a correctly sorted list so we don't need to do that for this list. * add tests to prove the correct order when performing actions * cleanup code just for tests It could still happen that this internal list is not ordered correctly but that's not really a problem we just have the list to keep track of things. For our tests we now use the position from the DOM directly. * update changelog Co-authored-by: ChiefORZ <seb.schaffernak@gmail.com> Co-authored-by: superDragon <xkloveme@gmail.com> Co-authored-by: Ryan Gossiaux <ryan.gossiaux@gmail.com>
rgossiaux
added a commit
to rgossiaux/svelte-headlessui
that referenced
this pull request
Jun 11, 2023
tailwindlabs/headlessui#1038 This test didn't actually fail for me but w/e.
rgossiaux
added a commit
to rgossiaux/svelte-headlessui
that referenced
this pull request
Jun 11, 2023
tailwindlabs/headlessui#1038 This test didn't actually fail for me but w/e.
rgossiaux
added a commit
to rgossiaux/svelte-headlessui
that referenced
this pull request
Jun 11, 2023
tailwindlabs/headlessui#1038 This test didn't actually fail for me but w/e.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR will ensure that whenever we are performing focus actions on DOM nodes, that those DOM nodes are in the correct order. This is important so that when you want to
focusIn(listOfDOMNodes, Focus.Next)that you are actually focusing the next item in the DOM, and not the next item in the list.This is currently fixed in a spot where we are performing the actions, in a perfect world the list is already correctly sorted. But there are a few other issues I want to fix that should solve this as well. But in the meantime this is good place to have this logic.
Fixes: #1015