Skip to content

Conversation

@abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented May 20, 2024

Description 📝

While aria-label can be used on any element that can have an accessible name, in practice however, it is supported only on interactive elements, widgets, landmarks, images, and iframes.

While aria-label can be used on any element that has an accessible name, it's best supported for interactive elements, widgets, landmarks, images, and iframes. In general, it shouldn't be used on static content. Using aria-label on non-interactive elements can cause a confusing user experience or the label may not be announced by assistive technology. For example, Microsoft's screen reader, JAWS, ignores aria-label on all static content except for content with “an interactive role, img role, or nav, main, search roles”.

also: https://stackoverflow.com/questions/72568204/what-can-aria-label-be-used-on

In the case of our table rows, having an aria label dos not really make sense:

  • Our tables already have accessible elements, which is encouraged albeit not being technically interactive

aria-label, aria-labelledby and aria-describedby work well on table, th and td elements with a few exceptions for NVDA, VoiceOver on iOS, and Talkback discussed in next section.
see this link

  • all we were doing (which this PR removes) is taking the label in the first column and use it as an aria label on the row. The table context provides enough accessibility with the table header and its content.

Changes 🔄

  • Remove ariaLabel on <TableRow />
  • Update components passing the prop

Preview 📷

No visual changes should be expected as part of this PR

How to test 🧪

Verification steps

  • There isn't much to verify here, except that our tests pass and the tag was properly removed from the rows affected by this PR

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@abailly-akamai abailly-akamai added the Accessibility Contains accessibility improvements or changes label May 20, 2024
@abailly-akamai abailly-akamai self-assigned this May 20, 2024
@abailly-akamai abailly-akamai marked this pull request as ready for review May 20, 2024 14:10
@abailly-akamai abailly-akamai requested a review from a team as a code owner May 20, 2024 14:10
@abailly-akamai abailly-akamai requested review from dwiley-akamai and hana-akamai and removed request for a team May 20, 2024 14:10
@github-actions
Copy link

github-actions bot commented May 20, 2024

Coverage Report:
Base Coverage: 81.53%
Current Coverage: 81.53%

@abailly-akamai abailly-akamai requested a review from a team as a code owner May 20, 2024 17:04
@abailly-akamai abailly-akamai requested review from jdamore-linode and removed request for a team May 20, 2024 17:04
Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

Doing a search for ariaLabel in the codebase, would the StatusIcon and StyledStatusIcon components be a good contender for removal as well since they aren't interactive?

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Code review ✅
Tests pass ✅

@abailly-akamai
Copy link
Contributor Author

@hana-linode yup! adding those to my epic, thanks for pointing it out

@hana-akamai hana-akamai added the Approved Multiple approvals and ready to merge! label May 21, 2024
@abailly-akamai abailly-akamai merged commit 0883798 into linode:develop May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Accessibility Contains accessibility improvements or changes Approved Multiple approvals and ready to merge!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants