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

[broccolinisoup]

This PR removes detecting the pointer type and coarse pointer's scroll behaviour according to @ericwbailey 's https://github.com/github/primer/issues/1112#issuecomment-1281157150 on the sign-off review.

Storybook link: https://primer-bebeaa10ea-13348165.drafts.github.io/storybook/?path=/story/components-underlinenav--default-nav
Docs link: https://primer-bebeaa10ea-13348165.drafts.github.io/drafts/UnderlineNav2

What should code reviewers look at?

This PR mainly addresses removing the scroll overflow behaviour. As long as the main overflow behaviour (Pulling items into the overflow More menu when they don't fit into the screen) works regardless of the pointer type, this PR's main functionality should be good ✨

There are 2 merged PRs' commits in the history

• [UnderlineNav2]: React router implementation fixes & docs improvement is reviewed by @joshblack ✨
• [UnderlineNav2]: Add string type to the counter prop and display loading counter for all The storybook implementation is reviewed by @danielguillan ✨ It would be nice to get 👀 on the react code.

Merge checklist

• Added/updated documentation
• 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]

🦋 Changeset detected

Latest commit: 6c076f1

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

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	78.12 KB (0%)
dist/browser.umd.js	78.77 KB (0%)

[broccolinisoup]

@ericwbailey @danielguillan
This PR removes detecting the pointer type and coarse pointer's scroll behaviour according to @ericwbailey 's comment on the sign-off review.

Storybook link: https://primer-bebeaa10ea-13348165.drafts.github.io/storybook/?path=/story/components-underlinenav--default-nav

I didn't add anything extra yet (except some UI design adjustments which shouldn't affect the behaviour) because I have a few questions around the items on the comment. Mostly accessibility.

Utilize a disclosure widget to contain links that horizontally overflow.

Currently the More button has aria-expanded attribute and conditionally render its value when the menu is open/closed. I guess this considered a disclosure widget? When I check the codepen example on WAI, the button also has aria-controls and the value of it is the id of the disclosure widget. Would we need this? Because currently ActionMenu doesn't have such pattern.

• Utilize a ul with a elements for each link item inside the disclosure widget.

We have it already from the ActionList component ✅

• Conditionally add the last two links to the disclosure widget if only one link is visually truncated, so the list declaration makes sense.

Our current overflow behaviour does this. I would appreciate a confirmation though to make sure if we are are talking about the same thing

• Conditionally add aria-current="true" to the disclosure widget if it contains the active nav link.

Currently we take the item out of the menu when it is selected and pop it into the list as the last item. So disclosure widget would actually never get to have an active nav link with the current behaviour. If this behaviour is not desirable , I might want to chat it more in dept. If the behaviour is okay, we might not need to do anything else for this matter because the selected nav item itself has aria-current=true

Add a heading whose name matches the landmark's aria-label.

I am not sure if I understand this correctly. When the nav item's name matches the landmark's (nav) aria-label, we will need to wrap the a link with a heading element? Or did I completely misunderstand this? 🙈

Re the keyboard navigation, current implementation only work with arrow keys in the menu (disclosure widget). Is that what we need or do we need to bind tab key into the menu items as well?

Thank you so much in advance 🙏🏼

[ericwbailey]

When I check the codepen example on WAI, the button also has aria-controls and the value of it is the id of the disclosure widget. Would we need this? Because currently ActionMenu doesn't have such pattern.

aria-controls hypothetically creates a programatic association between the disclosure's toggle button and its list content, but support for it is very poor.

So, it's good that it is present in the APG example, in that things that support it will utilize it. However, not including it isn't detrimental, provided the disclosure widget's content is placed immediately after the disclosure widget's button in the DOM.

I am not sure if I understand this correctly. When the nav item's name matches the landmark's (nav) aria-label, we will need to wrap the a link with a heading element? Or did I completely misunderstand this? 🙈

I think I could have been more clear about this, apologies.

I believe the feedback was to provide a visually-hidden heading element before the nav element, provided an aria-label was present on the nav. The heading's content would use the aria-label's provided string. So, something like:

<div class="underline-nav-wrapper">
  <h2 class="sr-only">
    Code navigation
  </h2>
  <nav aria-label="Code">
    <!-- UnderlineNav internals -->
  </nav>
</div>

The reason for this is headings are used far more frequently than landmarks as a navigation/discoverability aide, but this approach allows us to utilize both.

If this behaviour is not desirable , I might want to chat it more in dept.

The thing we are trying to avoid is having only one list item in the "More" nav. If a user cannot see the screen, they may be confused as to why a single list item is present, and mistakenly go looking for the other "missing" list content.

Re the keyboard navigation, current implementation only work with arrow keys in the menu (disclosure widget). Is that what we need or do we need to bind tab key into the menu items as well?

Yep!

I am now wondering if, with this friction, should we have a discrete disclosure widget pattern added to Primer?

[broccolinisoup]

So, it's good that it is present in the APG example, in that things that support it will utilize it. However, not including it isn't detrimental, provided the disclosure widget's content is placed immediately after the disclosure widget's button in the DOM.

@ericwbailey Thanks for clarifying that! The issue here for us is that the Action Menu is a portal and portals hierarchically exist outside of the parent component. So I guess we don't have any cue that the More button controls the disclosure widget here. Considering that we will need to bind the tab key along with the existing arrow button navigation, I agree that we might need a discrete disclosure widget pattern in Primer. cc @danielguillan here to deepen the convo.

Re the sr-only heading, thanks for the clarification, it makes sense! I'll implement that.

Avoiding having only one list item in the more menu, I see what you mean and thanks for the explanation! @danielguillan how should we proceed with this? Thinking of a little bit edge case where only 2 items but long items and one is selected; how should it behave in this case?

[danielguillan]

I am now wondering if, with this friction, should we have a discrete disclosure widget pattern added to Primer?

@ericwbailey can you provide more details on how this pattern would work?

The thing we are trying to avoid is having only one list item in the "More" nav.

This makes a lot of sense. I think we have two options here:

1. @ericwbailey's suggestion of adding a second item to the "More" menu.
2. truncate the last item with an ellipsis to avoid the more menu altogether.

@broccolinisoup I think we could test Eric's idea first and see how it works.

[ericwbailey]

@ericwbailey can you provide more details on how this pattern would work?

I think it would probably be more on the low-level/primitive end of things. Essentially we'd want a way to have a button toggle that reveals associated child navigation content.

This pattern is a little different than existing components, as I understand them:

• The Details component is similar, but is used to reveal non-navigation content.
• The Dialog component traps focus, which a disclosure widget should not do.
• The ActionMenu/ActionList component may be similar, but is more intended for application-style menu options, and not navigation.

I'm also unsure if ActionMenu/ActionList's props allow us to configure it into a disclosure navigation widget, especially with @broccolinisoup's comment about it being a portal component. Disclosure widgets are typically highly contextual, so it feels like we'd be working against that component pairing's grain.

[danielguillan]

@ericwbailey thank you for clarifying!

[broccolinisoup]

@ericwbailey Thanks for clarifying! I feel like we need a combination of all of those components you mentioned above 🙂

I tried to whip something up quickly today, mainly to get something more tangible for us to discuss from. You can see the half baked prototype here . Please note that It is a very raw example, so most of the keyboard navigation doesn't work and styles are just there to demonstrate the main functionality.

I discarded the ActionMenu due to being portal and ActionList.Item due to not using the tab key for navigation. Alternative was to use ActionList.LinkItem but this one doesn't accept arrow keys to navigate. So I simply removed them all and wrote something from scratch.

Essentially we'd want a way to have a button toggle that reveals associated child navigation content.

• 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.
• Menu items are navigable with arrow up and arrow down as well as the tab key. (Arrow keys are not implemented in the demo)
• Menu doesn't trap the focus.
• Menu can be closed with clicking outside and esc. (Not implemented in the demo)

These are the behaviours I think we will need for the disclosure widget pattern as far as I understand. Please add if I miss anything. I am looking forward to hearing what you think and if this is something we can build from. @danielguillan

Appreciate you both 🙇🏼‍♀️

[broccolinisoup]

@ericwbailey @danielguillan I created another PR for the disclosure pattern : #2466 Could we continue the convo there please?

[broccolinisoup]

Context: the type of the counter is string | number and not every item has to have a counter.

[broccolinisoup]

@colebemis Could I please get a review from you on this one? ☺️

[colebemis]

💖