Skip to content

Commit 09ac7d4

Browse files
ruibriiruibrii
authored
Fix Nav Flyout Accessibility (#8279)
* fix(NavItem): disallow flyout and link props to both be defined on NavItem * docs(Navigation): remove links from NavItems with flyouts * fix(NavItem): remove aria-expanded prop * fix(NavItem): use a button for Component if there is a flyout * fix(NavItem): allow flyout to be opened on pressing Enter * fix(MenuItem): disallow flyoutMenu and link props to both be defined * docs(Navigation): remove links from MenuItems with flyouts * fix(SelectOption): make props conditional to align with MenuItemProps * fix(DropdownItem): make props conditional to align with MenuItemProps * fix(NavItem): revert conditional prop changes * fix(NavItem): fix opening nested MenuItem-based flyouts with keyboard * fix(NavItem): add a console.err message if to and flyout are used together
1 parent d0a31d9 commit 09ac7d4

5 files changed

Lines changed: 25 additions & 35 deletions

File tree

‎packages/react-core/src/components/Menu/MenuItem.tsx‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ export interface MenuItemProps extends Omit<React.HTMLProps<HTMLLIElement>, 'onC
2222
className?: string;
2323
/** Identifies the component in the Menu onSelect or onActionClick callback */
2424
itemId?: any;
25-
/** Target navigation link */
25+
/** Target navigation link. Should not be used if the flyout prop is defined. */
2626
to?: string;
2727
/** @beta Flag indicating the item has a checkbox */
2828
hasCheck?: boolean;
@@ -54,7 +54,7 @@ export interface MenuItemProps extends Omit<React.HTMLProps<HTMLLIElement>, 'onC
5454
isFocused?: boolean;
5555
/** Flag indicating the item is in danger state */
5656
isDanger?: boolean;
57-
/** @beta Flyout menu */
57+
/** @beta Flyout menu. Should not be used if the to prop is defined. */
5858
flyoutMenu?: React.ReactElement;
5959
/** @beta Callback function when mouse leaves trigger */
6060
onShowFlyout?: (event?: any) => void;
@@ -202,6 +202,7 @@ const MenuItemBase: React.FunctionComponent<MenuItemProps> = ({
202202

203203
if (key === ' ' || key === 'Enter' || key === 'ArrowRight' || type === 'click') {
204204
event.stopPropagation();
205+
event.preventDefault();
205206
if (!flyoutVisible) {
206207
showFlyout(true);
207208
setFlyoutTarget(target as HTMLElement);

‎packages/react-core/src/components/Nav/NavItem.tsx‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ export interface NavItemProps extends Omit<React.HTMLProps<HTMLAnchorElement>, '
1414
styleChildren?: boolean;
1515
/** Additional classes added to the nav item */
1616
className?: string;
17-
/** Target navigation link */
17+
/** Target navigation link. Should not be used if the flyout prop is defined. */
1818
to?: string;
1919
/** Flag indicating whether the item is active */
2020
isActive?: boolean;
@@ -28,7 +28,7 @@ export interface NavItemProps extends Omit<React.HTMLProps<HTMLAnchorElement>, '
2828
onClick?: NavSelectClickHandler;
2929
/** Component used to render NavItems if React.isValidElement(children) is false */
3030
component?: React.ReactNode;
31-
/** Flyout of a nav item. This should be a Menu component. */
31+
/** Flyout of a nav item. This should be a Menu component. Should not be used if the to prop is defined. */
3232
flyout?: React.ReactElement;
3333
/** Callback when flyout is opened or closed */
3434
onShowFlyout?: () => void;
@@ -68,8 +68,14 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
6868
const ref = React.useRef<HTMLLIElement>();
6969
const flyoutVisible = ref === flyoutRef;
7070
const popperRef = React.useRef<HTMLDivElement>();
71-
const Component = component as any;
7271
const hasFlyout = flyout !== undefined;
72+
const Component = hasFlyout ? 'button' : (component as any);
73+
74+
// A NavItem should not be both a link and a flyout
75+
if (to && hasFlyout) {
76+
// eslint-disable-next-line no-console
77+
console.error('NavItem cannot have both "to" and "flyout" props.');
78+
}
7379

7480
const showFlyout = (show: boolean, override?: boolean) => {
7581
if ((!flyoutVisible || override) && show) {
@@ -106,11 +112,7 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
106112
const key = event.key;
107113
const target = event.target as HTMLElement;
108114

109-
if (!(popperRef?.current?.contains(target) || (hasFlyout && ref?.current?.contains(target)))) {
110-
return;
111-
}
112-
113-
if (key === ' ' || key === 'ArrowRight') {
115+
if ((key === ' ' || key === 'Enter' || key === 'ArrowRight') && hasFlyout && ref?.current?.contains(target)) {
114116
event.stopPropagation();
115117
event.preventDefault();
116118
if (!flyoutVisible) {
@@ -119,7 +121,9 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
119121
}
120122
}
121123

122-
if (key === 'Escape' || key === 'ArrowLeft') {
124+
// We only want the NavItem to handle closing a flyout menu if only the first level flyout is open.
125+
// Otherwise, MenuItem should handle closing its flyouts
126+
if ((key === 'Escape' || key === 'ArrowLeft') && popperRef?.current?.querySelectorAll('.pf-c-menu').length === 1) {
123127
if (flyoutVisible) {
124128
event.stopPropagation();
125129
event.preventDefault();
@@ -165,6 +169,8 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
165169
'aria-expanded': flyoutVisible
166170
};
167171

172+
const tabIndex = isNavOpen ? null : -1;
173+
168174
const renderDefaultLink = (context: any): React.ReactNode => {
169175
const preventLinkDefault = preventDefault || !to;
170176
return (
@@ -178,7 +184,7 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
178184
className
179185
)}
180186
aria-current={isActive ? 'page' : null}
181-
tabIndex={isNavOpen ? null : '-1'}
187+
tabIndex={tabIndex}
182188
{...(hasFlyout && { ...ariaFlyoutProps })}
183189
{...props}
184190
>
@@ -195,7 +201,7 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
195201
...(styleChildren && {
196202
className: css(styles.navLink, isActive && styles.modifiers.current, child.props && child.props.className)
197203
}),
198-
tabIndex: child.props.tabIndex || isNavOpen ? null : -1,
204+
tabIndex: child.props.tabIndex || tabIndex,
199205
children: hasFlyout ? (
200206
<React.Fragment>
201207
{child.props.children}

‎packages/react-core/src/components/Nav/examples/NavFlyout.tsx‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,7 @@ export const NavFlyout: React.FunctionComponent = () => {
1919
<Menu key={depth} containsFlyout isNavFlyout id={`nav-flyout-menu-${depth}`} onSelect={onMenuSelect}>
2020
<MenuContent>
2121
<MenuList>
22-
<MenuItem
23-
onClick={onMenuItemClick}
24-
flyoutMenu={children}
25-
itemId={`nav-flyout-next-menu-${depth}`}
26-
to={`#next-menu-link-${depth}`}
27-
>
22+
<MenuItem onClick={onMenuItemClick} flyoutMenu={children} itemId={`nav-flyout-next-menu-${depth}`}>
2823
Next menu
2924
</MenuItem>
3025
{Array.apply(0, Array(numFlyouts - depth)).map((_item, index: number) => (
@@ -38,12 +33,7 @@ export const NavFlyout: React.FunctionComponent = () => {
3833
Menu {depth} item {index}
3934
</MenuItem>
4035
))}
41-
<MenuItem
42-
onClick={onMenuItemClick}
43-
flyoutMenu={children}
44-
itemId={`nav-flyout-next-menu-2-${depth}`}
45-
to={`#next-menu-2-link-${depth}`}
46-
>
36+
<MenuItem onClick={onMenuItemClick} flyoutMenu={children} itemId={`nav-flyout-next-menu-2-${depth}`}>
4737
Next menu
4838
</MenuItem>
4939
</MenuList>
@@ -81,7 +71,6 @@ export const NavFlyout: React.FunctionComponent = () => {
8171
preventDefault
8272
flyout={curFlyout}
8373
id="nav-flyout-default-link-3"
84-
to="#nav-flyout-default-link-3"
8574
itemId="nav-flyout-default-link-3"
8675
isActive={activeItem === 'nav-flyout-default-link-3'}
8776
>

‎packages/react-core/src/demos/Nav.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1463,15 +1463,15 @@ class VerticalPage extends React.Component {
14631463
<Menu key={depth} containsFlyout isNavFlyout id={`menu-${depth}`} onSelect={this.onMenuSelect}>
14641464
<MenuContent>
14651465
<MenuList>
1466-
<MenuItem flyoutMenu={children} itemId={`next-menu-${depth}`} to={`#menu-link-${depth}`}>
1466+
<MenuItem flyoutMenu={children} itemId={`next-menu-${depth}`}>
14671467
Additional settings
14681468
</MenuItem>
14691469
{[...Array(numFlyouts - depth).keys()].map(j => (
14701470
<MenuItem key={`${depth}-${j}`} itemId={`${depth}-${j}`} to={`#menu-link-${depth}-${j}`}>
14711471
Settings menu {depth} item {j}
14721472
</MenuItem>
14731473
))}
1474-
<MenuItem flyoutMenu={children} itemId={`next-menu-2-${depth}`} to={`#second-menu-link-${depth}`}>
1474+
<MenuItem flyoutMenu={children} itemId={`next-menu-2-${depth}`}>
14751475
Additional settings
14761476
</MenuItem>
14771477
</MenuList>

‎packages/react-integration/demo-app-ts/src/components/demos/NavDemo/NavDemo.tsx‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -384,13 +384,7 @@ export class NavDemo extends Component {
384384
<NavItem id="flyout-link2" to="#flyout-link2" itemId={1} isActive={flyoutActiveItem === 1}>
385385
Link 2
386386
</NavItem>
387-
<NavItem
388-
flyout={curFlyout}
389-
id="flyout-link3"
390-
to="#flyout-link3"
391-
itemId={2}
392-
isActive={flyoutActiveItem === 2}
393-
>
387+
<NavItem flyout={curFlyout} id="flyout-link3" itemId={2} isActive={flyoutActiveItem === 2}>
394388
Link 3
395389
</NavItem>
396390
<NavItem id="flyout-link4" to="#flyout-link4" itemId={3} isActive={flyoutActiveItem === 3}>

0 commit comments

Comments
 (0)