[UnderlineNav] Accessibility Remediations

[broccolinisoup]

This PR addresses the accessibility feedback from @ericwbailey on UnderlineNav. It also includes re-styling the arrow buttons to give more real estate for items for be selected as well as styling the focus ring of these buttons. I thought it will be good to ship it with accessibility remediations as focus rings were not in a great state to be sent to sign off review. Please let me know if anyone has any concerns with it 🙌🏼

Storybook link: https://primer-9cc12887c1-13348165.drafts.github.io/storybook/?path=/story/components-underlinenav--internal-responsive-nav

Accessibility Eng Review ticket: #921

Remediations

• Add aria-label to the UnderlineNav(nav item) on the stories and docs
• Update the arrow buttons' accessible names by making the UnderlineNav's ariaLabel prop available to them
• Scroll focused item into the view
• Add aria-disabled attribute to the arrow buttons and conditionally set true/false when they become visible/hidden ( visibility depends on if there is any overflowing item on left/right)
• Trigger focus on the hidden arrow button after their aria-disabled attribute set to true. (Main motivation is to trigger a reread on the assistive technology with the aria-disabled=true to communicate the arrow button is no longer there)
• Update selectedLink style to ensure it is visible in Forced Colours mode
• Add focusable=true to icons that are aria-hidden=true

Screenshots and screen recordings

UnderlineNav-slideinandout.mov

Min touch target size

Focus rings (Feedback needed)

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: e70fe8e

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	Patch

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	77.59 KB (+0.49% 🔺)
dist/browser.umd.js	78.26 KB (+0.53% 🔺)

[ericwbailey]

I can't speak to the underlying logic, but the functionality works from an assistive technology perspective.

One small comment on preserving focus rings, otherwise I'd say this is good to go!

[broccolinisoup]

@danielguillan I have updated the arrow button styles and the focus ring! I would appreciate your review 🙏🏼 I might have to address your feedback if there is any on a different PR because I'm aiming to send the UnderlineNav for the sign off a11y review early next week 🥳 Let me know if you have any concerns with it 🙌🏼

[danielguillan]

@broccolinisoup! Thank you for the updates!

We'll need to ensure the arrow buttons provide a minimum touch target size of 44px. This is where the arrow slide-in/out effect becomes needed so we don't end up with large paddings at both ends of the component. We can do this in a new PR.

[broccolinisoup]

@broccolinisoup! Thank you for the updates!

We'll need to ensure the arrow buttons provide a minimum touch target size of 44px. This is where the arrow slide-in/out effect becomes needed so we don't end up with large paddings at both ends of the component. We can do this in a new PR.

Hi @danielguillan sorry that I ignored the minimal touch target! I have pushed another commit with the slide in/out. I would appreciate your review 🙏🏼 It is working fine on my side and I like it, definitely less paddings and more space.

Also, could I get a review on the focus rings as well? I gave enough paddingX to the button to make the minimal touch target but not sure about paddingY. It is rectangular now, so not sure if it is a desired state. Let me know what you think 😊

Note: I did the slide in/out here because there were still other concerns holding this PR - just to let you know 😊

[danielguillan]

@broccolinisoup Thanks for all the updates! 🙌

Focus ring + touch target

I don't think we need to override the paddings of the arrow buttons. We want a small IconButton with its default size and focus ring and a safe touch target area of '44 × 44 pixels`.

The current wrapper div is already the right size, so we might be able to leverage that one for the touch target.

Also, while playing with the scroll behavior on mobile I've seen the focus ring flashing on the arrow buttons when they slide in and out. But I don't seem to be able to reproduce it every time…

Component overflow hidden

We need to set the overflow of the root element to hidden so the arrow buttons aren't visible outside of it when sliding out.

Without overflow: hidden (current):

With overflow: hidden:

Fade effect

Sometimes, the fade effect gradient layer becomes a solid background when scrolling and using the arrow buttons.

[broccolinisoup]

@broccolinisoup Thanks for all the updates! 🙌

Focus ring + touch target

I don't think we need to override the paddings of the arrow buttons. We want a small IconButton with its default size and focus ring and a safe touch target area of '44 × 44 pixels`.

The current wrapper div is already the right size, so we might be able to leverage that one for the touch target.

Also, while playing with the scroll behavior on mobile I've seen the focus ring flashing on the arrow buttons when they slide in and out. But I don't seem to be able to reproduce it every time…

Component overflow hidden

We need to set the overflow of the root element to hidden so the arrow buttons aren't visible outside of it when sliding out.

Without overflow: hidden (current):

With overflow: hidden:

Fade effect

Sometimes, the fade effect gradient layer becomes a solid background when scrolling and using the arrow buttons.

Great feedback, thanks!! I'll address them in the next PR 🙌🏼

[ericwbailey]

🚀

[pksjce]

Lets go! 🚀