patternfly / patternfly-elements

PatternFly Elements. A set of community-created web components based on PatternFly design.
https://patternflyelements.org/
MIT License
377 stars 92 forks source link

`<pf-tabs>`: roles not being read #2692

Closed Arathy-s closed 6 months ago

Arathy-s commented 7 months ago

Description of the issue

Elements with an ARIA [role] that require children to contain a specific [role] are missing some or all of those required children.

Impacted component(s)

Screenshots

Screenshot 2024-02-08 at 3 23 25 PM

bennypowers commented 7 months ago

@Arathy-s can you paste in the HTML here?

Arathy-s commented 7 months ago

Sure @bennypowers

<div class="overflow-tab-wrapper">
    <pf-tabs id="pf-tabs-p32svoeid" active-index="-1">
      <pf-tab id="users" slot="tab" aria-controls="pf-tab-panel-g6mjshlpm" tabindex="0" active="">Users</pf-tab>
      <pf-tab-panel id="pf-tab-panel-g6mjshlpm" tabindex="0" aria-labelledby="users">Users</pf-tab-panel>
      <pf-tab slot="tab" id="pf-tab-kvtmvh9o6" aria-controls="pf-tab-panel-29u49w0ww" tabindex="-1">Containers</pf-tab>
      <pf-tab-panel id="pf-tab-panel-29u49w0ww" tabindex="0" aria-labelledby="pf-tab-kvtmvh9o6" hidden="">Containers <a href="#">Focusable element</a></pf-tab-panel>
      <pf-tab slot="tab" id="pf-tab-h2hxamqi1" aria-controls="pf-tab-panel-ivkcx36yb" tabindex="-1">Database</pf-tab>
      <pf-tab-panel id="pf-tab-panel-ivkcx36yb" tabindex="0" aria-labelledby="pf-tab-h2hxamqi1" hidden="">Database</pf-tab-panel>
      <pf-tab slot="tab" disabled="" id="pf-tab-ec95bv707" aria-controls="pf-tab-panel-tw8aep4i5">Disabled</pf-tab>
      <pf-tab-panel id="pf-tab-panel-tw8aep4i5" tabindex="0" aria-labelledby="pf-tab-ec95bv707" hidden="">Disabled</pf-tab-panel>
      <pf-tab slot="tab" aria-disabled="true" id="pf-tab-lvtj31uqi" aria-controls="pf-tab-panel-9vlum7gix" tabindex="-1">Aria Disabled</pf-tab>
      <pf-tab-panel id="pf-tab-panel-9vlum7gix" tabindex="0" aria-labelledby="pf-tab-lvtj31uqi" hidden="">Aria Disabled</pf-tab-panel>
    </pf-tabs>
  </div>
Arathy-s commented 7 months ago

I believe the issue is occurring as the roles: region, tab, tabpanel, tablist are not mentioned.

bennypowers commented 7 months ago

Which browser and version? Which os?

Arathy-s commented 7 months ago

Browser: Chrome Version: 121.0.6167.139 OS: macOS Sonoma 14.3

The issue can be reproduced on running a lighthouse scan on https://patternflyelements.org/components/tabs/.

zeroedin commented 7 months ago

@bennypowers could this be a element internals/cross root aria issue, I'm leaning towards that it is. /cc @nikkimk

this.#internals.role = 'tab'; is set in PfTab and this.#internals.role = 'tabpanel'; is set in PfTabPanel, but those don't seem to get reported to the role="tablist" that is set on the slot in the parent PfTabs.

A work around that passes the lighthouse check is to manually set role="tab" and role="tabpanel directly on the pf-tab and pf-tab-panel elements.

Tabs currently does report role="tabs" in the Accessibility tree and we do test for that in the spec tests.

image

If it indeed a cross root aria issue, then we should have the element write these attributes out on the host element instead of trying to use element internals here (scratch that doing so seems to cause other issues in axe DevTools). Thoughts?

bennypowers commented 7 months ago

yeah my gut reaction was that this is a ua thing, but it would be better to try and rule out our own code first. One thing we can do to check this is screen reader testing

hellogreg commented 7 months ago

Doing some testing on this. One odd thing on the docs page is that, if you click on (or shift keyboard focus for) a tab in one of the demos, it also changes which tab is selected in the "Reacting to changes" HTML/Lit/React tablist!

If you click on the first tab in any example on the page (e.g., Users in this screencap), then HTML will be selected. If you click on the second tab (e.g., Containers below), then Lit gets selected.

screencap of the Tabs docs page

This video shows what I mean: Video of Tabs docs page issue

Not sure if this is at all related, but something I noticed in multiple browsers.

hellogreg commented 7 months ago

Did some basic real-world keyboard and screen reader testing too, with the following desktop combos:

First, keyboard usage without a screen reader on works in all browsers.

However, with screen readers on...

Mac

Safari and VoiceOver work fine for navigating the tabs. The screen reader switches to focus mode when focus is within the tab group, and the tabs can be operated via the arrow keys.

However, if you are just having VoiceOver read the page content, it does not read the individual tab headings. Instead, it announces the tab group, and then does so again for each of the tabs within.

In other words, for the following example...

screencap of a tab group

VoiceOver says, "Users and two more items, tab group," four times in a row. Once for the tab group, and once for each tab.

Windows

Interestingly, neither Chrome/JAWS nor Firefox/NVDA automatically enter focus mode for the tab groups. So the arrow keys do nothing, At least not at first.

In JAWS, the tabs can be navigated in browse mode via the apostrophe (and shift-apostrophe) key, and then selected. Focus mode cannot be entered manually (via the enter key), though this should be possible.

In NVDA, the user can enter focus mode manually (via the NVDA-space key combo), after which the tabs work as expected via the arrow keys.

Our tabs compared to WAI

I tested the WAI tabs example with the same combos.

The results:

bennypowers commented 7 months ago

I want to make some changes in TabsController which I believe will help with some of the issues raised here. see #2695. this boils down to cross-root aria issues with the shadow button in PfTab, so i'm exploring the option of using a div in place of button and exposing the tab semantics on the host instead

bennypowers commented 7 months ago

blocked by https://github.com/patternfly/patternfly-elements/pull/2677

bennypowers commented 6 months ago

@hellogreg @nikkimk @Arathy-s is this still an issue after #2677 and #2695 were both merged?