pkg/lib: Give empty "Th" elements a aria-label attribute

[mvollmer]

As requested by this warning:

Th: Table headers must have an accessible name. If the Th is
intended to be visually empty, pass in screenReaderText. If the Th
contains only non-text, interactive content such as a checkbox or
expand toggle, pass in an aria-label.

[martinpitt]

@jelly Marking as release-blocker. This causes zillions of console messages (see e.g. here), and would be really nice. But if it's too hard, please feel free to remove again.

[jelly]

@jelly Marking as release-blocker. This causes zillions of console messages (see e.g. here), and would be really nice. But if it's too hard, please feel free to remove again.

Already waiting on 2 other blockers so we can land this.

[martinpitt]

As nobody has a better idea, this should at least improve things for screenreaders and fix the warning. Thanks!

[garrett]

What is this? You shouldn't add ARIA unless it's needed. Here, it's not. This is definitely not the correct approach.

If I understand it correctly, this actually would make screenreader experience worse.

What is "Select"? What does it mean? What is this actually supposed to be describing? Why don't we just have the label as normal instead of adding ARIA? "Expander" probably also doesn't make sense out of context.

Where's the actual useful screen reader parts, like the scope attribute?

Got a nack by Garrett

[martinpitt]

OK, thanks @garrett . I have absolutely no idea what to do here, and you have too many things on your plate already. Dropping release-blocker.

[garrett]

Sorry; I'd like to help out in depth, but I'm way overburdened with everything right now. Perhaps soon? Hopefully?

It's important, but so is everything else. 😢

[mvollmer]

We have to shut up the Patternfly warning somehow, hopefully by making our tables more accessible. But if the correct thing for accessibility doesn't also shut up the warning, we haven't won yet.

In order to shut up the Patternfly warning we have to use aria-label or screenReaderText, or add children to the Th element.

Both screenReaderText and adding children to Th cause layout changes. Patternfly assigns some largish min-width to things in the thead element, except when they are empty. Setting screenReaderText adds a invisible child to the Th, which causes it to no longer be empty according to CSS match rules, and we get the unwanted largish min-width.

The same happens when adding a empty div into the Th. It shuts up the warning, but changes the layout.

So, this leaves us with aria-label, from the point of view of shutting up the warning. I vote to get his PR in, maybe with some tweaks to the actual label text, and file a issue to improve the accessibility for real (without triggering the warning again).

[mvollmer]

In order to shut up the Patternfly warning we have to use aria-label or screenReaderText, or add children to the Th element.

Or get Patternfly to change conditions for the warning.

[martinpitt]

Thanks for your investigations! The only remaining idea that I have is to try with an empty aria-label, and check if that quiesces PF. But I don't know how screenreaders react to this. But even if they said "empty", that'd be okay I think. On second thought it doesn't make sense to let them read "select" and right afterwards say "this is a checkbox".

But at this point, I'm good with basically any hack which quiesces this, as long as it comes with a bug reference, preferably against PF. They make it actively hard to do a selectable table line correctly, so perhaps they have some idea how it should be done.

[mvollmer]

The only remaining idea that I have is to try with an empty aria-label, and check if that quiesces PF.

Anything truthy will work, but a empty string is falsey. A single space shuts PF up.

[martinpitt]

A single space shuts PF up.

hmmkay, but that'd be even more confusing in screen readers, I figure. So I'd say let's go with this plus a "# HACK: Pf issue" reference?

[mvollmer]

They make it actively hard to do a selectable table line correctly, so perhaps they have some idea how it should be done.

The example for a Table with expandable rows uses <Th screenReaderText="Row expansion" />. Another example uses

<Th
            select={{
              onSelect: (_event, isSelecting) => selectAllRepos(isSelecting),
              isSelected: areAllReposSelected
            }}
            aria-label="Row select"
          />

Here is a good article about the difference between aria-label and screenReaderText (which is the PF version of Bootcamps sr-only class).

Conclusion:

I recommend using the .sr-only class whenever possible when adding accessible visually hidden content to a web page. Screen readers are not required to support ARIA. By using the CSS off-screen text method, you can ensure that the visually hidden content can be encountered by people using screen readers.

So both have the same goal, but aria-label doesn't work as well as we would hope. But I am happy to declare that a problem of screen readers.

screenReaderText unfortunately changes the layout, but maybe we can hack the CSS to not do that.

shrug

[mvollmer]

Updated to use the labels from the examples in the PatternFly documentation.

[mvollmer]

So I'd say let's go with this plus a "# HACK: Pf issue" reference?

What should the PF issue report be about? That the warning gives bad advice?

I will file a issue with PF about how using screenReaderText accidentally influences column width.

[mvollmer]

I will file a issue with PF about how using screenReaderText accidentally influences column width.

patternfly/patternfly#6643

[martinpitt]

Thanks! What a yak shave..

Overruled! :-)

[cockpituous]

These 2 added lines are not executed by any test.