cockpit-project / cockpit-machines

Cockpit UI for virtual machines
GNU Lesser General Public License v2.1
289 stars 74 forks source link

patternfly: Give empty "Th" elements a aria-label attribute #1635

Closed mvollmer closed 4 months ago

mvollmer commented 4 months ago

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.

We can't use screenReaderText because of

https://github.com/patternfly/patternfly/issues/6643
garrett commented 4 months ago

So... for this particular table, specifically for the icons, they're visual cues that repeat information already present in the strings (specifically, the "type" column, kind of). We should probably consider adding aria-hidden="true" to both the <th> and <td> elements for these icons. This should help assistive tech avoid the icons altogether, as I think they're unnecessary to announce and would be redundant... right?

Also, as we're talking about accessibility in this table, it is important to include the scope="col" attribute on other header cells (<th>) in the table for additional context.

We'd need to do something else for "empty" <th>s which actually label things that screenreader users care about, such as actions (for a big example)... and that's also in this table. .screenReaderText might work for that, and if it hits the same problem (and it might, especially on other languages), then we'd probably have to use aria-label="Actions" along with scope="col".

We'd also want scope="row" for the <th> elements on each line which identify the entry. (The disk names in this case.)

Having both scopes would let someone know the context for a cell.

Example from your screenshot:

image

You'd know the "unformatted data" is a type that applies to the sda disk due to the scopes.

I do see you've added the aria-label for actions, which is great, but we need more of the changes I mentioned in this comment. Thanks!

mvollmer commented 4 months ago

We'd also want scope="row" for the <th> elements on each line which identify the entry. (The disk names in this case.)

Do you maybe mean "scope=row for <td> elements"?

mvollmer commented 4 months ago

We'd also want scope="row" for the <th> elements on each line which identify the entry. (The disk names in this case.)

Do you maybe mean "scope=row for <td> elements"?

Or, should the cells with the name be th instead of the current td?

garrett commented 4 months ago

Do you maybe mean "scope=row for elements"?

Or, should the cells with the name be th instead of the current td?

Scope can be added to <td> elements also. Labelling names should usually be <th>.

Example from https://www.w3.org/WAI/WCAG21/Techniques/html/H63:

 <table border="1">
  <caption>Contact Information</caption>
  <tr>
    <td></td>
    <th scope="col">Name</th>
    <th scope="col">Phone#</th>
    <th scope="col">Fax#</th>
    <th scope="col">City</th>
  </tr><tr>
    <td>1.</td>
    <th scope="row">Joel Garner</th>
    <td>412-212-5421</td>
    <td>412-212-5400</td>
    <td>Pittsburgh</td>
  </tr><tr>
    <td>2.</td>
    <th scope="row">Clive Lloyd</th>
    <td>410-306-1420</td>
    <td>410-306-5400</td>
    <td>Baltimore</td>
  </tr><tr>
    <td>3.</td>
    <th scope="row">Gordon Greenidge</th>
    <td>281-564-6720</td>
    <td>281-511-6600</td>
    <td>Houston</td>
  </tr>
</table>

That's an old example, and it doesn't have ARIA at all (for the empty <td> in the heading row), nor does it use <thead> for the table's header row nor <tbody> for the table body. You can tell it's old because it's an example that's lacking those things and it uses a border="1" too. :laughing:


An aside...

Today I learned that things can get really complex with headers referencing headers, like this example: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/th#associate_header_cells_with_other_header_cells (it uses a headers="" attribute to point to the IDs of related headers). I'm glad we don't get that complex. :wink:

It's basically the Pronunciation associated with IPA and Respelling here, due to all the colspans and rowspans:

image

Just pointing it out that tables can get wild, and thankfully what we need to do isn't so much. However, PatternFly does have examples with this concept... https://www.patternfly.org/components/table/#nested-column-headers (and they're not doing the proper headers attribute thing... either they've dropped the ball and/or expect devs to know and add in the HTML attributes in the React)

mvollmer commented 4 months ago

Scope can be added to <td> elements also.

MDN says it is deprecated.

mvollmer commented 4 months ago

@garrett, I have tried to implement your suggestions here: https://github.com/cockpit-project/cockpit/pull/20471