[Navigation] Add aria attributes for sub nav items #3661

kyledurand · 2020-11-27T19:37:00Z

WHY are these changes introduced?

Fixes https://github.com/Shopify/polaris-ux/issues/448

WHAT is this pull request doing?

Always render sub nav items so that aria-controls has an id to reference
Adds aria-expanded true/false when nav items have sub items
Add area-controls attribute with ID and corresponding id to Collapsible

Converted Navigation Item tests to use mountWithApp and added aria tests
Added missing tests for Secondary

How to 🎩

Check out the details page or any navigation examples and make sure that only nav items with sub navs have the two attributes mentioned above. Make sure that aria-expanded is true when the sub nav menu is expanded, and false when it isn't

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

github-actions · 2020-11-27T19:39:15Z

🟢 This pull request modifies 7 files and might impact 3 other files.

Details:

All files potentially affected (total: 3)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 playground/DetailsPage.tsx (total: 0)

Files potentially affected (total: 0)

🎨 src/components/Navigation/Navigation.scss (total: 1)

Files potentially affected (total: 1)

/src/components/Navigation/Navigation.tsx

🧩 src/components/Navigation/components/Item/Item.tsx (total: 2)

Files potentially affected (total: 2)

🧩 src/components/Navigation/components/Item/components/Secondary/Secondary.tsx (total: 3)

Files potentially affected (total: 3)

🧩 src/components/Navigation/components/Item/components/Secondary/tests/Secondary.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Navigation/components/Item/tests/Item.test.tsx (total: 0)

Files potentially affected (total: 0)

kyledurand · 2020-11-27T19:55:14Z

src/components/Navigation/components/Item/Item.tsx


  let secondaryNavigationMarkup: ReactNode = null;

-  if (subNavigationItems.length > 0 && showExpanded) {


This is the scariest thing for me in this PR. This makes sub menus and their items render in the DOM, though hidden. I did this so that the aria-controls id will have a valid reference ID but not sure that's super necessary to see until the sub menu has actually rendered.

This broke the Nav in web the last time I tried this, for adding animations but Alex's fixes to Collapsible has fixed that.

Anyone see any potential issues in doing this?

This makes sub menus and their items render in the DOM, though hidden. I did this so that the aria-controls id will have a valid reference ID but not sure that's super necessary to see until the sub menu has actually rendered.

Hmm, interesting! I'm not sure about aria-control, but certain screen readers require elements to always be in the DOM for certain attributes (e.g aria-live)

Anyone sees any potential issues in doing this?

As long as the Collapsible changes roll out ok and collapsible hides the children from keyboard access I don't foresee any isssues.

AndrewMusgrave

🎩 LTGM ✅

AndrewMusgrave · 2020-12-07T17:58:50Z

src/components/Navigation/components/Item/components/Secondary/Secondary.tsx

+    <Collapsible
+      id={id || uid}
+      open={expanded}
+      transition={{duration: '0ms', timingFunction: 'linear'}}


TIL we use collapsible in navigation - why do we need these props now? I don't recall there being an animation before 🤔

Collapsible defaults to an animation so I'm cancelling that here with 0ms.

AndrewMusgrave · 2020-12-07T18:00:31Z

src/components/Navigation/Navigation.scss

  overflow-x: var(--p-override-visible, hidden);

+  &.isExpanded {
+    margin-bottom: spacing(tight);


AndrewMusgrave · 2020-12-07T18:17:05Z

src/components/Navigation/components/Item/Item.tsx


  let secondaryNavigationMarkup: ReactNode = null;

-  if (subNavigationItems.length > 0 && showExpanded) {


This makes sub menus and their items render in the DOM, though hidden. I did this so that the aria-controls id will have a valid reference ID but not sure that's super necessary to see until the sub menu has actually rendered.

Hmm, interesting! I'm not sure about aria-control, but certain screen readers require elements to always be in the DOM for certain attributes (e.g aria-live)

Anyone sees any potential issues in doing this?

As long as the Collapsible changes roll out ok and collapsible hides the children from keyboard access I don't foresee any isssues.

BPScott

I was a little scared by the size of this PR but then I looked and found out most of it was migrating tests to use @shopify/react-testing!

I've not got much context on this but it looks like it does what you say it should

* [Navigation] Render sub-sections with aria attributes * Switch tests to use mountWithApp * Add test for Secondary

st-baonguyen · 2022-09-30T02:55:07Z

@kyledurand @AndrewMusgrave
Excuse me! I have a case about expanded navigation.
I have 2 item in subNavigation, and i want to expanded subNavigation when I click main menu and subNavigation, I tried by use expanded and onToggleExpandedState property but it is not expanded, When I click subNavigation, subMenu is unexpanded and it just expanded when selected of main menu = true. But I don't want use selected property. Pls help me. Thank you so much

kyledurand · 2022-09-30T13:09:21Z

@st-baonguyen can you create a playground or file an issue with more context?

st-baonguyen · 2022-10-05T04:27:17Z

@kyledurand Sorry about this delay.
I cann't share my source but I had create a playground on Codesanbox. Pls check it. Thank you
Could you check at line 40.
Expended = true, subNavigation doesn't expended, but with selected = true, subNavigation expended.
And I don't know what is Expended used for?.
https://codesandbox.io/s/navigation-sandbox-forked-svqgdn?file=/Nav.js

kyledurand · 2022-10-05T12:37:34Z

I see what you mean @st-baonguyen, thanks for the playground.

The Nav was ported over from the original rails implementation in ~2017, and we needed to copy the functionality from there.

We might have changed the way the nav works along the way and expanded is no longer needed. We'd have to check every implementation and prop combination used internally to be sure and we don't have the bandwidth for that right now.

Another way to achieve the expanded state is to set matches: true, or matchPaths: ['<path1>', <path2>].

This will also give you the selected style, however, which is what we want for our use case.

kyledurand force-pushed the navigation_render-sub-sections-with-aria-attributes branch from 7501eb7 to 049a50c Compare November 27, 2020 19:37

kyledurand requested review from AndrewMusgrave, BPScott and dfmcphee November 27, 2020 19:42

kyledurand commented Nov 27, 2020

View reviewed changes

AndrewMusgrave reviewed Dec 7, 2020

View reviewed changes

AndrewMusgrave approved these changes Dec 7, 2020

View reviewed changes

BPScott approved these changes Dec 15, 2020

View reviewed changes

alex-page force-pushed the master branch from ee1f920 to 405548f Compare December 21, 2020 18:28

kyledurand added 2 commits January 11, 2021 15:28

[Navigation] Render sub-sections with aria attributes

c7bd7d9

Switch tests to use mountWithApp

3577b38

kyledurand force-pushed the navigation_render-sub-sections-with-aria-attributes branch from 049a50c to 2d3d2da Compare January 11, 2021 20:29

Add test for Secondary

13ea6d8

kyledurand force-pushed the navigation_render-sub-sections-with-aria-attributes branch from 2d3d2da to 13ea6d8 Compare January 11, 2021 20:29

kyledurand merged commit 196e116 into master Jan 11, 2021

kyledurand deleted the navigation_render-sub-sections-with-aria-attributes branch January 11, 2021 20:49

sylvhama pushed a commit that referenced this pull request Mar 26, 2021

[Navigation] Add aria attributes for sub nav items (#3661)

b328084

* [Navigation] Render sub-sections with aria attributes * Switch tests to use mountWithApp * Add test for Secondary


		let secondaryNavigationMarkup: ReactNode = null;

		if (subNavigationItems.length > 0 && showExpanded) {

[Navigation] Add aria attributes for sub nav items #3661

[Navigation] Add aria attributes for sub nav items #3661

Uh oh!

Conversation

kyledurand commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

Uh oh!

github-actions bot commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 1)

Files potentially affected (total: 2)

Files potentially affected (total: 3)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Uh oh!

kyledurand Nov 27, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

kyledurand Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

BPScott left a comment

Choose a reason for hiding this comment

Uh oh!

st-baonguyen commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyledurand commented Sep 30, 2022

Uh oh!

st-baonguyen commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyledurand commented Oct 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kyledurand commented Nov 27, 2020 •

edited

Loading

github-actions bot commented Nov 27, 2020 •

edited

Loading

kyledurand Jan 11, 2021 •

edited

Loading

st-baonguyen commented Sep 30, 2022 •

edited

Loading

st-baonguyen commented Oct 5, 2022 •

edited

Loading