Fix arrow key handling in Tab (after DOM order changes) (#2145)

* detect change in `Tab` order

This will guarantee that when you are using your arrow keys that the
previous / next values are the correct ones instead of the "old" values
before the order change happened.

Fixes: #2131

* update changelog

packages/@headlessui-react/CHANGELOG.md

@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - Fix SSR tab rendering on React 17 ([#2102](https://github.com/tailwindlabs/headlessui/pull/2102))
+- Fix arrow key handling in `Tab` (after DOM order changes) ([#2145](https://github.com/tailwindlabs/headlessui/pull/2145))
 
 ## [1.7.7] - 2022-12-16
 

packages/@headlessui-react/src/components/tabs/tabs.test.tsx

@@ -167,6 +167,177 @@ describe('Rendering', () => {
     })
   )
 
+  it(
+    'should guarantee the order of DOM nodes when reversing the tabs and panels themselves, then performing actions (controlled component)',
+    suppressConsoleLogs(async () => {
+      function Example() {
+        let [selectedIndex, setSelectedIndex] = useState(1)
+        let [tabs, setTabs] = useState([0, 1, 2])
+
+        return (
+          <>
+            <button
+              onClick={() => {
+                setTabs((tabs) => tabs.slice().reverse())
+                setSelectedIndex((idx) => tabs.length - 1 - idx)
+              }}
+            >
+              reverse
+            </button>
+            <Tab.Group selectedIndex={selectedIndex} onChange={setSelectedIndex}>
+              <Tab.List>
+                {tabs.map((tab) => (
+                  <Tab key={tab}>Tab {tab}</Tab>
+                ))}
+              </Tab.List>
+
+              <Tab.Panels>
+                {tabs.map((tab) => (
+                  <Tab.Panel key={tab}>Content {tab}</Tab.Panel>
+                ))}
+              </Tab.Panels>
+            </Tab.Group>
+            <p id="selectedIndex">{selectedIndex}</p>
+          </>
+        )
+      }
+
+      render(<Example />)
+
+      let selectedIndexElement = document.getElementById('selectedIndex')
+
+      assertTabs({ active: 1 })
+
+      await click(getByText('Tab 0'))
+      assertTabs({ active: 0 })
+      expect(selectedIndexElement).toHaveTextContent('0')
+
+      await click(getByText('Tab 1'))
+      assertTabs({ active: 1 })
+      expect(selectedIndexElement).toHaveTextContent('1')
+
+      await click(getByText('Tab 2'))
+      assertTabs({ active: 2 })
+      expect(selectedIndexElement).toHaveTextContent('2')
+
+      await click(getByText('reverse'))
+
+      // Note: the indices are reversed now
+      await click(getByText('Tab 0'))
+      assertTabs({ active: 2 })
+      expect(selectedIndexElement).toHaveTextContent('2')
+
+      await click(getByText('Tab 1'))
+      assertTabs({ active: 1 })
+      expect(selectedIndexElement).toHaveTextContent('1')
+
+      await click(getByText('Tab 2'))
+      assertTabs({ active: 0 })
+      expect(selectedIndexElement).toHaveTextContent('0')
+
+      await click(getByText('reverse'))
+
+      // Note: the indices are reversed again now (back to normal)
+      await click(getByText('Tab 0'))
+      assertTabs({ active: 0 })
+      expect(selectedIndexElement).toHaveTextContent('0')
+
+      await click(getByText('Tab 1'))
+      assertTabs({ active: 1 })
+      expect(selectedIndexElement).toHaveTextContent('1')
+
+      await click(getByText('Tab 2'))
+      assertTabs({ active: 2 })
+      expect(selectedIndexElement).toHaveTextContent('2')
+    })
+  )
+
+  it(
+    'should guarantee the order of DOM nodes when reversing the tabs and panels themselves, then performing actions (uncontrolled component)',
+    suppressConsoleLogs(async () => {
+      function Example() {
+        let [tabs, setTabs] = useState([0, 1, 2])
+
+        return (
+          <>
+            <button
+              onClick={() => {
+                setTabs((tabs) => tabs.slice().reverse())
+              }}
+            >
+              reverse
+            </button>
+            <Tab.Group>
+              {({ selectedIndex }) => (
+                <>
+                  <Tab.List>
+                    {tabs.map((tab) => (
+                      <Tab key={tab}>Tab {tab}</Tab>
+                    ))}
+                  </Tab.List>
+
+                  <Tab.Panels>
+                    {tabs.map((tab) => (
+                      <Tab.Panel key={tab}>Content {tab}</Tab.Panel>
+                    ))}
+                  </Tab.Panels>
+
+                  <p id="selectedIndex">{selectedIndex}</p>
+                </>
+              )}
+            </Tab.Group>
+          </>
+        )
+      }
+
+      render(<Example />)
+
+      let selectedIndexElement = document.getElementById('selectedIndex')
+
+      await click(getByText('Tab 0'))
+      assertTabs({ active: 0 })
+      expect(selectedIndexElement).toHaveTextContent('0')
+
+      await click(getByText('Tab 1'))
+      assertTabs({ active: 1 })
+      expect(selectedIndexElement).toHaveTextContent('1')
+
+      await click(getByText('Tab 2'))
+      assertTabs({ active: 2 })
+      expect(selectedIndexElement).toHaveTextContent('2')
+
+      await click(getByText('reverse'))
+
+      // Note: the indices are reversed now
+      await click(getByText('Tab 0'))
+      assertTabs({ active: 2 })
+      expect(selectedIndexElement).toHaveTextContent('2')
+
+      await click(getByText('Tab 1'))
+      assertTabs({ active: 1 })
+      expect(selectedIndexElement).toHaveTextContent('1')
+
+      await click(getByText('Tab 2'))
+      assertTabs({ active: 0 })
+      expect(selectedIndexElement).toHaveTextContent('0')
+
+      await click(getByText('reverse'))
+
+      // Note: the indices are reversed again now (back to normal)
+      await click(getByText('Tab 0'))
+      assertTabs({ active: 0 })
+      expect(selectedIndexElement).toHaveTextContent('0')
+
+      await click(getByText('Tab 1'))
+      assertTabs({ active: 1 })
+      expect(selectedIndexElement).toHaveTextContent('1')
+
+      await click(getByText('Tab 2'))
+      assertTabs({ active: 2 })
+      expect(selectedIndexElement).toHaveTextContent('2')
+    })
+  )
+
   describe('`renderProps`', () => {
     it(
       'should be possible to render using as={Fragment}',

packages/@headlessui-react/src/components/tabs/tabs.tsx

@@ -61,29 +61,37 @@ let reducers: {
   ) => StateDefinition
 } = {
   [ActionTypes.SetSelectedIndex](state, action) {
-    let focusableTabs = state.tabs.filter((tab) => !tab.current?.hasAttribute('disabled'))
+    let tabs = sortByDomNode(state.tabs, (tab) => tab.current)
+    let panels = sortByDomNode(state.panels, (panel) => panel.current)
+
+    let focusableTabs = tabs.filter((tab) => !tab.current?.hasAttribute('disabled'))
+
+    let nextState = { ...state, tabs, panels }
 
     // Underflow
     if (action.index < 0) {
-      return { ...state, selectedIndex: state.tabs.indexOf(focusableTabs[0]) }
+      return { ...nextState, selectedIndex: tabs.indexOf(focusableTabs[0]) }
     }
 
     // Overflow
-    else if (action.index > state.tabs.length) {
+    else if (action.index > tabs.length) {
       return {
-        ...state,
-        selectedIndex: state.tabs.indexOf(focusableTabs[focusableTabs.length - 1]),
+        ...nextState,
+        selectedIndex: tabs.indexOf(focusableTabs[focusableTabs.length - 1]),
       }
     }
 
     // Middle
-    let before = state.tabs.slice(0, action.index)
-    let after = state.tabs.slice(action.index)
+    let before = tabs.slice(0, action.index)
+    let after = tabs.slice(action.index)
 
     let next = [...after, ...before].find((tab) => focusableTabs.includes(tab))
-    if (!next) return state
+    if (!next) return nextState
 
-    return { ...state, selectedIndex: state.tabs.indexOf(next) }
+    let selectedIndex = tabs.indexOf(next) ?? state.selectedIndex
+    if (selectedIndex === -1) selectedIndex = state.selectedIndex
+
+    return { ...nextState, selectedIndex }
   },
   [ActionTypes.RegisterTab](state, action) {
     if (state.tabs.includes(action.tab)) return state
@@ -237,6 +245,20 @@ let Tabs = forwardRefWithAs(function Tabs<TTag extends ElementType = typeof DEFA
     dispatch({ type: ActionTypes.SetSelectedIndex, index: selectedIndex ?? defaultIndex })
   }, [selectedIndex /* Deliberately skipping defaultIndex */])
 
+  useIsoMorphicEffect(() => {
+    if (realSelectedIndex.current === undefined) return
+    if (state.tabs.length <= 0) return
+
+    // TODO: Figure out a way to detect this without the slow sort on every render. Might be fine
+    //       unless you have a lot of tabs.
+    let sorted = sortByDomNode(state.tabs, (tab) => tab.current)
+    let didOrderChange = sorted.some((tab, i) => state.tabs[i] !== tab)
+
+    if (didOrderChange) {
+      change(sorted.indexOf(state.tabs[realSelectedIndex.current]))
+    }
+  })
+
   let SSRCounter = useRef({ tabs: 0, panels: 0 })
   let ourProps = { ref: tabsRef }
 

packages/@headlessui-vue/CHANGELOG.md

@@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - Ensure `disabled="false"` is not incorrectly passed to the underlying DOM Node ([#2138](https://github.com/tailwindlabs/headlessui/pull/2138))
+- Fix arrow key handling in `Tab` (after DOM order changes) ([#2145](https://github.com/tailwindlabs/headlessui/pull/2145))
 
 ## [1.7.7] - 2022-12-16
 

packages/@headlessui-vue/src/components/tabs/tabs.test.ts

@@ -174,6 +174,175 @@ describe('Rendering', () => {
     })
   )
 
+  it(
+    'should guarantee the order of DOM nodes when reversing the tabs and panels themselves, then performing actions (controlled component)',
+    suppressConsoleLogs(async () => {
+      renderTemplate({
+        template: html`
+          <button @click="reverse">reverse</button>
+          <TabGroup :selectedIndex="selectedIndex" @change="handleChange">
+            <TabList>
+              <Tab v-for="tab in tabs" :key="tab">Tab {{ tab }}</Tab>
+            </TabList>
+            <TabPanels>
+              <TabPanel v-for="tab in tabs" :key="tab">Content {{ tab }}</TabPanel>
+            </TabPanels>
+          </TabGroup>
+          <p id="selectedIndex">{{ selectedIndex }}</p>
+        `,
+        setup() {
+          let selectedIndex = ref(1)
+          let tabs = ref([0, 1, 2])
+
+          return {
+            tabs,
+            selectedIndex,
+            reverse() {
+              tabs.value = tabs.value.slice().reverse()
+              selectedIndex.value = tabs.value.length - 1 - selectedIndex.value
+            },
+            handleChange(value: number) {
+              selectedIndex.value = value
+            },
+          }
+        },
+      })
+
+      await new Promise<void>(nextTick)
+
+      let selectedIndexElement = document.getElementById('selectedIndex')
+
+      assertTabs({ active: 1 })
+
+      await click(getByText('Tab 0'))
+      assertTabs({ active: 0 })
+      expect(selectedIndexElement).toHaveTextContent('0')
+
+      await click(getByText('Tab 1'))
+      assertTabs({ active: 1 })
+      expect(selectedIndexElement).toHaveTextContent('1')
+
+      await click(getByText('Tab 2'))
+      assertTabs({ active: 2 })
+      expect(selectedIndexElement).toHaveTextContent('2')
+
+      await click(getByText('reverse'))
+
+      // Note: the indices are reversed now
+      await click(getByText('Tab 0'))
+      assertTabs({ active: 2 })
+      expect(selectedIndexElement).toHaveTextContent('2')
+
+      await click(getByText('Tab 1'))
+      assertTabs({ active: 1 })
+      expect(selectedIndexElement).toHaveTextContent('1')
+
+      await click(getByText('Tab 2'))
+      assertTabs({ active: 0 })
+      expect(selectedIndexElement).toHaveTextContent('0')
+
+      await click(getByText('reverse'))
+
+      // Note: the indices are reversed again now (back to normal)
+      await click(getByText('Tab 0'))
+      assertTabs({ active: 0 })
+      expect(selectedIndexElement).toHaveTextContent('0')
+
+      await click(getByText('Tab 1'))
+      assertTabs({ active: 1 })
+      expect(selectedIndexElement).toHaveTextContent('1')
+
+      await click(getByText('Tab 2'))
+      assertTabs({ active: 2 })
+      expect(selectedIndexElement).toHaveTextContent('2')
+    })
+  )
+
+  it(
+    'should guarantee the order of DOM nodes when reversing the tabs and panels themselves, then performing actions (uncontrolled component)',
+    suppressConsoleLogs(async () => {
+      renderTemplate({
+        template: html`
+          <button @click="reverse">reverse</button>
+          <TabGroup
+            :selectedIndex="selectedIndex"
+            @change="handleChange"
+            v-slot="{ selectedIndex }"
+          >
+            <TabList>
+              <Tab v-for="tab in tabs" :key="tab">Tab {{ tab }}</Tab>
+            </TabList>
+            <TabPanels>
+              <TabPanel v-for="tab in tabs" :key="tab">Content {{ tab }}</TabPanel>
+            </TabPanels>
+            <p id="selectedIndex">{{ selectedIndex }}</p>
+          </TabGroup>
+        `,
+        setup() {
+          let selectedIndex = ref(1)
+          let tabs = ref([0, 1, 2])
+
+          return {
+            tabs,
+            selectedIndex,
+            reverse() {
+              tabs.value = tabs.value.slice().reverse()
+            },
+            handleChange(value: number) {
+              selectedIndex.value = value
+            },
+          }
+        },
+      })
+
+      await new Promise<void>(nextTick)
+
+      let selectedIndexElement = document.getElementById('selectedIndex')
+
+      await click(getByText('Tab 0'))
+      assertTabs({ active: 0 })
+      expect(selectedIndexElement).toHaveTextContent('0')
+
+      await click(getByText('Tab 1'))
+      assertTabs({ active: 1 })
+      expect(selectedIndexElement).toHaveTextContent('1')
+
+      await click(getByText('Tab 2'))
+      assertTabs({ active: 2 })
+      expect(selectedIndexElement).toHaveTextContent('2')
+
+      await click(getByText('reverse'))
+
+      // Note: the indices are reversed now
+      await click(getByText('Tab 0'))
+      assertTabs({ active: 2 })
+      expect(selectedIndexElement).toHaveTextContent('2')
+
+      await click(getByText('Tab 1'))
+      assertTabs({ active: 1 })
+      expect(selectedIndexElement).toHaveTextContent('1')
+
+      await click(getByText('Tab 2'))
+      assertTabs({ active: 0 })
+      expect(selectedIndexElement).toHaveTextContent('0')
+
+      await click(getByText('reverse'))
+
+      // Note: the indices are reversed again now (back to normal)
+      await click(getByText('Tab 0'))
+      assertTabs({ active: 0 })
+      expect(selectedIndexElement).toHaveTextContent('0')
+
+      await click(getByText('Tab 1'))
+      assertTabs({ active: 1 })
+      expect(selectedIndexElement).toHaveTextContent('1')
+
+      await click(getByText('Tab 2'))
+      assertTabs({ active: 2 })
+      expect(selectedIndexElement).toHaveTextContent('2')
+    })
+  )
+
   describe('`renderProps`', () => {
     it('should expose the `selectedIndex` on the `Tabs` component', async () => {
       renderTemplate(

packages/@headlessui-vue/src/components/tabs/tabs.ts

@@ -20,7 +20,7 @@ import { useId } from '../../hooks/use-id'
 import { Keys } from '../../keyboard'
 import { dom } from '../../utils/dom'
 import { match } from '../../utils/match'
-import { focusIn, Focus, FocusResult } from '../../utils/focus-management'
+import { focusIn, Focus, FocusResult, sortByDomNode } from '../../utils/focus-management'
 import { useResolveButtonType } from '../../hooks/use-resolve-button-type'
 import { FocusSentinel } from '../../internal/focus-sentinel'
 import { microTask } from '../../utils/micro-task'
@@ -134,6 +134,9 @@ export let TabGroup = defineComponent({
       if (api.tabs.value.length <= 0) return
       if (props.selectedIndex === null && selectedIndex.value !== null) return
 
+      api.tabs.value = sortByDomNode(api.tabs.value, dom)
+      api.panels.value = sortByDomNode(api.panels.value, dom)
+
       let tabs = api.tabs.value.map((tab) => dom(tab)).filter(Boolean) as HTMLElement[]
       let focusableTabs = tabs.filter((tab) => !tab.hasAttribute('disabled'))
 
@@ -157,7 +160,25 @@ export let TabGroup = defineComponent({
         let next = [...after, ...before].find((tab) => focusableTabs.includes(tab))
         if (!next) return
 
-        selectedIndex.value = tabs.indexOf(next)
+        let localSelectedIndex = tabs.indexOf(next) ?? api.selectedIndex.value
+        if (localSelectedIndex === -1) localSelectedIndex = api.selectedIndex.value
+
+        selectedIndex.value = localSelectedIndex
+      }
+    })
+
+    watchEffect(() => {
+      if (!isControlled.value) return
+      if (realSelectedIndex.value == null) return
+      if (api.tabs.value.length <= 0) return
+
+      let sorted = sortByDomNode(api.tabs.value, dom)
+      let didOrderChange = sorted.some((tab, i) => dom(api.tabs.value[i]) !== dom(tab))
+
+      if (didOrderChange) {
+        api.setSelectedIndex(
+          sorted.findIndex((x) => dom(x) === dom(api.tabs.value[realSelectedIndex.value!]))
+        )
       }
     })