Skip to content

Fix Nav Flyout Accessibility #8279

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

Merged
tlabaj merged 15 commits into from
Dec 8, 2022
Merged

Fix Nav Flyout Accessibility #8279

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
91d7907
fix(NavItem): disallow flyout and link props to both be defined on Na…
ruibrii Oct 21, 2022
2020766
docs(Navigation): remove links from NavItems with flyouts
ruibrii Oct 21, 2022
cb472c7
Merge branch 'patternfly:main' into 7781-nav-flyout-accessibility
ruibrii Oct 21, 2022
9d09055
Merge branch 'patternfly:main' into 7781-nav-flyout-accessibility
ruibrii Oct 24, 2022
06d7d62
fix(NavItem): remove aria-expanded prop
ruibrii Oct 24, 2022
435d2ae
fix(NavItem): use a button for Component if there is a flyout
ruibrii Nov 10, 2022
61aa96f
Merge branch 'patternfly:main' into 7781-nav-flyout-accessibility
ruibrii Nov 22, 2022
1699a7c
fix(NavItem): allow flyout to be opened on pressing Enter
ruibrii Nov 25, 2022
af8d64f
fix(MenuItem): disallow flyoutMenu and link props to both be defined
ruibrii Nov 28, 2022
5ec7ea6
docs(Navigation): remove links from MenuItems with flyouts
ruibrii Nov 28, 2022
de73623
fix(SelectOption): make props conditional to align with MenuItemProps
ruibrii Nov 30, 2022
c8cdd3a
fix(DropdownItem): make props conditional to align with MenuItemProps
ruibrii Nov 30, 2022
5abd569
fix(NavItem): revert conditional prop changes
ruibrii Dec 8, 2022
dbcd85b
fix(NavItem): fix opening nested MenuItem-based flyouts with keyboard
ruibrii Dec 8, 2022
3db3fa1
fix(NavItem): add a console.err message if to and flyout are used tog…
ruibrii Dec 8, 2022
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
22 changes: 17 additions & 5 deletions packages/react-core/src/components/Menu/MenuItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,13 @@ import { canUseDOM } from '../../helpers/util';
import { useIsomorphicLayoutEffect } from '../../helpers/useIsomorphicLayout';
import { GenerateId } from '../../helpers/GenerateId/GenerateId';

export interface MenuItemProps extends Omit<React.HTMLProps<HTMLLIElement>, 'onClick'> {
interface CommonMenuItemProps extends Omit<React.HTMLProps<HTMLLIElement>, 'onClick'> {
/** Content rendered inside the menu list item. */
children?: React.ReactNode;
/** Additional classes added to the menu list item */
className?: string;
/** Identifies the component in the Menu onSelect or onActionClick callback */
itemId?: any;
/** Target navigation link */
to?: string;
/** @beta Flag indicating the item has a checkbox */
hasCheck?: boolean;
/** Flag indicating whether the item is active */
Expand Down Expand Up @@ -54,8 +52,6 @@ export interface MenuItemProps extends Omit<React.HTMLProps<HTMLLIElement>, 'onC
isFocused?: boolean;
/** Flag indicating the item is in danger state */
isDanger?: boolean;
/** @beta Flyout menu */
flyoutMenu?: React.ReactElement;
/** @beta Callback function when mouse leaves trigger */
onShowFlyout?: (event?: any) => void;
/** @beta Drilldown menu of the item. Should be a Menu or DrilldownMenu type. */
Expand All @@ -70,6 +66,22 @@ export interface MenuItemProps extends Omit<React.HTMLProps<HTMLLIElement>, 'onC
innerRef?: React.Ref<HTMLAnchorElement | HTMLButtonElement>;
}

type ConditionalMenuItemProps =
| {
/** Target navigation link */
to?: string;
/** @beta Flyout menu. Disallowed if nav link is defined */
flyoutMenu?: never;
}
| {
/** Target navigation link. Disallowed if flyoutMenu is defined */
to?: never;
/** @beta Flyout menu */
flyoutMenu?: React.ReactElement;
};
Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman Dec 6, 2022

Choose a reason for hiding this comment

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

@tlabaj Would this change be breaking? I know it's an a11y bug, but I feel like there's a decent chance that there are consumers who followed the example and would need to make changes for the proposed type change here. Also I know that we have the beta tag on flyoutMenu, but we don't have it on to.

Genuine question as I'm not sure. A conditional type like this is a really neat idea though.

Copy link
Contributor

@thatblindgeye thatblindgeye Dec 7, 2022

Choose a reason for hiding this comment

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

Another thing that was noticed in our React huddle is that by doing this the to and flyoutMenu props are no longer rendered in the props table on the React page for the component. If anything, perhaps we can put the implementation proposed here on the backburner and discuss it further down the line?

Copy link
Contributor

@tlabaj tlabaj Dec 8, 2022

Choose a reason for hiding this comment

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

yes. This would be a breaking change.
I would probably not recommend defining type this way and instead have 2 separate types and a prop that could be one or the other.
We would need to wait for the breaking change release to do this.


export type MenuItemProps = CommonMenuItemProps & ConditionalMenuItemProps;

const FlyoutContext = React.createContext({
direction: 'right' as 'left' | 'right'
});
Expand Down
32 changes: 23 additions & 9 deletions packages/react-core/src/components/Nav/NavItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,13 @@ import { useOUIAProps, OUIAProps } from '../../helpers';
import { Popper } from '../../helpers/Popper/Popper';
import AngleRightIcon from '@patternfly/react-icons/dist/esm/icons/angle-right-icon';

export interface NavItemProps extends Omit<React.HTMLProps<HTMLAnchorElement>, 'onClick'>, OUIAProps {
interface CommonNavItemProps extends Omit<React.HTMLProps<HTMLAnchorElement>, 'onClick'>, OUIAProps {
/** Content rendered inside the nav item. */
children?: React.ReactNode;
/** Whether to set className on children when React.isValidElement(children) */
styleChildren?: boolean;
/** Additional classes added to the nav item */
className?: string;
/** Target navigation link */
to?: string;
/** Flag indicating whether the item is active */
isActive?: boolean;
/** Group identifier, will be returned with the onToggle and onSelect callback passed to the Nav component */
Expand All @@ -28,8 +26,6 @@ export interface NavItemProps extends Omit<React.HTMLProps<HTMLAnchorElement>, '
onClick?: NavSelectClickHandler;
/** Component used to render NavItems if React.isValidElement(children) is false */
component?: React.ReactNode;
/** Flyout of a nav item. This should be a Menu component. */
flyout?: React.ReactElement;
/** Callback when flyout is opened or closed */
onShowFlyout?: () => void;
/** @beta Opt-in for updated popper that does not use findDOMNode. */
Expand All @@ -42,6 +38,22 @@ export interface NavItemProps extends Omit<React.HTMLProps<HTMLAnchorElement>, '
ouiaSafe?: boolean;
}

type ConditionalNavItemProps =
| {
/** Target navigation link */
to?: string;
/** Flyout of a nav item. Disallowed if nav link is defined */
flyout?: never;
}
| {
/** Target navigation link. Disallowed if flyout is defined */
to?: never;
/** Flyout of a nav item. This should be a Menu component. */
flyout?: React.ReactNode;
};

export type NavItemProps = CommonNavItemProps & ConditionalNavItemProps;

export const NavItem: React.FunctionComponent<NavItemProps> = ({
children,
styleChildren = true,
Expand All @@ -68,8 +80,8 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
const ref = React.useRef<HTMLLIElement>();
const flyoutVisible = ref === flyoutRef;
const popperRef = React.useRef<HTMLDivElement>();
const Component = component as any;
const hasFlyout = flyout !== undefined;
const Component = hasFlyout ? 'button' : (component as any);

const showFlyout = (show: boolean, override?: boolean) => {
if ((!flyoutVisible || override) && show) {
Expand Down Expand Up @@ -110,7 +122,7 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
return;
}

if (key === ' ' || key === 'ArrowRight') {
if (key === ' ' || key === 'Enter' || key === 'ArrowRight') {
event.stopPropagation();
event.preventDefault();
if (!flyoutVisible) {
Expand Down Expand Up @@ -165,6 +177,8 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
'aria-expanded': flyoutVisible
};

const tabIndex = isNavOpen ? null : -1;

const renderDefaultLink = (context: any): React.ReactNode => {
const preventLinkDefault = preventDefault || !to;
return (
Expand All @@ -178,7 +192,7 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
className
)}
aria-current={isActive ? 'page' : null}
tabIndex={isNavOpen ? null : '-1'}
tabIndex={tabIndex}
{...(hasFlyout && { ...ariaFlyoutProps })}
{...props}
>
Expand All @@ -195,7 +209,7 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
...(styleChildren && {
className: css(styles.navLink, isActive && styles.modifiers.current, child.props && child.props.className)
}),
tabIndex: child.props.tabIndex || isNavOpen ? null : -1,
tabIndex: child.props.tabIndex || tabIndex,
children: hasFlyout ? (
<React.Fragment>
{child.props.children}
Expand Down
15 changes: 2 additions & 13 deletions packages/react-core/src/components/Nav/examples/NavFlyout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,7 @@ export const NavFlyout: React.FunctionComponent = () => {
<Menu key={depth} containsFlyout isNavFlyout id={`nav-flyout-menu-${depth}`} onSelect={onMenuSelect}>
<MenuContent>
<MenuList>
<MenuItem
onClick={onMenuItemClick}
flyoutMenu={children}
itemId={`nav-flyout-next-menu-${depth}`}
to={`#next-menu-link-${depth}`}
>
<MenuItem onClick={onMenuItemClick} flyoutMenu={children} itemId={`nav-flyout-next-menu-${depth}`}>
Next menu
</MenuItem>
{Array.apply(0, Array(numFlyouts - depth)).map((_item, index: number) => (
Expand All @@ -38,12 +33,7 @@ export const NavFlyout: React.FunctionComponent = () => {
Menu {depth} item {index}
</MenuItem>
))}
<MenuItem
onClick={onMenuItemClick}
flyoutMenu={children}
itemId={`nav-flyout-next-menu-2-${depth}`}
to={`#next-menu-2-link-${depth}`}
>
<MenuItem onClick={onMenuItemClick} flyoutMenu={children} itemId={`nav-flyout-next-menu-2-${depth}`}>
Next menu
</MenuItem>
</MenuList>
Expand Down Expand Up @@ -81,7 +71,6 @@ export const NavFlyout: React.FunctionComponent = () => {
preventDefault
flyout={curFlyout}
id="nav-flyout-default-link-3"
to="#nav-flyout-default-link-3"
itemId="nav-flyout-default-link-3"
isActive={activeItem === 'nav-flyout-default-link-3'}
>
Expand Down
4 changes: 2 additions & 2 deletions packages/react-core/src/demos/Nav.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1463,15 +1463,15 @@ class VerticalPage extends React.Component {
<Menu key={depth} containsFlyout isNavFlyout id={`menu-${depth}`} onSelect={this.onMenuSelect}>
<MenuContent>
<MenuList>
<MenuItem flyoutMenu={children} itemId={`next-menu-${depth}`} to={`#menu-link-${depth}`}>
<MenuItem flyoutMenu={children} itemId={`next-menu-${depth}`}>
Additional settings
</MenuItem>
{[...Array(numFlyouts - depth).keys()].map(j => (
<MenuItem key={`${depth}-${j}`} itemId={`${depth}-${j}`} to={`#menu-link-${depth}-${j}`}>
Settings menu {depth} item {j}
</MenuItem>
))}
<MenuItem flyoutMenu={children} itemId={`next-menu-2-${depth}`} to={`#second-menu-link-${depth}`}>
<MenuItem flyoutMenu={children} itemId={`next-menu-2-${depth}`}>
Additional settings
</MenuItem>
</MenuList>
Expand Down
18 changes: 17 additions & 1 deletion packages/react-core/src/next/components/Dropdown/DropdownItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { css } from '@patternfly/react-styles';
import { MenuItemProps, MenuItem } from '../../../components/Menu';
import { useOUIAProps, OUIAProps } from '../../../helpers';

export interface DropdownItemProps extends Omit<MenuItemProps, 'ref'>, OUIAProps {
interface CommonDropdownItemProps extends Omit<MenuItemProps, 'ref'>, OUIAProps {
/** Anything which can be rendered in a dropdown item */
children?: React.ReactNode;
/** Classes applied to root element of dropdown item */
Expand All @@ -16,6 +16,22 @@ export interface DropdownItemProps extends Omit<MenuItemProps, 'ref'>, OUIAProps
ouiaSafe?: boolean;
}

type ConditionalDropdownItemProps =
| {
/** Target navigation link */
to?: string;
/** Flyout menu. Disallowed if nav link is defined */
flyoutMenu?: never;
}
| {
/** Target navigation link. Disallowed if flyoutMenu is defined */
to?: never;
/** Flyout menu */
flyoutMenu?: React.ReactElement;
};

export type DropdownItemProps = CommonDropdownItemProps & ConditionalDropdownItemProps;

Copy link
Contributor Author

@ruibrii ruibrii Nov 30, 2022

Choose a reason for hiding this comment

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

Had to make this change so that the props would agree with MenuItemProps. Is there a better way to achieve this?

export const DropdownItem: React.FunctionComponent<MenuItemProps> = ({
children,
className,
Expand Down
18 changes: 17 additions & 1 deletion packages/react-core/src/next/components/Select/SelectOption.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React from 'react';
import { css } from '@patternfly/react-styles';
import { MenuItemProps, MenuItem } from '../../../components/Menu';

export interface SelectOptionProps extends Omit<MenuItemProps, 'ref'> {
interface CommonSelectOptionProps extends Omit<MenuItemProps, 'ref'> {
/** Anything which can be rendered in a select option */
children?: React.ReactNode;
/** Classes applied to root element of select option */
Expand All @@ -19,6 +19,22 @@ export interface SelectOptionProps extends Omit<MenuItemProps, 'ref'> {
isFocused?: boolean;
}

type ConditionalSelectOptionProps =
| {
/** Target navigation link */
to?: string;
/** Flyout menu. Disallowed if nav link is defined */
flyoutMenu?: never;
}
| {
/** Target navigation link. Disallowed if flyoutMenu is defined */
to?: never;
/** Flyout menu */
flyoutMenu?: React.ReactElement;
};

export type SelectOptionProps = CommonSelectOptionProps & ConditionalSelectOptionProps;

Copy link
Contributor Author

@ruibrii ruibrii Nov 30, 2022

Choose a reason for hiding this comment

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

Same situation as DropdownItem changes

export const SelectOption: React.FunctionComponent<MenuItemProps> = ({
children,
className,
Expand Down
8 changes: 1 addition & 7 deletions packages/react-integration/demo-app-ts/src/components/demos/NavDemo/NavDemo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,13 +384,7 @@ export class NavDemo extends Component {
<NavItem id="flyout-link2" to="#flyout-link2" itemId={1} isActive={flyoutActiveItem === 1}>
Link 2
</NavItem>
<NavItem
flyout={curFlyout}
id="flyout-link3"
to="#flyout-link3"
itemId={2}
isActive={flyoutActiveItem === 2}
>
<NavItem flyout={curFlyout} id="flyout-link3" itemId={2} isActive={flyoutActiveItem === 2}>
Link 3
</NavItem>
<NavItem id="flyout-link4" to="#flyout-link4" itemId={3} isActive={flyoutActiveItem === 3}>
Expand Down