Fix broken ComparisonTable columns on screen readers #694
Conversation
🦋 Changeset detectedLatest commit: 7c6faac The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🟢 No design token changes found |
joshfarrant
temporarily deployed
to
github-pages
GitHub Actions
Inactive
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
joshfarrant
force-pushed
the
joshfarrant/a11y-3753
branch
from
3f00da5 to
ac54a41
Compare
joshfarrant
temporarily deployed
to
github-pages
GitHub Actions
Inactive
joshfarrant
temporarily deployed
to
github-pages
GitHub Actions
Inactive
| @@ -55,8 +55,7 @@ export const _ComparisonTable = forwardRef( | |||
| children: React.Children.map(child.props.children, (rowChild, rowChildIndex) => { | |||
| if (rowChild.type === Cell) { | |||
| return React.cloneElement(rowChild, { | |||
| as: 'th', | |||
| 'aria-hidden': !rowChild.props.children ? 'true' : undefined, | |||
| as: rowChild.props.children ? 'th' : 'td', | |||
There was a problem hiding this comment.
Is the conditional behaviour needed here? Wouldn't it always be a th for the first row?
There was a problem hiding this comment.
Sorry, I should have added context on the th/td behaviour here (and checked the snapshots after making the change!).
I added this as axe was complaining about empty th cells. You can see the rule here
https://dequeuniversity.com/rules/axe/4.7/empty-table-header?application=axeAPI
I couldn't find a recommended approach for this kind of 2-way table where the top-left cell doesn't have any meaning, hence why I landed on the td approach.
Thanks for spotting the CSS issues. Fixing the CSS might take a bit of extra work as there are a lot of selectors which expect that first cell to be a th.
Let me dig into this a bit more and find an approach that works.
There was a problem hiding this comment.
Based on the suggestion from Tetralogical I've opted to keep this as a <td> and have fixed the style issues
There was a problem hiding this comment.
Related to https://github.com/primer/brand/pull/694/files#r1718673566. You can see the column misalignment in this diff 👇
joshfarrant
temporarily deployed
to
github-pages
GitHub Actions
Inactive
joshfarrant
temporarily deployed
to
github-pages
GitHub Actions
Inactive
There was a problem hiding this comment.
Can't for life of me figure out what's changed here 😄 ... any ideas?
There was a problem hiding this comment.
No me neither — I've had this a few times recently, I don't know if it's related to me running the snapshots locally vs using the label
There was a problem hiding this comment.
Worth removing these changes, or fixing it in a follow. up. FWIW I usually trust my local machine to regen vs Actions...
Summary
Removed the
aria-hiddenattribute from empty<th>cells which was causing table structure to be misrepresented by screen readersList of notable changes:
aria-hiddenfromComparisonTablecolumn headersWhat should reviewers focus on?
Steps to test:
Supporting resources (related issues, external links, etc):
Contributor checklist:
Reviewer checklist:
Videos
Note the mis-reported column numbers in the first video.
ComparisonTable-before.mp4
ComparisonTable-after.mp4