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

TabPanel: implement Ariakit internally #52133

Merged
merged 30 commits into from
Aug 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
f20e989
Add Ariakit-powered version of TabPanel
chad1008 Jun 15, 2023
8b002ae
Copy `TabPanel` stories over to `Tabs`
chad1008 Jun 15, 2023
3f9b7e0
Add updated versions of `TabPanel` unit tests
chad1008 Jun 15, 2023
97a4edc
add "Manual Activation" story
chad1008 Jun 19, 2023
bbac641
switch to async/findby based unit testing
chad1008 Jun 28, 2023
3795477
update changelog
chad1008 Jun 29, 2023
5ede3dc
remove old radix dependency
chad1008 Jul 5, 2023
1bab116
rename monolithic component to `TabPanel` for consistency
chad1008 Jul 5, 2023
75f090c
add `useInstanceId` support
chad1008 Jul 6, 2023
ea1e21e
add className and id parity with existing component
chad1008 Jul 6, 2023
dac69b7
update setSelectedId helper
chad1008 Jul 6, 2023
e7b0aa2
apply Ariakit internals to existing TabPanel component
chad1008 Jul 6, 2023
6195305
update TabPanel unit tests to work with Ariakit internals
chad1008 Jul 6, 2023
68fcf7e
remove temporary separate/new component files
chad1008 Jul 12, 2023
b9e019c
Update packages/components/src/tab-panel/stories/index.tsx
chad1008 Jul 13, 2023
37c014b
update CHANGELOG entry
chad1008 Jul 13, 2023
c89aedd
restore package-lock.json
chad1008 Jul 13, 2023
582afa5
remove unnecessary `await`s
chad1008 Jul 13, 2023
c97cbee
move `extractTabName` out of component
chad1008 Jul 13, 2023
00d2eaa
restore original component lazy loading
chad1008 Jul 24, 2023
4b7d7bb
update CHANGELOG entry
chad1008 Jul 24, 2023
aa1bfc2
up editor preferences modal unit tests
chad1008 Jul 24, 2023
defdb27
merge trunk into add/ariakit-tabs
chad1008 Jul 25, 2023
9731da4
remove onSelect call count assertions in unit test
chad1008 Jul 26, 2023
9507f4f
update snapshot and add a component-level test to ensure proper tabin…
chad1008 Jul 26, 2023
d938bd3
merge trunk into add/ariakit-tabs
chad1008 Jul 26, 2023
60c70d5
merge trunk into add/ariakit-tabs
chad1008 Aug 2, 2023
6b98f0a
revert accidental change from `classnames` to `cx`
chad1008 Aug 2, 2023
1782f26
Update CHANGELOG entry
chad1008 Aug 2, 2023
4924d02
remove unneeded `key`
chad1008 Aug 2, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

- `ColorPalette`, `BorderControl`: Don't hyphenate hex value in `aria-label` ([#52932](https://github.com/WordPress/gutenberg/pull/52932)).
- `MenuItemsChoice`, `MenuItem`: Support a `disabled` prop on a menu item ([#52737](https://github.com/WordPress/gutenberg/pull/52737)).
- `TabPanel`: Introduce a new version of `TabPanel` with updated internals and improved adherence to ARIA guidance on `tabpanel` focus behavior while maintaining the same functionality and API surface.([#52133](https://github.com/WordPress/gutenberg/pull/52133)).

### Bug Fix

Expand Down
205 changes: 121 additions & 84 deletions packages/components/src/tab-panel/index.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/**
* External dependencies
*/
import * as Ariakit from '@ariakit/react';
import classnames from 'classnames';
import type { ForwardedRef } from 'react';

Expand All @@ -9,38 +10,30 @@ import type { ForwardedRef } from 'react';
*/
import {
forwardRef,
useState,
useEffect,
useLayoutEffect,
useCallback,
} from '@wordpress/element';
import { useInstanceId } from '@wordpress/compose';
import { useInstanceId, usePrevious } from '@wordpress/compose';

/**
* Internal dependencies
*/
import { NavigableMenu } from '../navigable-container';

import Button from '../button';
import type { TabButtonProps, TabPanelProps } from './types';
import type { TabPanelProps } from './types';
import type { WordPressComponentProps } from '../ui/context';

const TabButton = ( {
tabId,
children,
selected,
...rest
}: TabButtonProps ) => (
<Button
role="tab"
tabIndex={ selected ? undefined : -1 }
aria-selected={ selected }
id={ tabId }
__experimentalIsFocusable
{ ...rest }
>
{ children }
</Button>
);
// Separate the actual tab name from the instance ID. This is
// necessary because Ariakit internally uses the element ID when
// a new tab is selected, but our implementation looks specifically
// for the tab name to be passed to the `onSelect` callback.
const extractTabName = ( id: string | undefined | null ) => {
if ( typeof id === 'undefined' || id === null ) {
return;
}
return id.match( /^tab-panel-[0-9]*-(.*)/ )?.[ 1 ];
};

/**
* TabPanel is an ARIA-compliant tabpanel.
Expand Down Expand Up @@ -92,112 +85,156 @@ const UnforwardedTabPanel = (
ref: ForwardedRef< any >
) => {
const instanceId = useInstanceId( TabPanel, 'tab-panel' );
const [ selected, setSelected ] = useState< string >();

const handleTabSelection = useCallback(
( tabKey: string ) => {
setSelected( tabKey );
onSelect?.( tabKey );
const prependInstanceId = useCallback(
( tabName: string | undefined ) => {
if ( typeof tabName === 'undefined' ) {
return;
}
return `${ instanceId }-${ tabName }`;
},
[ instanceId ]
);

const tabStore = Ariakit.useTabStore( {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just having a look at this PR — is there a reason why we imported * from Ariakit instead of importing only the needed components / functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, actually. Importing just the needed items would have been better. I'll plan a followup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case this is a factor, besides code style/aesthetics, there's no difference between importing the namespace and components individually. In the docs, we sometimes use the namespace import. The reasons are described in this section of the docs: https://ariakit.org/guide/coding-guidelines#import-namespace

setSelectedId: ( newTabValue ) => {
if ( typeof newTabValue === 'undefined' || newTabValue === null ) {
return;
}

const newTab = tabs.find(
( t ) => prependInstanceId( t.name ) === newTabValue
);
if ( newTab?.disabled || newTab === selectedTab ) {
return;
}

const simplifiedTabName = extractTabName( newTabValue );
if ( typeof simplifiedTabName === 'undefined' ) {
return;
}

onSelect?.( simplifiedTabName );
},
orientation,
selectOnMove,
defaultSelectedId: prependInstanceId( initialTabName ),
} );

const selectedTabName = extractTabName( tabStore.useState( 'selectedId' ) );

const setTabStoreSelectedId = useCallback(
( tabName: string ) => {
tabStore.setState( 'selectedId', prependInstanceId( tabName ) );
},
[ onSelect ]
[ prependInstanceId, tabStore ]
);

// Simulate a click on the newly focused tab, which causes the component
// to show the `tab-panel` associated with the clicked tab.
const activateTabAutomatically = (
_childIndex: number,
child: HTMLElement
) => {
child.click();
};
const selectedTab = tabs.find( ( { name } ) => name === selected );
const selectedId = `${ instanceId }-${ selectedTab?.name ?? 'none' }`;
const selectedTab = tabs.find( ( { name } ) => name === selectedTabName );

const previousSelectedTabName = usePrevious( selectedTabName );

// Ensure `onSelect` is called when the initial tab is selected.
useEffect( () => {
if (
previousSelectedTabName !== selectedTabName &&
selectedTabName === initialTabName &&
!! selectedTabName
) {
onSelect?.( selectedTabName );
}
}, [ selectedTabName, initialTabName, onSelect, previousSelectedTabName ] );

// Handle selecting the initial tab.
useLayoutEffect( () => {
// If there's a selected tab, don't override it.
if ( selectedTab ) {
return;
}

const initialTab = tabs.find( ( tab ) => tab.name === initialTabName );

// Wait for the denoted initial tab to be declared before making a
// selection. This ensures that if a tab is declared lazily it can
// still receive initial selection.
if ( initialTabName && ! initialTab ) {
return;
}

if ( initialTab && ! initialTab.disabled ) {
// Select the initial tab if it's not disabled.
handleTabSelection( initialTab.name );
setTabStoreSelectedId( initialTab.name );
} else {
// Fallback to the first enabled tab when the initial is disabled.
// Fallback to the first enabled tab when the initial tab is
// disabled or it can't be found.
const firstEnabledTab = tabs.find( ( tab ) => ! tab.disabled );
if ( firstEnabledTab ) handleTabSelection( firstEnabledTab.name );
if ( firstEnabledTab ) {
setTabStoreSelectedId( firstEnabledTab.name );
}
}
}, [ tabs, selectedTab, initialTabName, handleTabSelection ] );
}, [
tabs,
selectedTab,
initialTabName,
instanceId,
setTabStoreSelectedId,
] );

// Handle the currently selected tab becoming disabled.
useEffect( () => {
// This effect only runs when the selected tab is defined and becomes disabled.
if ( ! selectedTab?.disabled ) {
return;
}

const firstEnabledTab = tabs.find( ( tab ) => ! tab.disabled );

// If the currently selected tab becomes disabled, select the first enabled tab.
// (if there is one).
if ( firstEnabledTab ) {
handleTabSelection( firstEnabledTab.name );
setTabStoreSelectedId( firstEnabledTab.name );
}
}, [ tabs, selectedTab?.disabled, handleTabSelection ] );

}, [ tabs, selectedTab?.disabled, setTabStoreSelectedId, instanceId ] );
return (
<div className={ className } ref={ ref }>
<NavigableMenu
role="tablist"
orientation={ orientation }
onNavigate={
selectOnMove ? activateTabAutomatically : undefined
}
<Ariakit.TabList
store={ tabStore }
className="components-tab-panel__tabs"
>
{ tabs.map( ( tab ) => (
<TabButton
className={ classnames(
'components-tab-panel__tabs-item',
tab.className,
{
[ activeClass ]: tab.name === selected,
{ tabs.map( ( tab ) => {
return (
<Ariakit.Tab
key={ tab.name }
id={ prependInstanceId( tab.name ) }
className={ classnames(
'components-tab-panel__tabs-item',
tab.className,
{
[ activeClass ]:
tab.name === selectedTabName,
}
) }
disabled={ tab.disabled }
aria-controls={ `${ prependInstanceId(
tab.name
) }-view` }
Comment on lines +213 to +215
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current TabPanel component adds aria-controls attributes to all of the Tabs, even though with lazy loading only the currently active tab was actually rendered. Ariakit was only adding this attribute for the current tab, because technically it's the only one that exists. I've manually added this in for consistency to match existing snapshots as closely as possible, but we can easily remove it and update the snapshots to expect the new behavior instead.

render={
<Button
icon={ tab.icon }
label={ tab.icon && tab.title }
showTooltip={ !! tab.icon }
/>
}
) }
tabId={ `${ instanceId }-${ tab.name }` }
aria-controls={ `${ instanceId }-${ tab.name }-view` }
selected={ tab.name === selected }
key={ tab.name }
onClick={ () => handleTabSelection( tab.name ) }
disabled={ tab.disabled }
label={ tab.icon && tab.title }
icon={ tab.icon }
showTooltip={ !! tab.icon }
>
{ ! tab.icon && tab.title }
</TabButton>
) ) }
</NavigableMenu>
>
{ ! tab.icon && tab.title }
</Ariakit.Tab>
);
} ) }
</Ariakit.TabList>
{ selectedTab && (
<div
key={ selectedId }
aria-labelledby={ selectedId }
role="tabpanel"
id={ `${ selectedId }-view` }
className="components-tab-panel__tab-content"
<Ariakit.TabPanel
id={ `${ prependInstanceId( selectedTab.name ) }-view` }
store={ tabStore }
tabId={ prependInstanceId( selectedTab.name ) }
className={ 'components-tab-panel__tab-content' }
>
{ children( selectedTab ) }
</div>
</Ariakit.TabPanel>
) }
</div>
);
Expand Down
6 changes: 6 additions & 0 deletions packages/components/src/tab-panel/stories/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,9 @@ WithTabIconsAndTooltips.args = {
},
],
};

export const ManualActivation = Template.bind( {} );
ManualActivation.args = {
...Default.args,
selectOnMove: false,
};