fix: Fix NVDA menu item counts that are broken by <form>

[smockle]

What are you trying to accomplish?

Fixes https://github.com/github/accessibility-audits/issues/7582 (Hubber login required)

When a role="menu" element contains a <form>, NVDA skips over that <form>’s role="menuitem" children.

NVDA generally announces “menu item i of t” for every menu item. However, NVDA neither announces an “i” (item index), nor sums in “t” (total item count), any role="menuitem" element inside a <form>.

Here are two examples:

• NVDA announces that this menu has 3 items, but it has 4. If, using browser devtools, you add role="none" to the <form>, then NVDA correctly detects the 4 menu items.

• This occurs on https://github.com/orgs/github/projects (Hubber login required). Under “Recently viewed” projects, in the three-dot “More project options” menu (to the right of each project), NVDA doesn’t count “Close project” and “Remove from recently viewed” as menu items. Both are forms-inside-menu.

List the issues that this change affects.

Closes https://github.com/github/accessibility-audits/issues/7582 (Hubber login required)

Risk Assessment

• Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.

I’m rating this as medium risk, because I don’t know how likely we are to detect unintended side-effects of this change. I don’t know the relevant components’ test coverage.

What approach did you choose and why?

I chose to suppress the <form> element’s implicit role="form" using role="none". This makes its child role="menuitem" element become a direct child of the role="menu" element in the accessibility tree. This makes NVDA announce the child menu item properly.

I asked if this would break anything, in #accessibility (Hubber login required). @zersiax said: “…nulling the form semantics is what people generally do in this case…carry on.”

Notably, as of 418b6cc, role="none" is only applied for <form>s inside ActionMenus, not <form>s inside ActionLists generally.

Anything you want to highlight for special attention from reviewers?

• Do we have adequate test coverage to feel safe from unintended side-effects?

• Do I need to update any docs?

Accessibility

• No new axe scan violation - This change does not introduce any new axe scan violations.

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Lookbook)
• 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: 418b6cc

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

This PR includes changesets to release 1 package

Name	Type
@primer/view-components	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

[joshblack]

LGTM! Including @jonrohan too for another set of 👀

[owenniblock]

This looks good to me (after reading through the Slack thread) although it does feel weird. Seems like the best solution we can implement right now that will fix this accessibility violation 🤷

[jonrohan]

Looks good 👍🏻 I'll leave this to @camertron to merge into the current release, or wait until next one. We're close to shipping the current release and don't want to add more on the plate at the moment.

[camertron]

The ActionMenu component is quite well-tested, so I'm ok with this 👍

[smockle]

Related PR: #2828