diff --git a/UNRELEASED.md b/UNRELEASED.md index 91bad086b15..36ba8d844a0 100644 --- a/UNRELEASED.md +++ b/UNRELEASED.md @@ -23,6 +23,7 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f - Updated `Collapsible` to be a functional component ([#3779](https://github.com/Shopify/polaris-react/pull/3779)) - Coverted `TooltipOverlay` to a functional component ([#3631](https://github.com/Shopify/polaris-react/pull/3631)) - New `ariaDescribedBy` prop for `Button` ([#3664](https://github.com/Shopify/polaris-react/pull/3686)) +- Changed the way sub navigation menus are rendered for improved accessibility ([#3661](https://github.com/Shopify/polaris-react/pull/3661)) ### Bug fixes diff --git a/playground/DetailsPage.tsx b/playground/DetailsPage.tsx index f85982cc58c..15d05511382 100644 --- a/playground/DetailsPage.tsx +++ b/playground/DetailsPage.tsx @@ -239,6 +239,35 @@ export function DetailsPage() { }, matches: navItemActive === 'orders', url: '#', + subNavigationItems: [ + { + label: 'All orders', + onClick: () => { + toggleIsLoading(); + setNavItemActive('all-orders'); + }, + matches: navItemActive.includes('orders'), + url: '#', + }, + { + url: '#', + label: 'Drafts', + onClick: () => { + toggleIsLoading(); + setNavItemActive('drafts'); + }, + matches: navItemActive === 'drafts', + }, + { + url: '#', + label: 'Abandoned checkouts', + onClick: () => { + toggleIsLoading(); + setNavItemActive('abandoned'); + }, + matches: navItemActive === 'abandoned', + }, + ], }, { label: 'Products', @@ -261,21 +290,21 @@ export function DetailsPage() { }, { url: '#', - label: 'Drafts', + label: 'Inventory', onClick: () => { toggleIsLoading(); - setNavItemActive('drafts'); + setNavItemActive('inventory'); }, - matches: navItemActive === 'drafts', + matches: navItemActive === 'inventory', }, { url: '#', - label: 'Abandoned checkouts', + label: 'Transfers', onClick: () => { toggleIsLoading(); - setNavItemActive('abandoned'); + setNavItemActive('transfers'); }, - matches: navItemActive === 'abandoned', + matches: navItemActive === 'transfers', }, ], }, diff --git a/src/components/Navigation/Navigation.scss b/src/components/Navigation/Navigation.scss index d9ca12eb8be..c640d421ebb 100644 --- a/src/components/Navigation/Navigation.scss +++ b/src/components/Navigation/Navigation.scss @@ -265,10 +265,13 @@ $disabled-fade: 0.6; $secondary-item-font-size: rem(15px); .SecondaryNavigation { flex-basis: 100%; - margin-bottom: spacing(tight); margin-left: nav(icon-size) + spacing(loose); overflow-x: var(--p-override-visible, hidden); + &.isExpanded { + margin-bottom: spacing(tight); + } + .Navigation-newDesignLanguage & { margin-left: 0; } diff --git a/src/components/Navigation/components/Item/Item.tsx b/src/components/Navigation/components/Item/Item.tsx index 57357ba29af..52b813766ff 100644 --- a/src/components/Navigation/components/Item/Item.tsx +++ b/src/components/Navigation/components/Item/Item.tsx @@ -16,6 +16,7 @@ import {Indicator} from '../../../Indicator'; import {UnstyledLink} from '../../../UnstyledLink'; import {useI18n} from '../../../../utilities/i18n'; import {useMediaQuery} from '../../../../utilities/media-query'; +import {useUniqueId} from '../../../../utilities/unique-id'; import styles from '../../Navigation.scss'; import {Secondary} from './components'; @@ -83,6 +84,7 @@ export function Item({ }: ItemProps) { const i18n = useI18n(); const {isNavigationCollapsed} = useMediaQuery(); + const secondaryNavigationId = useUniqueId('SecondaryNavigation'); const {location, onNavigationDismiss} = useContext(NavigationContext); const [expanded, setExpanded] = useState(false); const [keyFocused, setKeyFocused] = useState(false); @@ -230,19 +232,20 @@ export function Item({ let secondaryNavigationMarkup: ReactNode = null; - if (subNavigationItems.length > 0 && showExpanded) { + if (subNavigationItems.length > 0) { const longestMatch = matchingSubNavigationItems.sort( ({url: firstUrl}, {url: secondUrl}) => secondUrl.length - firstUrl.length, )[0]; const SecondaryNavigationClassName = classNames( styles.SecondaryNavigation, + showExpanded && styles.isExpanded, !icon && styles['SecondaryNavigation-noIcon'], ); secondaryNavigationMarkup = (
- + {subNavigationItems.map((item) => { const {label, ...rest} = item; return ( @@ -277,6 +280,11 @@ export function Item({ onClick={getClickHandler(onClick)} onKeyUp={handleKeyUp} onBlur={handleBlur} + {...normalizeAriaAttributes( + secondaryNavigationId, + subNavigationItems.length > 0, + showExpanded, + )} > {itemContentMarkup} @@ -386,3 +394,16 @@ function matchStateForItem( : safeStartsWith(location, url); return matchesUrl ? MatchState.MatchUrl : MatchState.NoMatch; } + +function normalizeAriaAttributes( + controlId: string, + hasSubMenu: boolean, + expanded: boolean, +) { + return hasSubMenu + ? { + 'aria-expanded': expanded, + 'aria-controls': controlId, + } + : undefined; +} diff --git a/src/components/Navigation/components/Item/components/Secondary/Secondary.tsx b/src/components/Navigation/components/Item/components/Secondary/Secondary.tsx index 9ea4691e593..b35b0a9041a 100644 --- a/src/components/Navigation/components/Item/components/Secondary/Secondary.tsx +++ b/src/components/Navigation/components/Item/components/Secondary/Secondary.tsx @@ -7,12 +7,17 @@ import styles from '../../../../Navigation.scss'; interface SecondaryProps { expanded: boolean; children?: React.ReactNode; + id?: string; } -export function Secondary({children, expanded}: SecondaryProps) { - const id = useUniqueId('SecondaryNavigation'); +export function Secondary({id, children, expanded}: SecondaryProps) { + const uid = useUniqueId('SecondaryNavigation'); return ( - +
    {children}
); diff --git a/src/components/Navigation/components/Item/components/Secondary/tests/Secondary.test.tsx b/src/components/Navigation/components/Item/components/Secondary/tests/Secondary.test.tsx index 6bc425410f1..372326a324e 100644 --- a/src/components/Navigation/components/Item/components/Secondary/tests/Secondary.test.tsx +++ b/src/components/Navigation/components/Item/components/Secondary/tests/Secondary.test.tsx @@ -1,12 +1,43 @@ import React from 'react'; -// eslint-disable-next-line no-restricted-imports -import {mountWithAppProvider} from 'test-utilities/legacy'; +import {mountWithApp} from 'test-utilities'; +import {Collapsible} from '../../../../../../Collapsible'; import {Secondary} from '../Secondary'; describe('Secondary()', () => { - it('mounts', () => { - const secondary = mountWithAppProvider(); - expect(secondary.exists()).toBe(true); + it('passes a default id to Collapsible', () => { + const component = mountWithApp(); + expect(component).toContainReactComponent(Collapsible, { + id: 'PolarisSecondaryNavigation1', + }); + }); + + it('passes a custom id to Collapsible when provided', () => { + const component = mountWithApp( + , + ); + expect(component).toContainReactComponent(Collapsible, { + id: 'CustomSecondaryId', + }); + }); + + it('adds custom transition props to Collapsible', () => { + const component = mountWithApp(); + expect(component).toContainReactComponent(Collapsible, { + transition: { + duration: expect.any(String), + timingFunction: expect.any(String), + }, + }); + }); + + it('passes expanded to Collapsible', () => { + const component = mountWithApp(); + expect(component).toContainReactComponent(Collapsible, {open: true}); + }); + + it('renders an unorders list for its children', () => { + const component = mountWithApp(); + expect(component).toContainReactComponent('ul'); }); }); diff --git a/src/components/Navigation/components/Item/tests/Item.test.tsx b/src/components/Navigation/components/Item/tests/Item.test.tsx index b6a793cf248..f4ba4369609 100644 --- a/src/components/Navigation/components/Item/tests/Item.test.tsx +++ b/src/components/Navigation/components/Item/tests/Item.test.tsx @@ -2,12 +2,12 @@ import React from 'react'; import {PlusMinor} from '@shopify/polaris-icons'; import {matchMedia} from '@shopify/jest-dom-mocks'; import {Icon, UnstyledLink, Indicator, Badge} from 'components'; -// eslint-disable-next-line no-restricted-imports -import {trigger, mountWithAppProvider} from 'test-utilities/legacy'; +import {mountWithApp} from 'test-utilities'; import {NavigationContext} from '../../../context'; import {Item, ItemProps} from '../Item'; import {Secondary} from '../components'; +import {Key} from '../../../../../types'; describe('', () => { beforeEach(() => { @@ -42,36 +42,36 @@ describe('', () => { const mediaAddListener = spy.mock.calls[0][0]; matchMedia.setMedia(() => ({matches: true})); - trigger(item.find(UnstyledLink).first(), 'onClick', { + + item.find(UnstyledLink)!.trigger('onClick', { preventDefault: jest.fn(), currentTarget: { getAttribute: () => '/admin/orders', }, }); - expect(item.find(Secondary).prop('expanded')).toBe(true); + expect(item).toContainReactComponent(Secondary, {expanded: true}); matchMedia.setMedia(() => ({matches: false})); mediaAddListener(); - item.update(); + item.forceUpdate(); - expect(item.find(Secondary).exists()).toBe(false); + expect(item).toContainReactComponent(Secondary, {expanded: false}); }); it('remains expanded on resize when navigationBarCollapsed and location matches', () => { const item = itemForLocation('/admin/orders'); matchMedia.setMedia(() => ({matches: true})); - trigger(item.find(UnstyledLink).first(), 'onClick', { + item!.find(UnstyledLink)!.trigger('onClick', { preventDefault: jest.fn(), currentTarget: { getAttribute: () => '/admin/orders', }, }); - - expect(item.find(Secondary).prop('expanded')).toBe(true); + expect(item).toContainReactComponent(Secondary, {expanded: true}); matchMedia.setMedia(() => ({matches: false})); - expect(item.find(Secondary).prop('expanded')).toBe(true); + expect(item).toContainReactComponent(Secondary, {expanded: true}); }); describe('renders', () => { @@ -93,15 +93,13 @@ describe('', () => { }, ); - const button = item.find('button'); - expect(button.exists()).toBe(true); + expect(item).toContainReactComponent('button'); }); it('renders an UnstyledLink when url is provided', () => { const item = itemForLocation('/admin/orders'); - const link = item.find(UnstyledLink); - expect(link.exists()).toBe(true); + expect(item).toContainReactComponent(UnstyledLink); }); it('renders a small badge with new status if the prop is provided with a string', () => { @@ -109,7 +107,7 @@ describe('', () => { , ); - expect(item.find(Badge).props()).toMatchObject({ + expect(item).toContainReactComponent(Badge, { status: 'new', size: 'small', children: '1', @@ -121,17 +119,16 @@ describe('', () => { Custom badge} />, ); - expect(item.find(Badge).text()).toContain('Custom badge'); + expect(item.find(Badge)).toContainReactText('Custom badge'); }); it('renders a single new badge even if a badge prop is also provided', () => { const item = mountWithNavigationProvider( Custom badge} new />, ); - const badge = item.find(Badge); - expect(badge).toHaveLength(1); - expect(badge.text()).toContain('New'); + expect(item).toContainReactComponentTimes(Badge, 1); + expect(item.find(Badge)).toContainReactText('New'); }); }); @@ -139,36 +136,63 @@ describe('', () => { it('renders expanded when given url is a perfect match for location', () => { const item = itemForLocation('/admin/orders'); - const secondary = item.find(Secondary); - expect(secondary.exists()).toBe(true); + expect(item).toContainReactComponent(Secondary); }); it('renders expanded when a url is a startsWith match for location', () => { const item = itemForLocation('/admin/orders?foo=bar'); - const secondary = item.find(Secondary); - expect(secondary.exists()).toBe(true); + expect(item).toContainReactComponent(Secondary); }); it('renders expanded when a child is a perfect match for location', () => { const item = itemForLocation('/admin/draft_orders'); - const secondary = item.find(Secondary); - expect(secondary.exists()).toBe(true); + expect(item).toContainReactComponent(Secondary); }); it('renders expanded when a child is a startsWith match for location', () => { const item = itemForLocation('/admin/draft_orders?foo=bar'); - const secondary = item.find(Secondary); - expect(secondary.exists()).toBe(true); + expect(item).toContainReactComponent(Secondary); }); it('does not render expanded when parent and children both have no match on the location', () => { const item = itemForLocation('/admin/notARealRoute'); - const secondary = item.find(Secondary); - expect(secondary.exists()).toBe(false); + expect(item).toContainReactComponent(Secondary, {expanded: false}); + }); + + it('sets aria labels', () => { + const item = itemForLocation('/admin/notARealRoute'); + + expect(item).toContainReactComponent('a', { + 'aria-expanded': false, + 'aria-controls': 'PolarisSecondaryNavigation1', + }); + }); + + it('sets aria-expanded to true when item with subNavItems is expanded', () => { + const item = mountWithNavigationProvider( + , + { + location: '/admin/orders', + }, + ); + + expect(item).toContainReactComponent(UnstyledLink, { + 'aria-expanded': true, + }); }); }); @@ -176,22 +200,19 @@ describe('', () => { it('renders expanded when given url is a perfect match for location', () => { const item = itemForLocation('/admin/orders', {exactMatch: true}); - const secondary = item.find(Secondary); - expect(secondary.exists()).toBe(true); + expect(item).toContainReactComponent(Secondary); }); it('does not render expanded when no exact match on url', () => { const item = itemForLocation('/admin/orders/1', {exactMatch: true}); - const secondary = item.find(Secondary); - expect(secondary.exists()).toBe(false); + expect(item).toContainReactComponent(Secondary, {expanded: false}); }); it('still renders expanded when there is a match on url for one of it`s children', () => { const item = itemForLocation('/admin/draft_orders', {exactMatch: true}); - const secondary = item.find(Secondary); - expect(secondary.exists()).toBe(true); + expect(item).toContainReactComponent(Secondary); }); }); @@ -203,7 +224,10 @@ describe('', () => { location: 'bar', }, ); - expect(item.find(Icon).prop('source')).toBe(PlusMinor); + + expect(item).toContainReactComponent(Icon, { + source: PlusMinor, + }); }); it('delegates label to ', () => { @@ -214,7 +238,7 @@ describe('', () => { }, ); - expect(item.find(UnstyledLink).text()).toBe('baz'); + expect(item.find(UnstyledLink)).toContainReactText('baz'); }); it('delegates url to ', () => { @@ -225,7 +249,9 @@ describe('', () => { }, ); - expect(item.find(UnstyledLink).prop('url')).toBe('foo'); + expect(item).toContainReactComponent(UnstyledLink, { + url: 'foo', + }); }); it('delegates disabled to ', () => { @@ -236,7 +262,9 @@ describe('', () => { }, ); - expect(item.find(UnstyledLink).prop('aria-disabled')).toBe(true); + expect(item).toContainReactComponent(UnstyledLink, { + 'aria-disabled': true, + }); }); it('delegates accessibilityLabel to ', () => { @@ -252,9 +280,9 @@ describe('', () => { }, ); - expect(item.find(UnstyledLink).prop('aria-label')).toBe( - accessibilityLabel, - ); + expect(item).toContainReactComponent(UnstyledLink, { + 'aria-label': accessibilityLabel, + }); }); it('delegates onClick to ', () => { @@ -263,8 +291,10 @@ describe('', () => { , {location: 'bar'}, ); - - item.find(UnstyledLink).find('a').simulate('click'); + const link = item.find(UnstyledLink)!.find('a'); + link!.trigger('onClick', { + currentTarget: link!.domNode as HTMLDivElement, + }); expect(spy).toHaveBeenCalledTimes(1); }); @@ -279,7 +309,7 @@ describe('', () => { {location: 'bar'}, ); - expect(item.find('button').props()).toMatchObject({ + expect(item).toContainReactComponent('button', { 'aria-disabled': false, 'aria-label': 'some a11y label', }); @@ -295,7 +325,15 @@ describe('', () => { , {...context}, ); - item.find(UnstyledLink).find('a').simulate('click'); + item + .find(UnstyledLink)! + .find('a')! + .trigger('onClick', { + preventDefault: jest.fn(), + currentTarget: { + getAttribute: () => 'foo', + }, + }); expect(context.onNavigationDismiss).toHaveBeenCalledTimes(1); }); @@ -320,7 +358,15 @@ describe('', () => { />, {...context}, ); - item.find(UnstyledLink).last().find('a').simulate('click'); + item + .find(UnstyledLink)! + .find('a')! + .trigger('onClick', { + preventDefault: jest.fn(), + currentTarget: { + getAttribute: () => 'foo', + }, + }); expect(context.onNavigationDismiss).toHaveBeenCalledTimes(1); }); }); @@ -346,7 +392,7 @@ describe('', () => { }, ); - expect(item.find(Indicator).exists()).toBe(true); + expect(item).toContainReactComponent(Indicator); }); it('renders a new badge on sub navigation item if marked as new', () => { @@ -375,7 +421,7 @@ describe('', () => { }, ); - expect(item.find(Item).last().find(Badge).exists()).toBe(true); + expect(item).toContainReactComponent(Badge); }); describe('small screens', () => { @@ -406,7 +452,12 @@ describe('', () => { }, ); - item.find('button').simulate('click'); + item.find('button')!.trigger('onClick', { + preventDefault: jest.fn(), + currentTarget: { + getAttribute: () => 'baz', + }, + }); expect(spy).toHaveBeenCalledTimes(1); }); @@ -419,7 +470,12 @@ describe('', () => { }, ); - item.find('button').simulate('click'); + item.find('button')!.trigger('onClick', { + preventDefault: jest.fn(), + currentTarget: { + getAttribute: () => 'baz', + }, + }); expect(spy).toHaveBeenCalledTimes(1); }); }); @@ -431,11 +487,19 @@ describe('', () => { , ); - item.find('button').simulate('keyup', {keyCode: 9}); - expect(item.find('button').hasClass('keyFocused')).toBe(true); + const event: KeyboardEventInit & {keyCode: Key} = { + keyCode: Key.Tab, + }; + + item.find('button')!.trigger('onKeyUp', event); + expect(item).toContainReactComponent('button', { + className: 'Item keyFocused', + }); - item.find('button').simulate('blur'); - expect(item.find('button').hasClass('keyFocused')).toBe(false); + item.find('button')!.trigger('onBlur'); + expect(item).toContainReactComponent('button', { + className: 'Item', + }); }); it('adds and removes a class to a link when item was tabbed into focus and then blurred', () => { @@ -443,11 +507,19 @@ describe('', () => { , ); - item.find('a').simulate('keyup', {keyCode: 9}); - expect(item.find('a').hasClass('keyFocused')).toBe(true); + const event: KeyboardEventInit & {keyCode: Key} = { + keyCode: Key.Tab, + }; - item.find('a').simulate('blur'); - expect(item.find('a').hasClass('keyFocused')).toBe(false); + item.find('a')!.trigger('onKeyUp', event); + expect(item).toContainReactComponent('a', { + className: 'Item keyFocused', + }); + + item.find('a')!.trigger('onBlur'); + expect(item).toContainReactComponent('a', { + className: 'Item', + }); }); }); }); @@ -477,7 +549,7 @@ function mountWithNavigationProvider( node: React.ReactElement, context: React.ContextType = {location: ''}, ) { - return mountWithAppProvider( + return mountWithApp( {node} ,