Ensure controlled Tabs don't change automagically (#1680)

RobinMalfait · web-flow · commit 6d13e7958ee3 · 2022-07-15T16:54:56.000+02:00
* fix controlled tabs should not switch tabs

When the `Tabs` component is used ina a controlled way, then clicking on
a tab should call the `onChange` callback, but it should not change the
actual tab internally.

* update changelog
diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md
@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fix outside clicks to close dialog when nested, unopened dialogs are present ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667))
 - Close `Menu` component when using `tab` key ([#1673](https://github.com/tailwindlabs/headlessui/pull/1673))
 - Resync input when display value changes ([#1679](https://github.com/tailwindlabs/headlessui/pull/1679))
+- Ensure controlled `Tabs` don't change automagically ([#1680](https://github.com/tailwindlabs/headlessui/pull/1680))
 
 ## [1.6.6] - 2022-07-07
 
diff --git a/packages/@headlessui-react/src/components/tabs/tabs.test.tsx b/packages/@headlessui-react/src/components/tabs/tabs.test.tsx
@@ -543,6 +543,64 @@ describe('Rendering', () => {
   })
 
   describe('`selectedIndex`', () => {
+    it(
+      'should not change the tab in a controlled component if you do not respond to the onChange',
+      suppressConsoleLogs(async () => {
+        let handleChange = jest.fn()
+
+        function ControlledTabs() {
+          let [selectedIndex, setSelectedIndex] = useState(0)
+
+          return (
+            <>
+              <Tab.Group
+                selectedIndex={selectedIndex}
+                onChange={(value) => {
+                  handleChange(value)
+                }}
+              >
+                <Tab.List>
+                  <Tab>Tab 1</Tab>
+                  <Tab>Tab 2</Tab>
+                  <Tab>Tab 3</Tab>
+                </Tab.List>
+
+                <Tab.Panels>
+                  <Tab.Panel>Content 1</Tab.Panel>
+                  <Tab.Panel>Content 2</Tab.Panel>
+                  <Tab.Panel>Content 3</Tab.Panel>
+                </Tab.Panels>
+              </Tab.Group>
+
+              <button>after</button>
+              <button onClick={() => setSelectedIndex((prev) => prev + 1)}>setSelectedIndex</button>
+            </>
+          )
+        }
+
+        render(<ControlledTabs />)
+
+        assertActiveElement(document.body)
+
+        // test controlled behaviour
+        await click(getByText('setSelectedIndex'))
+        assertTabs({ active: 1 })
+        await click(getByText('setSelectedIndex'))
+        assertTabs({ active: 2 })
+
+        // test uncontrolled behaviour again
+        await click(getByText('Tab 1'))
+        assertTabs({ active: 2 }) // Should still be Tab 3 because `selectedIndex` didn't update
+        await click(getByText('Tab 2'))
+        assertTabs({ active: 2 }) // Should still be Tab 3 because `selectedIndex` didn't update
+        await click(getByText('Tab 3'))
+        assertTabs({ active: 2 }) // Should still be Tab 3 because `selectedIndex` didn't update
+        await click(getByText('Tab 1'))
+        expect(handleChange).toHaveBeenCalledTimes(3) // We did see the 'onChange' calls, but only 3 because clicking Tab 3 is already the active one which means that this doesn't trigger the onChange
+        assertTabs({ active: 2 }) // Should still be Tab 3 because `selectedIndex` didn't update
+      })
+    )
+
     it(
       'should be possible to change active tab controlled and uncontrolled',
       suppressConsoleLogs(async () => {
diff --git a/packages/@headlessui-react/src/components/tabs/tabs.tsx b/packages/@headlessui-react/src/components/tabs/tabs.tsx
@@ -26,6 +26,7 @@ import { useResolveButtonType } from '../../hooks/use-resolve-button-type'
 import { useLatestValue } from '../../hooks/use-latest-value'
 import { FocusSentinel } from '../../internal/focus-sentinel'
 import { useEvent } from '../../hooks/use-event'
+import { microTask } from '../../utils/micro-task'
 
 interface StateDefinition {
   selectedIndex: number
@@ -196,6 +197,8 @@ let Tabs = forwardRefWithAs(function Tabs<TTag extends ElementType = typeof DEFA
   const orientation = vertical ? 'vertical' : 'horizontal'
   const activation = manual ? 'manual' : 'auto'
 
+  let isControlled = selectedIndex !== null
+
   let tabsRef = useSyncRefs(ref)
   let [state, dispatch] = useReducer(stateReducer, {
     selectedIndex: selectedIndex ?? defaultIndex,
@@ -211,7 +214,7 @@ let Tabs = forwardRefWithAs(function Tabs<TTag extends ElementType = typeof DEFA
     [orientation, activation, state]
   )
 
-  let lastChangedIndex = useLatestValue(state.selectedIndex)
+  let realSelectedIndex = useLatestValue(isControlled ? props.selectedIndex : state.selectedIndex)
   let tabsActions: _Actions = useMemo(
     () => ({
       registerTab(tab) {
@@ -226,13 +229,16 @@ let Tabs = forwardRefWithAs(function Tabs<TTag extends ElementType = typeof DEFA
         dispatch({ type: ActionTypes.ForceRerender })
       },
       change(index: number) {
-        if (lastChangedIndex.current !== index) onChangeRef.current(index)
-        lastChangedIndex.current = index
+        if (realSelectedIndex.current !== index) {
+          onChangeRef.current(index)
+        }
 
-        dispatch({ type: ActionTypes.SetSelectedIndex, index })
+        if (!isControlled) {
+          dispatch({ type: ActionTypes.SetSelectedIndex, index })
+        }
       },
     }),
-    [dispatch]
+    [dispatch, isControlled]
   )
 
   useIsoMorphicEffect(() => {
@@ -388,9 +394,17 @@ let TabRoot = forwardRefWithAs(function Tab<TTag extends ElementType = typeof DE
     internalTabRef.current?.focus()
   })
 
+  let ready = useRef(false)
   let handleSelection = useEvent(() => {
+    if (ready.current) return
+    ready.current = true
+
     internalTabRef.current?.focus()
     actions.change(myIndex)
+
+    microTask(() => {
+      ready.current = false
+    })
   })
 
   // This is important because we want to only focus the tab when it gets focus
diff --git a/packages/@headlessui-vue/CHANGELOG.md b/packages/@headlessui-vue/CHANGELOG.md
@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fix outside clicks to close dialog when nested, unopened dialogs are present ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667))
 - Close `Menu` component when using `tab` key ([#1673](https://github.com/tailwindlabs/headlessui/pull/1673))
 - Resync input when display value changes ([#1679](https://github.com/tailwindlabs/headlessui/pull/1679))
+- Ensure controlled `Tabs` don't change automagically ([#1680](https://github.com/tailwindlabs/headlessui/pull/1680))
 
 ## [1.6.7] - 2022-07-12
 
diff --git a/packages/@headlessui-vue/src/components/tabs/tabs.test.ts b/packages/@headlessui-vue/src/components/tabs/tabs.test.ts
@@ -515,6 +515,67 @@ describe('Rendering', () => {
 })
 
 describe('`selectedIndex`', () => {
+  it(
+    'should not change the tab in a controlled component if you do not respond to the @change',
+    suppressConsoleLogs(async () => {
+      let handleChange = jest.fn()
+
+      renderTemplate({
+        template: html`
+          <TabGroup @change="handleChange" :selectedIndex="selectedIndex">
+            <TabList>
+              <Tab>Tab 1</Tab>
+              <Tab>Tab 2</Tab>
+              <Tab>Tab 3</Tab>
+            </TabList>
+
+            <TabPanels>
+              <TabPanel>Content 1</TabPanel>
+              <TabPanel>Content 2</TabPanel>
+              <TabPanel>Content 3</TabPanel>
+            </TabPanels>
+          </TabGroup>
+          <button>after</button>
+          <button @click="next">setSelectedIndex</button>
+        `,
+        setup() {
+          let selectedIndex = ref(0)
+
+          return {
+            selectedIndex,
+            handleChange(value: number) {
+              handleChange(value)
+            },
+            next() {
+              selectedIndex.value += 1
+            },
+          }
+        },
+      })
+
+      await new Promise<void>(nextTick)
+
+      assertActiveElement(document.body)
+
+      // test controlled behaviour
+      await click(getByText('setSelectedIndex'))
+      assertTabs({ active: 1 })
+      await click(getByText('setSelectedIndex'))
+      assertTabs({ active: 2 })
+
+      // test uncontrolled behaviour again
+      await click(getByText('Tab 1'))
+      assertTabs({ active: 2 }) // Should still be Tab 3 because `selectedIndex` didn't update
+      await click(getByText('Tab 2'))
+      assertTabs({ active: 2 }) // Should still be Tab 3 because `selectedIndex` didn't update
+      await click(getByText('Tab 3'))
+      assertTabs({ active: 2 }) // Should still be Tab 3 because `selectedIndex` didn't update
+      await click(getByText('Tab 1'))
+      expect(handleChange).toHaveBeenCalledTimes(3) // We did see the '@change' calls, but only 3 because clicking Tab 3 is already the active one which means that this doesn't trigger the @change
+      assertTabs({ active: 2 }) // Should still be Tab 3 because `selectedIndex` didn't update
+    })
+  )
+
   it('should be possible to change active tab controlled and uncontrolled', async () => {
     let handleChange = jest.fn()
 
diff --git a/packages/@headlessui-vue/src/components/tabs/tabs.ts b/packages/@headlessui-vue/src/components/tabs/tabs.ts
@@ -23,6 +23,7 @@ import { match } from '../../utils/match'
 import { focusIn, Focus } from '../../utils/focus-management'
 import { useResolveButtonType } from '../../hooks/use-resolve-button-type'
 import { FocusSentinel } from '../../internal/focus-sentinel'
+import { microTask } from '../../utils/micro-task'
 
 type StateDefinition = {
   // State
@@ -75,16 +76,25 @@ export let TabGroup = defineComponent({
     let tabs = ref<StateDefinition['tabs']['value']>([])
     let panels = ref<StateDefinition['panels']['value']>([])
 
+    let isControlled = computed(() => props.selectedIndex !== null)
+    let realSelectedIndex = computed(() =>
+      isControlled.value ? props.selectedIndex : selectedIndex.value
+    )
+
     let api = {
       selectedIndex,
       orientation: computed(() => (props.vertical ? 'vertical' : 'horizontal')),
       activation: computed(() => (props.manual ? 'manual' : 'auto')),
       tabs,
       panels,
       setSelectedIndex(index: number) {
-        if (selectedIndex.value === index) return
-        selectedIndex.value = index
-        emit('change', index)
+        if (realSelectedIndex.value !== index) {
+          emit('change', index)
+        }
+
+        if (!isControlled.value) {
+          selectedIndex.value = index
+        }
       },
       registerTab(tab: typeof tabs['value'][number]) {
         if (!tabs.value.includes(tab)) tabs.value.push(tab)
@@ -272,11 +282,19 @@ export let Tab = defineComponent({
       dom(internalTabRef)?.focus()
     }
 
+    let ready = ref(false)
     function handleSelection() {
+      if (ready.value) return
+      ready.value = true
+
       if (props.disabled) return
 
       dom(internalTabRef)?.focus()
       api.setSelectedIndex(myIndex.value)
+
+      microTask(() => {
+        ready.value = false
+      })
     }
 
     // This is important because we want to only focus the tab when it gets focus
