Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

mvollmer
Copy link
Member

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

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

    patternfly/patternfly#6643
@mvollmer mvollmer force-pushed the pf-access-empty-table-headers branch from ce7088a to 183813d Compare May 14, 2024 09:49
@mvollmer mvollmer requested a review from jelly May 14, 2024 10:01
@@ -527,7 +528,7 @@ export class VmNetworkTab extends React.Component {
let networkId = 1;
detailMap = detailMap.filter(d => !d.hidden);

const columnTitles = detailMap.map(target => target.name);
const columnTitles = detailMap.map(target => ({ title: target.name, props: { "aria-label": target.aria } }));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aria can be null here right? So we should probably not pass an aria-label which is null.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's undefined. There will be no "aria-label" attribute when aria is undefined (or null, actually).

@garrett
Copy link
Member

garrett commented May 14, 2024

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

garrett commented May 15, 2024

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. 😆


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. 😉

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
Copy link
Member Author

Scope can be added to <td> elements also.

MDN says it is deprecated.

@mvollmer
Copy link
Member Author

@garrett, I have tried to implement your suggestions here: cockpit-project/cockpit#20471

@mvollmer mvollmer requested review from martinpitt and jelly May 21, 2024 05:52
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mvollmer mvollmer merged commit e7dd5bb into cockpit-project:main May 21, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants