`TabNav` should exclude buttons within tabs from the focus zone

[iansan5653]

TabNav creates an automatic focus zone where focusing from tab to tab is performed via the arrow keys. This makes sense and matches the behavior recommended in the corresponding ARIA authoring guide.

However, there do exist tabs that have buttons inside them. For example, a tab might have a "close" button or a "menu" button. An example of this use-case is Projects, where we have a menu button on the active tab:

The anchor button for this menu is getting picked up by TabNav's focus zone, so it is only focusable via arrow keys. Instead, it should be the next focusable element via Tab. (See https://github.com/github/memex/issues/11966)

Confirmation that that is the correct approach was provided by the accessibility team in this Slack message:

Buttons are not valid inside a tab list structure, so must be placed outside of it. A pattern I've recommended in the past is to manage focus order so that when the user tabs from the tab list, they land on the button related to the tab they just came from, rather than there being a bunch of buttons in the tab order that don't map to anything in the user's mental mapping of the page.

Because TabNav does not allow configuring the focus zone, the only way to implement this behavior right now is to move the button outside of the TabNav altogether and then absolutely position it back into the tab. This is a really cumbersome and hacky approach, and it would be much easier to just fix this in the source component.

I think the simplest approach here would be to add a focusableElementFilter to the focus zone that checks if the element has role="tab". This wouldn't filter out tabs within tabs but that seems like a terrible idea anyway.

https://github.com/primer/react/blob/1030509285941da9b82680ff07c56a787178bcb5/src/TabNav.tsx#L41-L46

[iansan5653]

If this approach makes sense, I can go ahead and open a PR for it.

[lesliecdubs]

I'd like to nominate @hectahertz to take a look at this proposal and let us know what he thinks! Thanks @iansan5653, we'll get back to you.

[iansan5653]

Hi @lesliecdubs & @hectahertz - just following up on this issue. Any updates here? I'd really like to get this resolved because it's a significant accessibility concern in Projects.

[lesliecdubs]

Hi @iansan5653, thanks for pinging again and really sorry about the delay - Héctor is out sick.

I see you've gotten accessibility team guidance on this approach and it doesn't sound like it will cause a breaking change. @primer/react-reviewers should feel free to check me on this, but my assumption is that we would welcome a PR for this.

[iansan5653]

Great, I'll work on it!

[owenniblock]

I suspect that the solution provided will cause problems for some screen readers. Looking at the ARIA guidance for tabs I can see that browsers automatically apply a role of presentation to any children of a tab element - we'd need to test against a range of screen readers to see how this behaves but I'd still strongly recommend removing the button from the tab since it is not semantically valid.

cc @smockle as this week's FR in case there's any follow-up questions / discussion.

[iansan5653]

I suspect that the solution provided will cause problems for some screen readers. Looking at the ARIA guidance for tabs I can see that browsers automatically apply a role of presentation to any children of a tab element - we'd need to test against a range of screen readers to see how this behaves but I'd still strongly recommend removing the button from the tab since it is not semantically valid.

Thanks, I didn't realize this. I tested this with VoiceOver on Chrome (and with the Chrome accessibility tree inspector) and it works, but I guess there's no guarantee of it continuing to work if that's incorrect behavior according to the spec.

It looks like we'll need further work to make this compliant (moving the button to become the first element of the active tab's contents, then using CSS to put it back in the right place). I wonder if TabNav should natively support an accessible activeTabButton to make this easier for consumers.

[owenniblock]

I wonder if TabNav should natively support an accessible activeTabButton to make this easier for consumers.

Thanks a great idea @iansan5653 ! It totally makes sense to solve this at the component level in my opinion. cc @primer/react-reviewers - what's the best way to record this?

[lesliecdubs]

Thanks for the follow up and clarification on best practice @owenniblock! Apologies for the slow response here.

@iansan5653 would one of you be willing to write up a fresh issue in this repo outlining the activeTabButton proposal assuming this is still relevant, and the PRC crew will review at our next weekly sync?