Skip to content

Navigator: focus screen wrapper instead of first tabbable element #51816

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

Closed
ciampo wants to merge 8 commits into from
Closed

Navigator: focus screen wrapper instead of first tabbable element #51816

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
eae594c
Navigator: focus screen wrapper instead of first tabbable element
ciampo Jun 22, 2023
5feb5e1
Update unit tests
ciampo Jun 22, 2023
e1056a3
Update snapshot
ciampo Jun 22, 2023
5d0dc09
CHANGELOG
ciampo Jun 22, 2023
6a90b3a
Add aria-label, aria-labelledby and role props, including types, test…
ciampo Jun 23, 2023
4c868df
Add focus outline to screen
ciampo Jun 26, 2023
89c0286
Remove `aria-labelledby` from types and docs, add default `aria-label…
ciampo Jun 26, 2023
7409673
Merge branch 'trunk' of github.com:WordPress/gutenberg into feat/navi…
alexstine Aug 5, 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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@

### Enhancements

- `Navigator`: focus screen wrapper instead of first tabbable element. ([#51816](https://github.com/WordPress/gutenberg/pull/51816)).
- `SelectControl`: Added option to set hidden options. ([#51545](https://github.com/WordPress/gutenberg/pull/51545))
- `RangeControl`: Add `__next40pxDefaultSize` prop to opt into the new 40px default size ([#49105](https://github.com/WordPress/gutenberg/pull/49105)).
- `Button`: Introduce `size` prop with `default`, `compact`, and `small` variants ([#51842](https://github.com/WordPress/gutenberg/pull/51842)).
Expand Down
14 changes: 14 additions & 0 deletions packages/components/src/navigator/navigator-screen/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,17 @@ The component accepts the following props:
The screen's path, matched against the current path stored in the navigator.

- Required: Yes

### `aria-label`: `string`

Additional text used to label the component for assistive technology.

- Required: No
- Default: `"Navigator screen"`

### `role`: `string`

The aria-role attributed to the screen.

- Required: No
- Default: `"region"`
58 changes: 41 additions & 17 deletions packages/components/src/navigator/navigator-screen/component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { css } from '@emotion/react';
/**
* WordPress dependencies
*/
import { focus } from '@wordpress/dom';
import {
useContext,
useEffect,
Expand All @@ -18,7 +17,7 @@ import {
useId,
} from '@wordpress/element';
import { useReducedMotion, useMergeRefs } from '@wordpress/compose';
import { isRTL } from '@wordpress/i18n';
import { __, isRTL } from '@wordpress/i18n';
import { escapeAttribute } from '@wordpress/escape-html';

/**
Expand Down Expand Up @@ -51,10 +50,14 @@ function UnconnectedNavigatorScreen(
forwardedRef: ForwardedRef< any >
) {
const screenId = useId();
const { children, className, path, ...otherProps } = useContextSystem(
props,
'NavigatorScreen'
);
const {
children,
className,
path,
'aria-label': ariaLabel = __( 'Navigator screen' ),
role = 'region',
...otherProps
} = useContextSystem( props, 'NavigatorScreen' );

const prefersReducedMotion = useReducedMotion();
const { location, match, addScreen, removeScreen } =
Expand All @@ -75,12 +78,33 @@ function UnconnectedNavigatorScreen(
const classes = useMemo(
() =>
cx(
css( {
// Ensures horizontal overflow is visually accessible.
overflowX: 'auto',
// In case the root has a height, it should not be exceeded.
maxHeight: '100%',
} ),
css`
/* Ensures horizontal overflow is visually accessible. */
overflow-x: auto;
/* In case the root has a height, it should not be exceeded. */
max-height: 100%;

&::after {
content: '';
position: absolute;
inset: 0;
z-index: 9999;
pointer-events: none;
}

&:focus {
outline: none;
}
Copy link
Contributor

@afercia afercia Jun 23, 2023

Choose a reason for hiding this comment

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

Focus indication must always be visible. Instead of resetting native outline, we should provide our own custom focus style.


&:focus::after {
box-shadow: inset 0 0 0
var( --wp-admin-border-width-focus )
var( --wp-admin-theme-color );

// Windows High Contrast mode will show this outline, but not the box-shadow.
outline: 2px solid transparent;
}
`,
className
),
[ className, cx ]
Expand Down Expand Up @@ -130,12 +154,9 @@ function UnconnectedNavigatorScreen(
}

// If the previous query didn't run or find any element to focus, fallback
// to the first tabbable element in the screen (or the screen itself).
// to the screen itself.
if ( ! elementToFocus ) {
const firstTabbable = (
focus.tabbable.find( wrapperRef.current ) as HTMLElement[]
)[ 0 ];
elementToFocus = firstTabbable ?? wrapperRef.current;
elementToFocus = wrapperRef.current;
}

locationRef.current.hasRestoredFocus = true;
Expand Down Expand Up @@ -211,6 +232,9 @@ function UnconnectedNavigatorScreen(
<motion.div
ref={ mergedWrapperRef }
className={ classes }
tabIndex={ -1 }
Copy link
Contributor

@afercia afercia Jun 23, 2023

Choose a reason for hiding this comment

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

Setting focus on an element with no ARIA role and no label is less than ideal. Although I do see aria-labels in the test, I don't see actual aria-label attributes set in the DOM.
The element that receives focus also needs an ARIA role.

role={ role }
aria-label={ ariaLabel }
{ ...otherProps }
{ ...animatedProps }
>
Expand Down
19 changes: 13 additions & 6 deletions packages/components/src/navigator/stories/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ const Template: ComponentStory< typeof NavigatorProvider > = ( {
style={ { ...style, height: '100vh', maxHeight: '450px' } }
{ ...props }
>
<NavigatorScreen path="/">
<NavigatorScreen path="/" aria-label="Home screen">
<Card>
<CardBody>
<p>This is the home screen.</p>
Expand Down Expand Up @@ -97,18 +97,22 @@ const Template: ComponentStory< typeof NavigatorProvider > = ( {
</Card>
</NavigatorScreen>

<NavigatorScreen path="/child">
<NavigatorScreen path="/child" aria-labelledby="child-screen-title">
<Card>
<CardBody>
<p>This is the child screen.</p>
{ /* eslint-disable-next-line no-restricted-syntax */ }
<h1 id="child-screen-title">Child screen</h1>
<NavigatorBackButton variant="secondary">
Go back
</NavigatorBackButton>
</CardBody>
</Card>
</NavigatorScreen>

<NavigatorScreen path="/overflow-child">
<NavigatorScreen
path="/overflow-child"
aria-label="Child screen with overflowing content"
>
<Card>
<CardBody>
<NavigatorBackButton variant="secondary">
Expand All @@ -134,7 +138,10 @@ const Template: ComponentStory< typeof NavigatorProvider > = ( {
</Card>
</NavigatorScreen>

<NavigatorScreen path="/stickies">
<NavigatorScreen
path="/stickies"
aria-label="Screen with sticky content"
>
<Card>
<CardHeader style={ getStickyStyles( { zIndex: 2 } ) }>
<NavigatorBackButton variant="secondary">
Expand Down Expand Up @@ -173,7 +180,7 @@ const Template: ComponentStory< typeof NavigatorProvider > = ( {
</Card>
</NavigatorScreen>

<NavigatorScreen path="/product/:id">
<NavigatorScreen path="/product/:id" aria-label="Product screen">
<ProductDetails />
</NavigatorScreen>
</NavigatorProvider>
Expand Down
Loading