From 0b268a09bb3572843158fb01b878962090e535e8 Mon Sep 17 00:00:00 2001 From: Albert Yu Date: Fri, 10 Jan 2025 14:47:47 +0800 Subject: [PATCH 1/6] Add test --- packages/react/src/tabs/root/TabsRoot.test.tsx | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/react/src/tabs/root/TabsRoot.test.tsx b/packages/react/src/tabs/root/TabsRoot.test.tsx index 2233008e34..b1e88bf2d8 100644 --- a/packages/react/src/tabs/root/TabsRoot.test.tsx +++ b/packages/react/src/tabs/root/TabsRoot.test.tsx @@ -184,6 +184,21 @@ describe('', () => { expect(handleChange.firstCall.args[0]).to.equal(1); }); + it('should not call onValueChange on non-primary button clicks', async () => { + const handleChange = spy(); + const { getAllByRole } = await render( + + + + + + , + ); + + fireEvent.click(getAllByRole('tab')[1], { button: 2 }); + expect(handleChange.callCount).to.equal(0); + }); + it('should not call onValueChange when already selected', async () => { const handleChange = spy(); const { getAllByRole } = await render( From f81b0f436cb4b4b03b775c3170e1139a4b82f0d8 Mon Sep 17 00:00:00 2001 From: Albert Yu Date: Fri, 10 Jan 2025 23:36:03 +0800 Subject: [PATCH 2/6] Fix tab activating on focus triggered by non-primary button clicks --- packages/react/src/tabs/tab/useTabsTab.ts | 66 +++++++++++++++-------- 1 file changed, 45 insertions(+), 21 deletions(-) diff --git a/packages/react/src/tabs/tab/useTabsTab.ts b/packages/react/src/tabs/tab/useTabsTab.ts index 16e285d224..b4de1b86d3 100644 --- a/packages/react/src/tabs/tab/useTabsTab.ts +++ b/packages/react/src/tabs/tab/useTabsTab.ts @@ -1,6 +1,7 @@ 'use client'; import * as React from 'react'; import { mergeReactProps } from '../../utils/mergeReactProps'; +import { ownerDocument } from '../../utils/owner'; import { useEnhancedEffect } from '../../utils/useEnhancedEffect'; import { useForkRef } from '../../utils/useForkRef'; import { useBaseUiId } from '../../utils/useBaseUiId'; @@ -75,32 +76,56 @@ function useTabsTab(parameters: useTabsTab.Parameters): useTabsTab.ReturnValue { const tabPanelId = index > -1 ? getTabPanelIdByTabValueOrIndex(valueParam, index) : undefined; + const isPressingRef = React.useRef(false); + const isPrimaryButtonRef = React.useRef(false); + const getRootProps = React.useCallback( (externalProps = {}) => { return mergeReactProps<'button'>( externalProps, - mergeReactProps<'button'>( - { - role: 'tab', - 'aria-controls': tabPanelId, - 'aria-selected': selected, - id, - ref: handleRef, - onClick(event) { + { + role: 'tab', + 'aria-controls': tabPanelId, + 'aria-selected': selected, + id, + ref: handleRef, + onClick(event) { + if (selected) { + return; + } + + onTabActivation(tabValue, event.nativeEvent); + }, + onFocus(event) { + if (!activateOnFocus || selected) { + return; + } + + if (!isPressingRef.current || (isPressingRef.current && isPrimaryButtonRef.current)) { onTabActivation(tabValue, event.nativeEvent); - }, - onFocus(event) { - if (!activateOnFocus) { - return; - } - - if (selectedTabValue !== tabValue) { - onTabActivation(tabValue, event.nativeEvent); - } - }, + } + }, + onPointerDown(event) { + if (selected) { + return; + } + + isPressingRef.current = true; + + function handlePointerUp() { + isPressingRef.current = false; + isPrimaryButtonRef.current = false; + } + + if (!event.button || event.button === 0) { + isPrimaryButtonRef.current = true; + + const doc = ownerDocument(event.currentTarget); + doc.addEventListener('pointerup', handlePointerUp, { once: true }); + } }, - mergeReactProps(getItemProps(), getButtonProps()), - ), + }, + mergeReactProps(getItemProps(), getButtonProps()), ); }, [ @@ -111,7 +136,6 @@ function useTabsTab(parameters: useTabsTab.Parameters): useTabsTab.ReturnValue { id, onTabActivation, selected, - selectedTabValue, tabPanelId, tabValue, ], From 6e63c2bf352e0cb382a1f6ac7b89a35534e1b961 Mon Sep 17 00:00:00 2001 From: Albert Yu Date: Mon, 13 Jan 2025 13:59:11 +0800 Subject: [PATCH 3/6] Fix event.button check --- .../react/src/number-field/root/useNumberFieldRoot.ts | 3 +-- packages/react/src/tabs/tab/useTabsTab.ts | 10 +++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/packages/react/src/number-field/root/useNumberFieldRoot.ts b/packages/react/src/number-field/root/useNumberFieldRoot.ts index 98eef85a07..3f64b7bbdc 100644 --- a/packages/react/src/number-field/root/useNumberFieldRoot.ts +++ b/packages/react/src/number-field/root/useNumberFieldRoot.ts @@ -445,9 +445,8 @@ export function useNumberFieldRoot( incrementValue(amount, isIncrement ? 1 : -1, undefined, event.nativeEvent); }, onPointerDown(event) { - const isMainButton = !event.button || event.button === 0; const isDisabled = disabled || (isIncrement ? isMax : isMin); - if (event.defaultPrevented || readOnly || !isMainButton || isDisabled) { + if (event.defaultPrevented || readOnly || event.button !== 0 || isDisabled) { return; } diff --git a/packages/react/src/tabs/tab/useTabsTab.ts b/packages/react/src/tabs/tab/useTabsTab.ts index b4de1b86d3..9637d30f69 100644 --- a/packages/react/src/tabs/tab/useTabsTab.ts +++ b/packages/react/src/tabs/tab/useTabsTab.ts @@ -77,7 +77,7 @@ function useTabsTab(parameters: useTabsTab.Parameters): useTabsTab.ReturnValue { const tabPanelId = index > -1 ? getTabPanelIdByTabValueOrIndex(valueParam, index) : undefined; const isPressingRef = React.useRef(false); - const isPrimaryButtonRef = React.useRef(false); + const isMainButtonRef = React.useRef(false); const getRootProps = React.useCallback( (externalProps = {}) => { @@ -101,7 +101,7 @@ function useTabsTab(parameters: useTabsTab.Parameters): useTabsTab.ReturnValue { return; } - if (!isPressingRef.current || (isPressingRef.current && isPrimaryButtonRef.current)) { + if (!isPressingRef.current || (isPressingRef.current && isMainButtonRef.current)) { onTabActivation(tabValue, event.nativeEvent); } }, @@ -114,11 +114,11 @@ function useTabsTab(parameters: useTabsTab.Parameters): useTabsTab.ReturnValue { function handlePointerUp() { isPressingRef.current = false; - isPrimaryButtonRef.current = false; + isMainButtonRef.current = false; } - if (!event.button || event.button === 0) { - isPrimaryButtonRef.current = true; + if (event.button === 0) { + isMainButtonRef.current = true; const doc = ownerDocument(event.currentTarget); doc.addEventListener('pointerup', handlePointerUp, { once: true }); From 8c994413599da6111f2e556a4ca42e8d9208375f Mon Sep 17 00:00:00 2001 From: Albert Yu Date: Mon, 13 Jan 2025 14:05:42 +0800 Subject: [PATCH 4/6] Revert "Fix event.button check" 6e63c2bf352e0cb382a1f6ac7b89a35534e1b961 --- .../react/src/number-field/root/useNumberFieldRoot.ts | 3 ++- packages/react/src/tabs/tab/useTabsTab.ts | 10 +++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/react/src/number-field/root/useNumberFieldRoot.ts b/packages/react/src/number-field/root/useNumberFieldRoot.ts index 3f64b7bbdc..98eef85a07 100644 --- a/packages/react/src/number-field/root/useNumberFieldRoot.ts +++ b/packages/react/src/number-field/root/useNumberFieldRoot.ts @@ -445,8 +445,9 @@ export function useNumberFieldRoot( incrementValue(amount, isIncrement ? 1 : -1, undefined, event.nativeEvent); }, onPointerDown(event) { + const isMainButton = !event.button || event.button === 0; const isDisabled = disabled || (isIncrement ? isMax : isMin); - if (event.defaultPrevented || readOnly || event.button !== 0 || isDisabled) { + if (event.defaultPrevented || readOnly || !isMainButton || isDisabled) { return; } diff --git a/packages/react/src/tabs/tab/useTabsTab.ts b/packages/react/src/tabs/tab/useTabsTab.ts index 9637d30f69..b4de1b86d3 100644 --- a/packages/react/src/tabs/tab/useTabsTab.ts +++ b/packages/react/src/tabs/tab/useTabsTab.ts @@ -77,7 +77,7 @@ function useTabsTab(parameters: useTabsTab.Parameters): useTabsTab.ReturnValue { const tabPanelId = index > -1 ? getTabPanelIdByTabValueOrIndex(valueParam, index) : undefined; const isPressingRef = React.useRef(false); - const isMainButtonRef = React.useRef(false); + const isPrimaryButtonRef = React.useRef(false); const getRootProps = React.useCallback( (externalProps = {}) => { @@ -101,7 +101,7 @@ function useTabsTab(parameters: useTabsTab.Parameters): useTabsTab.ReturnValue { return; } - if (!isPressingRef.current || (isPressingRef.current && isMainButtonRef.current)) { + if (!isPressingRef.current || (isPressingRef.current && isPrimaryButtonRef.current)) { onTabActivation(tabValue, event.nativeEvent); } }, @@ -114,11 +114,11 @@ function useTabsTab(parameters: useTabsTab.Parameters): useTabsTab.ReturnValue { function handlePointerUp() { isPressingRef.current = false; - isMainButtonRef.current = false; + isPrimaryButtonRef.current = false; } - if (event.button === 0) { - isMainButtonRef.current = true; + if (!event.button || event.button === 0) { + isPrimaryButtonRef.current = true; const doc = ownerDocument(event.currentTarget); doc.addEventListener('pointerup', handlePointerUp, { once: true }); From 727bdd4e1881f925ccf88c10b91cacf14791c93d Mon Sep 17 00:00:00 2001 From: Albert Yu Date: Mon, 13 Jan 2025 14:06:49 +0800 Subject: [PATCH 5/6] Rename --- packages/react/src/tabs/tab/useTabsTab.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/react/src/tabs/tab/useTabsTab.ts b/packages/react/src/tabs/tab/useTabsTab.ts index b4de1b86d3..f138948379 100644 --- a/packages/react/src/tabs/tab/useTabsTab.ts +++ b/packages/react/src/tabs/tab/useTabsTab.ts @@ -77,7 +77,7 @@ function useTabsTab(parameters: useTabsTab.Parameters): useTabsTab.ReturnValue { const tabPanelId = index > -1 ? getTabPanelIdByTabValueOrIndex(valueParam, index) : undefined; const isPressingRef = React.useRef(false); - const isPrimaryButtonRef = React.useRef(false); + const isMainButtonRef = React.useRef(false); const getRootProps = React.useCallback( (externalProps = {}) => { @@ -101,7 +101,7 @@ function useTabsTab(parameters: useTabsTab.Parameters): useTabsTab.ReturnValue { return; } - if (!isPressingRef.current || (isPressingRef.current && isPrimaryButtonRef.current)) { + if (!isPressingRef.current || (isPressingRef.current && isMainButtonRef.current)) { onTabActivation(tabValue, event.nativeEvent); } }, @@ -114,11 +114,11 @@ function useTabsTab(parameters: useTabsTab.Parameters): useTabsTab.ReturnValue { function handlePointerUp() { isPressingRef.current = false; - isPrimaryButtonRef.current = false; + isMainButtonRef.current = false; } if (!event.button || event.button === 0) { - isPrimaryButtonRef.current = true; + isMainButtonRef.current = true; const doc = ownerDocument(event.currentTarget); doc.addEventListener('pointerup', handlePointerUp, { once: true }); From b65321cf3295105f2145b3303060a4644b9a0923 Mon Sep 17 00:00:00 2001 From: Albert Yu Date: Mon, 13 Jan 2025 14:19:59 +0800 Subject: [PATCH 6/6] Add a test --- .../react/src/tabs/root/TabsRoot.test.tsx | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/react/src/tabs/root/TabsRoot.test.tsx b/packages/react/src/tabs/root/TabsRoot.test.tsx index b1e88bf2d8..cc902c7472 100644 --- a/packages/react/src/tabs/root/TabsRoot.test.tsx +++ b/packages/react/src/tabs/root/TabsRoot.test.tsx @@ -168,6 +168,23 @@ describe('', () => { }); describe('prop: onValueChange', () => { + it('should call onValueChange on pointerdown', async () => { + const handleChange = spy(); + const handlePointerDown = spy(); + const { getAllByRole, user } = await render( + + + + + + , + ); + + await user.pointer({ keys: '[MouseLeft>]', target: getAllByRole('tab')[1] }); + expect(handleChange.callCount).to.equal(1); + expect(handlePointerDown.callCount).to.equal(1); + }); + it('should call onValueChange when clicking', async () => { const handleChange = spy(); const { getAllByRole } = await render( @@ -184,7 +201,7 @@ describe('', () => { expect(handleChange.firstCall.args[0]).to.equal(1); }); - it('should not call onValueChange on non-primary button clicks', async () => { + it('should not call onValueChange on non-main button clicks', async () => { const handleChange = spy(); const { getAllByRole } = await render(