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.

Sign up for GitHub

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

Tabs: improve focus behavior #55287

Merged
merged 8 commits into from
Nov 7, 2023
Merged

Tabs: improve focus behavior #55287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f47b595
forward `focusable` and add focus styles
chad1008 Oct 11, 2023
11d34e2
add focusable false story
chad1008 Oct 11, 2023
90617a3
move `focusable` demo to the default story
chad1008 Oct 12, 2023
0fc9c68
add focusable prop unit tests
chad1008 Oct 12, 2023
02dda6e
improve formatting consistency in types docs
chad1008 Nov 6, 2023
e779ff9
udate CHANGELOG
chad1008 Nov 6, 2023
fcd555d
stop being a dummy
chad1008 Nov 6, 2023
096f66a
Merge branch 'trunk' into enhance/improve-tabs-focus
chad1008 Nov 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

- Migrate `Divider` from `reakit` to `ariakit` ([#55622](https://github.com/WordPress/gutenberg/pull/55622))

### Experimental

- `Tabs`: Add `focusable` prop to the `Tabs.TabPanel` sub-component ([#55287](https://github.com/WordPress/gutenberg/pull/55287))

### Enhancements

- `ToggleGroupControl`: Add opt-in prop for 40px default size ([#55789](https://github.com/WordPress/gutenberg/pull/55789)).
Expand Down
7 changes: 7 additions & 0 deletions packages/components/src/tabs/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,3 +240,10 @@ The class name to apply to the tabpanel.
Custom CSS styles for the tab.

- Required: No

###### `focusable`: `boolean`

Determines whether or not the tabpanel element should be focusable. If `false`, pressing the tab key will skip over the tabpanel, and instead focus on the first focusable element in the panel (if there is one).

- Required: No
- Default: `true`
10 changes: 9 additions & 1 deletion packages/components/src/tabs/stories/index.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,16 @@ const Template: StoryFn< typeof Tabs > = ( props ) => {
<Tabs.TabPanel id={ 'tab2' }>
<p>Selected tab: Tab 2</p>
</Tabs.TabPanel>
<Tabs.TabPanel id={ 'tab3' }>
<Tabs.TabPanel id={ 'tab3' } focusable={ false }>
<p>Selected tab: Tab 3</p>
<p>
This tabpanel has its <code>focusable</code> prop set to
<code> false</code>, so it won&apos;t get a tab stop.
<br />
Instead, the [Tab] key will move focus to the first
focusable element within the panel.
</p>
<Button variant="primary">I&apos;m a button!</Button>
</Tabs.TabPanel>
</Tabs>
);
Expand Down
16 changes: 16 additions & 0 deletions packages/components/src/tabs/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,3 +101,19 @@ export const Tab = styled( Ariakit.Tab )`
}
}
`;

export const TabPanel = styled( Ariakit.TabPanel )`
&:focus {
box-shadow: none;
outline: none;
}

&:focus-visible {
border-radius: 2px;
box-shadow: 0 0 0 var( --wp-admin-border-width-focus )
${ COLORS.theme.accent };
// Windows high contrast mode.
outline: 2px solid transparent;
outline-offset: 0;
}
`;
13 changes: 8 additions & 5 deletions packages/components/src/tabs/tabpanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
/**
* External dependencies
*/
// eslint-disable-next-line no-restricted-imports
import * as Ariakit from '@ariakit/react';

/**
* WordPress dependencies
Expand All @@ -14,12 +12,16 @@ import { forwardRef, useContext } from '@wordpress/element';
* Internal dependencies
*/
import type { TabPanelProps } from './types';
import { TabPanel as StyledTabPanel } from './styles';

import warning from '@wordpress/warning';
import { TabsContext } from './context';

export const TabPanel = forwardRef< HTMLDivElement, TabPanelProps >(
function TabPanel( { children, id, className, style }, ref ) {
function TabPanel(
{ children, id, className, style, focusable = true },
ref
) {
const context = useContext( TabsContext );
if ( ! context ) {
warning( '`Tabs.TabPanel` must be wrapped in a `Tabs` component.' );
Expand All @@ -28,15 +30,16 @@ export const TabPanel = forwardRef< HTMLDivElement, TabPanelProps >(
const { store, instanceId } = context;

return (
<Ariakit.TabPanel
<StyledTabPanel
focusable={ focusable }
ref={ ref }
style={ style }
store={ store }
id={ `${ instanceId }-${ id }-view` }
className={ className }
>
{ children }
</Ariakit.TabPanel>
</StyledTabPanel>
);
}
);
66 changes: 65 additions & 1 deletion packages/components/src/tabs/test/index.tsx
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you run into any flaky test behavior? When watching the tests, sometimes I'll get a random test failing. For an example:

● Tabs › Uncontrolled mode › Disabled tab › should disable the tab when `disabled` is `true`

expect(element).toHaveFocus()

Expected element with focus:
<button aria-controls="tabs-21-delta-view" aria-disabled="true" aria-selected="false" class="delta-class css-1v2gegi-Tab enfox0g1" data-command="" id="tabs-21-delta" role="tab" tabindex="-1" type="button">Delta</button>
Received element with focus:
<button aria-controls="tabs-21-alpha-view" aria-selected="true" class="alpha-class css-1v2gegi-Tab enfox0g1" data-active-item="" data-command="" data-focus-visible="" id="tabs-21-alpha" role="tab" type="button">Alpha</button>

It'll pass the next 4-5 times then fail again. I'm wondering if it's something related to past flaky test issues we've enountered in the ariakit migrations: #52133 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. If I run them a bunch in a row, I do see that. 🤔 Took me probably close to 20 runs to trigger it, but there it is. I can actually see two tests doing it:

  • Tabs › Uncontrolled mode › Disabled tab › should disable the tab when disabledistrue`
  • Tabs › Tab Activation › should not focus the next tab when the Tab key is pressed

Maybe it's related to that other issue you mentioned, but if it is, the same fix isn't helping us here... one of those failures is actually already using findByRole.

I'll note this to investigate in a followup, that way if we deploy something to fix it, it isn't wrapped up in this change!

Copy link
Contributor

Choose a reason for hiding this comment

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

For tooltip, we added a helper function to ensure everything was resolved before moving to the next test. So I wonder if it would help to add another user.tab() at the end of each test (or to specific tests).

I'll note this to investigate in a followup, that way if we deploy something to fix it, it isn't wrapped up in this change!

Good plan!

Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ type Tab = {
icon?: IconType;
disabled?: boolean;
};
tabpanel?: {
focusable?: boolean;
};
};

const TABS: Tab[] = [
Expand Down Expand Up @@ -83,7 +86,11 @@ const UncontrolledTabs = ( {
) ) }
</Tabs.TabList>
{ tabs.map( ( tabObj ) => (
<Tabs.TabPanel key={ tabObj.id } id={ tabObj.id }>
<Tabs.TabPanel
key={ tabObj.id }
id={ tabObj.id }
focusable={ tabObj.tabpanel?.focusable }
>
{ tabObj.content }
</Tabs.TabPanel>
) ) }
Expand Down Expand Up @@ -184,6 +191,63 @@ describe( 'Tabs', () => {
);
} );
} );
describe( 'Focus Behavior', () => {
it( 'should focus on the related TabPanel when pressing the Tab key', async () => {
const user = userEvent.setup();

render( <UncontrolledTabs tabs={ TABS } /> );

const selectedTabPanel = await screen.findByRole( 'tabpanel' );

// Tab should initially focus the first tab in the tablist, which
// is Alpha.
await user.keyboard( '[Tab]' );
expect(
await screen.findByRole( 'tab', { name: 'Alpha' } )
).toHaveFocus();

// By default the tabpanel should receive focus
await user.keyboard( '[Tab]' );
expect( selectedTabPanel ).toHaveFocus();
Copy link
Contributor

Choose a reason for hiding this comment

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

In your testing instructions, you have the following step:

While the tabpanel is focused, inspect the element and confirm the component's new :focus-visible style is overriding the browser defaults.

Since you mentioned it in your testing instructions, maybe it would be worth including in the test? Something like:

expect( selectedTabPanel ).toHaveAttribute( 'data-focus-visible' );

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 tabpanel element receiving :focus-visible isn't new in this case, so I don't want to add a test for it. The change around :focus-visible for this PR is that when the tab-panel gains that attribute, it gets styled by our new CSS, rather than the browser defaults, which is what was styling it previously :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying! 👍

} );
it( 'should not focus on the related TabPanel when pressing the Tab key if `focusable: false` is set', async () => {
const user = userEvent.setup();

const TABS_WITH_ALPHA_FOCUSABLE_FALSE = TABS.map( ( tabObj ) =>
tabObj.id === 'alpha'
? {
...tabObj,
content: (
<>
Selected Tab: Alpha
<button>Alpha Button</button>
</>
),
tabpanel: { focusable: false },
}
: tabObj
);

render(
<UncontrolledTabs tabs={ TABS_WITH_ALPHA_FOCUSABLE_FALSE } />
);

const alphaButton = await screen.findByRole( 'button', {
name: /alpha button/i,
} );

// Tab should initially focus the first tab in the tablist, which
// is Alpha.
await user.keyboard( '[Tab]' );
expect(
await screen.findByRole( 'tab', { name: 'Alpha' } )
).toHaveFocus();
// Because the alpha tabpanel is set to `focusable: false`, pressing
// the Tab key should focus the button, not the tabpanel
await user.keyboard( '[Tab]' );
expect( alphaButton ).toHaveFocus();
} );
} );

describe( 'Tab Attributes', () => {
it( "should apply the tab's `className` to the tab button", async () => {
Expand Down
10 changes: 9 additions & 1 deletion packages/components/src/tabs/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ export type TabPanelProps = {
*/
children?: React.ReactNode;
/**
* A unique identifier for the TabPanel, which is used to generate a unique `id` for the underlying element.
* A unique identifier for the tabpanel, which is used to generate a unique `id` for the underlying element.
*/
id: string;
/**
Expand All @@ -139,4 +139,12 @@ export type TabPanelProps = {
* Custom CSS styles for the rendered `TabPanel` component.
*/
style?: React.CSSProperties;
/**
* Determines whether or not the tabpanel element should be focusable.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very minor, but I just noticed the variety of formatting of TabPanel in the types.

Screenshot 2023-11-06 at 11 10 06 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I made one small change, but I didn't update the style JSDoc because that one's going away in a separate PR merging soon, and changing it here would have just been asking for a merge conflict to reconcile 😆

* If `false`, pressing the tab key will skip over the tabpanel, and instead
* focus on the first focusable element in the panel (if there is one).
*
* @default true
*/
focusable?: boolean;
};
Loading