fix(ui-table): don't describe a sort state on unsorted v1 tables - #2666
Open
drakeaharper wants to merge 1 commit into
Open
fix(ui-table): don't describe a sort state on unsorted v1 tables#2666drakeaharper wants to merge 1 commit into
drakeaharper wants to merge 1 commit into
Conversation
`getCaptionText` lost its early return for the unsorted case, so a v1
Table given a plain string caption has ' Sorted by undefined (undefined)'
appended to both its <caption> and its accessible name whenever no
column is sorted:
aria-label="Movies Sorted by undefined (undefined)"
That is user-facing for screen reader users. Restore the early return so
an unsorted table uses its caption as given, and keep the caption
function path untouched.
Found while upgrading canvas-lms from 11.7.4-SECURITY.3 to 11.7.4, where
it broke 10 vitest files and 6 Selenium specs that locate tables by
accessible name.
Adds a v1 test file. The existing Table tests import
`@instructure/ui-table/latest` and only ever pass a caption function, so
the v1 string-caption path had no coverage.
|
|
1 similar comment
|
|
Contributor
|
Contributor
Visual regression report
Diff images (33)alert.png — baseline no longer producedavatar.png — baseline no longer producedbadge.png — baseline no longer producedbillboard.png — baseline no longer producedbreadcrumb.png — baseline no longer producedbutton-and-derivatives.png — baseline no longer producedbyline.png — baseline no longer producedcalendar.png — baseline no longer producedcheckbox.png — baseline no longer producedcheckboxgroup.png — baseline no longer producedcolorpicker.png — baseline no longer producedcontextview.png — baseline no longer producedcustom-and-lucide-icons.png — baseline no longer produceddateinput-dateinput2.png — baseline no longer produceddatetimeinput.png — baseline no longer produceddiff-demo.png — 6324 pixels differdrilldown.png — baseline no longer producedfiledrop.png — baseline no longer producedform-errors.png — baseline no longer producedheading.png — baseline no longer producedimg.png — baseline no longer producedlink.png — baseline no longer producedmenu.png — baseline no longer producedmetric-pill-tag-timeselect-text.png — baseline no longer producedoptions.png — baseline no longer producedpagination.png — baseline no longer producedprogressbar.png — baseline no longer producedselect-simpleselect.png — baseline no longer producedtable.png — baseline no longer producedtabs.png — baseline no longer producedtooltip.png — baseline no longer producedtreebrowser.png — baseline no longer producedview.png — baseline no longer producedBaselines come from the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A v1
Tablegiven a plain string caption appends" Sorted by undefined (undefined)"to both its<caption>and its accessible name whenever no column is sorted:getCaptionTextlost its early return for the unsorted case between11.7.4-SECURITY.3and11.7.4:This is user-facing for screen reader users, not only a test problem. It still reproduces on
11.7.5-snapshot-14.Changes
caption-as-function path is untouched, and the cast matches what SECURITY.3 did (props.caption as string).packages/ui-table/src/Table/v1/__tests__/Table.test.tsx. The existingTabletests import@instructure/ui-table/latestand only ever pass a caption function, so the v1 string-caption path had no coverage — which is how this shipped. The new tests import relatively so they exercise source rather than built output.Verification
Movies Sorted by undefined (undefined)) and pass with the fix. The third covers the sorted case and passes either way, guarding against over-correcting.pnpm run test:vitest ui-table— 25 passed.pnpm run bootstrapclean, includingbuild:types(the first draft of this fix failed type-checking;aria-labelneeds astring).Context
Found upgrading canvas-lms off the Artifactory-only
11.7.4-SECURITY.3onto public11.7.4, where it broke 10 vitest files and 6 Selenium specs that locate tables by accessible name. Reported in #instui. Canvas is carrying apatch-packagepatch for this that we'll drop once a fixed release ships.One question for reviewers
componentDidUpdatestill has a commented-out condition:That looks like this change was mid-flight, and the accompanying comment says sort changes should only be announced for function captions. I've left it exactly as-is because I don't know the intent — if the live-region announcement is also meant to be gated, that's a follow-up (or tell me and I'll fold it in here).