[UnderlineNav2]: Introduce disclosure widget pattern #2466

broccolinisoup · 2022-10-21T07:34:09Z

Following up the accessibility sign-off review feedback (ref: comment), this PR discarding the ActionMenu and introduces a disclosure widget.

Storybook test link : https://primer-0a924a1192-13348165.drafts.github.io/storybook/iframe.html?args=&id=components-underlinenav--internal-responsive-nav&viewMode=story

The reasons for discarding the ActionMenu are

The ActionMenu is a portal which means it doesn't follow the button element (that toggles the menu) in the DOM hierarchy. Considering aria-controls attribute's poor support, we cannot rely on it therefore, the menu has to follow the button in the DOM order.
Keyboard accessibility on the ActionMenu is not aligning with the disclosure pattern one. I.e focus trap or only arrow key navigation.

What did we introduce? (Disclosure widget pattern)

We have a button that toggles a menu.
Menu immediately follows the button element in the DOM hierarchy.
Button has aria-controls and its value is the id of the menu.
Button has aria-expanded attribute and conditionally have true / false value.
Navigation items (both list and menu items) are navigable with arrow keys (both horizontal and vertical) and the tab key.
Menu doesn't trap the focus.
Menu can be closed with clicking outside and ESC.

Screenshots

There shouldn't be any visual differences between the ActionMenu version and the disclosure widget version.

Merge checklist

Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

changeset-bot · 2022-10-21T07:34:11Z

🦋 Changeset detected

Latest commit: f1e411e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

broccolinisoup · 2022-10-21T20:54:08Z

@ericwbailey I apologise again for pointing out the wrong demo. Here is the correct one. I updated the description with items that we introduced with the disclosure pattern. Looking forward to hearing your feedback 🙌🏼

Thank you so much again 🙏🏼

github-actions · 2022-10-23T22:28:08Z

size-limit report 📦

Path	Size
dist/browser.esm.js	78.14 KB (0%)
dist/browser.umd.js	78.79 KB (0%)

siddharthkp · 2022-10-24T11:24:25Z

src/UnderlineNav2/styles.ts

+  right: '0',
+  boxShadow: '0 1px 3px rgba(0, 0, 0, 0.12), 0 1px 2px rgba(0, 0, 0, 0.24)',
+  borderRadius: '12px',
+  backgroundColor: `${theme?.colors.canvas.overlay}`,


Do we need to access this from theme instead of the token name?

Suggested change

backgroundColor: `${theme?.colors.canvas.overlay}`,

backgroundColor: 'canvas.overlay'

Oh I didn't know I can just pass the token name! Thank you 🙌🏼 Is it only for colours as we import only colour primitives currently maybe? I see mix practises around, where can I learn more about this? 👀 or should I just dig in? 😅

I'll do a clean up to remove the other unnecessary theme args in the next PR!

src/UnderlineNav2/UnderlineNav.tsx

ericwbailey

I left a comment about utilizing a UUID for the id, but other than that this sounds great to me in AT. Loving the roving tabindex, too!

src/UnderlineNav2/UnderlineNav.tsx

joshblack

Looks awesome! Just left a minor comment and wanted to +1 the id suggestion from @ericwbailey on the <ul> for the disclosure.

Awesome work ✨

src/UnderlineNav2/UnderlineNav.tsx

broccolinisoup · 2022-10-25T08:49:39Z

@colebemis following up our conversation around using ActionList.LinkItem instead of ActionList.Item, I have discovered some interesting things today. I currently have 3 issues with the LinkItem

Sometimes, super randomly, the menu items don't get the focus. Please see the screen recording below. I cannot get into the menu with keyboard until 0:29 second! I don't know why they start getting the focus after.
Focus gets trap in the menu. The last 5 seconds of the recording is showing this issue.
Focus ring colour is different on menu items only on Firefox. (We already chatted about this)

Also please notice my key strokes on the recording 😄 Shoutout to @mperrotti for sharing this app on his DOL talk ✨

ActionList.LinkItem.mp4

When ActionList.Item is given as prop, it becomes semantically the same element as ActionList.LinkItem on the DOM, so I am sticking to the ActionList.Item here for now if that is okay. That said, I am curious to understand why this happens, but also think this might more to do with the ActionList.LinkItem than UnderlineNav. So how would you feel if I investigate this separately?

There is one more commit left to review in this PR. Previous commits and implementation are reviewed and approved by Eric and Josh. Thanks to you all again for reviewing 💖

colebemis

Latest commit looks good 👍 Ship it 🚢

Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com>

…11y remediations) (#2471) * Display at least two items in the menu * add changeset

…esent (#2470) * Add visually hidden heading * add changeset and a test * append 'navigation' to the aria-label string

broccolinisoup force-pushed the underlineNav-sign-off-remediations branch from 74387fa to 4247884 Compare October 21, 2022 07:38

broccolinisoup temporarily deployed to github-pages October 21, 2022 07:40 Inactive

broccolinisoup mentioned this pull request Oct 21, 2022

[UnderlineNav2]: Deprecate coarse input detection and its scroll behaviour (A11y sign-off remediations) #2447

Merged

5 tasks

This comment was marked as outdated.

Sign in to view

broccolinisoup force-pushed the introduce-disclosure-pattern branch 2 times, most recently from 00b7607 to 177ab6b Compare October 21, 2022 20:36

broccolinisoup temporarily deployed to github-pages October 21, 2022 20:43 Inactive

broccolinisoup temporarily deployed to github-pages October 23, 2022 22:31 Inactive

broccolinisoup force-pushed the underlineNav-sign-off-remediations branch 2 times, most recently from 611c1a5 to 88e4586 Compare October 23, 2022 22:53

broccolinisoup force-pushed the introduce-disclosure-pattern branch from b115613 to 8427af9 Compare October 23, 2022 22:58

broccolinisoup temporarily deployed to github-pages October 23, 2022 23:04 Inactive

broccolinisoup force-pushed the introduce-disclosure-pattern branch from 8427af9 to ef2cb54 Compare October 24, 2022 01:50

broccolinisoup added the skip changeset This change does not need a changelog label Oct 24, 2022

broccolinisoup temporarily deployed to github-pages October 24, 2022 01:57 Inactive

broccolinisoup changed the title ~~[UnderlineNav2]: Introduce disclosure pattern~~ [UnderlineNav2]: Introduce disclosure widget pattern Oct 24, 2022

broccolinisoup marked this pull request as ready for review October 24, 2022 04:42

broccolinisoup requested review from a team, rezrah, ericwbailey and joshblack October 24, 2022 04:42

siddharthkp reviewed Oct 24, 2022

View reviewed changes

src/UnderlineNav2/UnderlineNav.tsx Outdated Show resolved Hide resolved

ericwbailey approved these changes Oct 24, 2022

View reviewed changes

src/UnderlineNav2/UnderlineNav.tsx Outdated Show resolved Hide resolved

joshblack approved these changes Oct 24, 2022

View reviewed changes

src/UnderlineNav2/UnderlineNav.tsx Outdated Show resolved Hide resolved

broccolinisoup force-pushed the underlineNav-sign-off-remediations branch 2 times, most recently from 7ac221a to 6c076f1 Compare October 24, 2022 23:59

Base automatically changed from underlineNav-sign-off-remediations to main October 25, 2022 00:11

broccolinisoup force-pushed the introduce-disclosure-pattern branch from ef2cb54 to 2b1a8d2 Compare October 25, 2022 00:15

broccolinisoup temporarily deployed to github-pages October 25, 2022 00:22 Inactive

broccolinisoup temporarily deployed to github-pages October 25, 2022 01:46 Inactive

broccolinisoup temporarily deployed to github-pages October 25, 2022 02:49 Inactive

broccolinisoup temporarily deployed to github-pages October 25, 2022 08:24 Inactive

broccolinisoup temporarily deployed to github-pages October 25, 2022 09:04 Inactive

broccolinisoup mentioned this pull request Oct 25, 2022

[UnderlineNav2]: Always show at least 2 items in the overflow menu (A11y remediations) #2471

Merged

4 tasks

broccolinisoup requested a review from colebemis October 25, 2022 19:42

colebemis approved these changes Oct 25, 2022

View reviewed changes

broccolinisoup and others added 7 commits October 26, 2022 10:03

Disclosure pattern implementation

a8e143a

use token name for colours instead of accessing them from theme

9f99f33

Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com>

code review feedback <3

cbee596

[UnderlineNav2]: Always show at least 2 items in the overflow menu (A…

53edba8

…11y remediations) (#2471) * Display at least two items in the menu * add changeset

[UnderlineNav2]: Add visually hidden heading where aria-label is pr…

858b449

…esent (#2470) * Add visually hidden heading * add changeset and a test * append 'navigation' to the aria-label string

use prop for 'as'

5c907fd

add changeset

f1e411e

broccolinisoup removed the skip changeset This change does not need a changelog label Oct 26, 2022

broccolinisoup force-pushed the introduce-disclosure-pattern branch from 5c48cdc to f1e411e Compare October 26, 2022 00:04

broccolinisoup temporarily deployed to github-pages October 26, 2022 00:10 Inactive

broccolinisoup merged commit 9ff856d into main Oct 26, 2022

broccolinisoup deleted the introduce-disclosure-pattern branch October 26, 2022 00:18

primer-css mentioned this pull request Oct 26, 2022

Release tracking #2482

Merged

broccolinisoup mentioned this pull request Nov 28, 2022

ActionMenu+ActionList Accessibility issues #2293

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UnderlineNav2]: Introduce disclosure widget pattern #2466

[UnderlineNav2]: Introduce disclosure widget pattern #2466

broccolinisoup commented Oct 21, 2022 •

edited

Loading

changeset-bot bot commented Oct 21, 2022 •

edited

Loading

This comment was marked as outdated.

broccolinisoup commented Oct 21, 2022 •

edited

Loading

github-actions bot commented Oct 23, 2022 •

edited

Loading

siddharthkp Oct 24, 2022 •

edited

Loading

broccolinisoup Oct 25, 2022

broccolinisoup Oct 25, 2022

ericwbailey left a comment

joshblack left a comment

broccolinisoup commented Oct 25, 2022 •

edited

Loading

colebemis left a comment

	backgroundColor: `${theme?.colors.canvas.overlay}`,
	backgroundColor: 'canvas.overlay'

[UnderlineNav2]: Introduce disclosure widget pattern #2466

[UnderlineNav2]: Introduce disclosure widget pattern #2466

Conversation

broccolinisoup commented Oct 21, 2022 • edited Loading

Screenshots

Merge checklist

changeset-bot bot commented Oct 21, 2022 • edited Loading

🦋 Changeset detected

This comment was marked as outdated.

broccolinisoup commented Oct 21, 2022 • edited Loading

github-actions bot commented Oct 23, 2022 • edited Loading

size-limit report 📦

siddharthkp Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

broccolinisoup Oct 25, 2022

Choose a reason for hiding this comment

broccolinisoup Oct 25, 2022

Choose a reason for hiding this comment

ericwbailey left a comment

Choose a reason for hiding this comment

joshblack left a comment

Choose a reason for hiding this comment

broccolinisoup commented Oct 25, 2022 • edited Loading

colebemis left a comment

Choose a reason for hiding this comment

broccolinisoup commented Oct 21, 2022 •

edited

Loading

changeset-bot bot commented Oct 21, 2022 •

edited

Loading

broccolinisoup commented Oct 21, 2022 •

edited

Loading

github-actions bot commented Oct 23, 2022 •

edited

Loading

siddharthkp Oct 24, 2022 •

edited

Loading

broccolinisoup commented Oct 25, 2022 •

edited

Loading