UnderlineNav2: Only allow Enter and Space to select item

[broccolinisoup]

Addressing an accessibility sign-off review issue that described below.

For some reason, focusing any of the nav links and typing any printable character on the keyboard (including numbers) activates the link and causes it to become the current page. This should be removed.

Merge checklist

• Added/updated tests - N/A
• Added/updated documentation - N/A
• 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: ce590a3

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	78.89 KB (0%)
dist/browser.umd.js	79.54 KB (0%)

[joshblack]

Just wanted to leave a potential refactor idea, shouldn't change any functionality. Feel free to dismiss if you think they aren't really helping anything 👀

• Since there are only two members in [' ', 'Enter'] it might make sense to just explicitly check for both of them. If there are more expected members, though, I totally would get having a collection
• Instead of doing the explicit guard for each callback, this might be able to be simplified into a single if branch and use the optional chaining syntax name?.()

Suggested change

	if ([' ', 'Enter'].includes(event.key)) {
	if (!event.defaultPrevented && typeof onSelect === 'function') onSelect(event)
	if (!event.defaultPrevented && typeof afterSelect === 'function') afterSelect(event)
	setSelectedLink(ref as RefObject<HTMLElement>)
	}
	if (event.key === ' ' || event.key === 'Enter') {
	if (!event.defaultPrevented) {
	onSelect?.(event)
	afterSelect?.(event);
	}
	setSelectedLink(ref as RefObject<HTMLElement>)
	}

Just a couple of thoughts, feel free to disregard if they're not helpful!

[broccolinisoup]

I like it ✨ They are helpful!!

onSelect?.(event) makes sense. How do I make sure that it is a function type as well? Or checking its type isn't necessary here at all? 🤔

[joshblack]

@broccolinisoup for the type of onSelect, do you know if it's either a function or undefined/null or could it be types other than those two?

I think if it's the former (can be a function or undefined), then it should be good. If it's the latter, then I think you're totally right in pointing out that you would need a type guard for it and what you already have is exactly what would be needed 👍

[broccolinisoup]

Because onSelect is an exposed event to the API, I feel like it might worth to check the type. It is unlikely that will error out when there is type checking and proper linting rules in place on consumer side but still just to be double cautious to prevent them passing things on to onSelect other than functions. I'd like to keep the function type checking if that is okay with you and see how it goes 👀

I'll apply your other suggestions on a commit from my local ✨