Bug - Th - PF warns about missing aria-label for elements that have aria-hidden="true"

[mvollmer]

We have a empty Th and Patternfly rightfully demands it to be made accessible. However, in our case, the best thing is to hide the whole column from the screenreader with aria-hidden in every cell in that column. However, PF will still emit its warning for a Th component like this:

<Th aria-hidden="true" />

[mattnolting]

addressing in patternfly/patternfly#6272

[mvollmer]

addressing in patternfly/patternfly#6272

But, that issue has label "wontfix"...

Also, I can see how you plan to fix patternfly/patternfly#6643 in patternfly/patternfly#6272, but this here is about changing the condition in Th.tsx for showing the warning to

!children && !screenReaderText && !ariaLabel && ariaHidden != "yes"

Anyway, thanks a lot for addressing this!

[mattnolting]

But, that issue has label "wontfix"...

"wontfix" has been removed

@thatblindgeye transferring this issue to pf react

[thatblindgeye]

@mvollmer I took a look at the commits/PR where this issue was mentioned in Cockpit, and some clarification may help with addressing this.

From what I could tell, the warnings we have for the Th component are working as intended. In testing and some discussion regarding the issue that led to us introducing this warning, we had found that if a column is intended to have some sort of header (which would typically be most of the time if the other cells in the column contain valid data/content), it must be a Th and it must have an accessible name to try and improve accessibility. One exception is for something like a table where there may be column and row headers, and the top-left most cell is intended to be completely empty (in which case it should be a Td, like this table with 2 headers W3C example), which would definitely be valid/accessible.

What I could use some help with clarifying is the intended structure/interaction of the table where this issue was mentioned. I might be lacking some context, so if I am please let me know, but it looks like a table is being built with a column that has td elements that only contain icons, and those td's have some click handler attached. If clicking within those elements is necessary for some sort of action, then I would think that the click functionality should be exposed to assistive tech (vs now it may only be via a primary mouse button click?) and the header should remain as a Th with an accessible name (either via visible text or the screenReaderText prop). Without the Th component for the column, any other cells exposed to AT in that column don't have any semantic heading which may lead to confusion.

Is there a specific reason why this entire column may need to be hidden from assistive tech in the first place?

[mvollmer]

@mvollmer I took a look at the commits/PR where this issue was mentioned in Cockpit, and some clarification may help with addressing this.

Thanks! This is very much appreciated!

From what I could tell, the warnings we have for the Th component are working as intended.

In general, I agree, but I think this bug report here is still valid. Evenif we don't end up doing it in this specific case, it is still allowed to put aria-hidden on a th in general, and in that case, no warning should be emitted about its content, no?

I might be lacking some context, so if I am please let me know, but it looks like a table is being built with a column that has td elements that only contain icons, and those td's have some click handler attached.

That handler is used for navigating to a new page with more details about the item represented by the row. Ideally, the click handler would be on the tr, but I couldn't make that work, probably because of the dropdown menu in the last cell in each row. (Opening the dropdown would trigger the navigation handler as well, or something like that.) So as a workaround, we now have click handlers on each td except the one with the menu.

Is there a specific reason why this entire column may need to be hidden from assistive tech in the first place?

We felt it doesn't hurt to hide it since it is decorative and redundant with the "Id" column, and hiding it does solve the accessibility issue. Maybe the icons should not be in their own column, and be part of the "Id" one?

An alternative would be to not hide the icon column, give it the accessibility label "Category" and add fitting labels like "Disk", "Volume", "Network" to the icons.

A bigger accessibility issue is actually the way we convey the hierearchical relationships of the rows in the table. This is done purely by messing with some CSS margins and is not exposed to AT at all...

[mvollmer]

@thatblindgeye transferring this issue to pf react

Oh, sorry, I could have filed this directly in patternfly-react.

[thatblindgeye]

@mvollmer no worries!

In general, I agree, but I think this bug report here is still valid. Evenif we don't end up doing it in this specific case, it is still allowed to put aria-hidden on a th in general, and in that case, no warning should be emitted about its content, no?

Initially I might be hesitant to omit the warning for a th that has aria-hidden="true" just because I'd be wary about hiding a table header in general. Since semantically the th should provide a heading for a column, it feels like that it shouldn't ever really be hidden and that if absolutely necessary, maybe the workaround of using a td instead is okay.

There's also a worry of aria-hidden being used solely to get rid of the warning - that is, hiding the header but not hiding anything else in the column. That said, that can still be achieved by just using an empty/hidden td which we can't (and shouldn't since there's valid cases for an empty td element) warn about. All that to say I may not be totally against updating the logic for this warning to include aria-hidden, but just to provide some additional context from my perspective.

We felt it doesn't hurt to hide it since it is decorative and redundant with the "Id" column, and hiding it does solve the accessibility issue. Maybe the icons should not be in their own column, and be part of the "Id" one?

Ah gotcha. Yeah, I almost feel like if the icons are really just decorative, putting them in another column that has some other meaningful content may be better: it doesn't require hiding an entire column from AT, and it may clear up whatever space this extra column may be taking up. Or as you said, providing additional context to the icon column and giving it a heading. That said, if a separate column is absolutely necessary/desired, hiding the entire column as you are in your Cockpit PR may be the next better alternative.

That handler is used for navigating to a new page with more details about the item represented by the row. Ideally, the click handler would be on the tr, but I couldn't make that work, probably because of the dropdown menu in the last cell in each row. (Opening the dropdown would trigger the navigation handler as well, or something like that.) So as a workaround, we now have click handlers on each td except the one with the menu.

This is out of scope of the current ask/issue, but just to mention that interactive cells/rows like this sort of use case have always felt trickier from an a11y perspective 😆 One issue with applying click handlers to tr or cells themselves is by default they won't be interactive via plain keyboard. Having other interactive content like a dropdown menu definitely makes it trickier. We have a clickable table rows example which doesn't exactly fit your use case with a dropdown menu being present, but I believe we either have or had a demo that has clickable table rows with some sort of select menu in the rows that would better match your particular implementation of Table. If I can find it I'll share it with you, but I want to say we may have ended up just checking in the onClick whether the event target was the menu toggle and if not fire whatever logic for row click.

Again, this is out of scope for your current ask, but wanted to share in case you'd find it helpful/interesting

[mvollmer]

Initially I might be hesitant to omit the warning for a th that has aria-hidden="true" just because I'd be wary about hiding a table header in general.

So, another warning about Th with aria-hidden? :-)

But more seriously, your arguments make sense to me and I wont fight you if you close this as wontfix.

Thanks a lot for your explanations, they have helped me understand a11y a lot more and I guess I can dtrt on my own more often now.

This is out of scope of the current ask/issue, but just to mention that interactive cells/rows like this sort of use case have always felt trickier from an a11y perspective 😆
[...]

There is also the pattern where the content of the first column is turned into a link, like we use in cockpit-machines:

That seems to avoid a lot of the problems of clickable rows.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[github-actions]

This issue has been closed because it has not had activity since being marked as stale.