[6.x] Fix listing search label and empty table header cells - #15406
[6.x] Fix listing search label and empty table header cells#15406bpmore wants to merge 2 commits into
Conversation
The listing search field was hardcoded to "Search entries" and to a fixed id, so the asset browser and users listing told screen reader users they were searching entries, and two listings on one page produced duplicate ids. The select and actions header cells rendered empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013YZV7JD2cyMHSeSu2fUXCK
jackmcdade
left a comment
There was a problem hiding this comment.
The label prop needs to follow the same pattern as every other label in the CP: translate at the call site, render the already-translated string in the component. Don't only wrap the call sites — Search.vue already runs __(label), so you'd double-translate.
| <div class="flex items-center gap-2 sm:gap-3 py-3 relative overflow-clip st-overflow-clip-margin"> | ||
| <div class="flex flex-1 items-center gap-2 sm:gap-3"> | ||
| <ListingSearch /> | ||
| <ListingSearch label="Search assets" /> |
There was a problem hiding this comment.
Needs the translation helper, same as every other label prop:
<ListingSearch :label="__('Search assets')" />| <div class="flex items-center gap-2 sm:gap-3 mb-4"> | ||
| <div class="flex flex-1 items-center gap-2 sm:gap-3"> | ||
| <Search ref="search" /> | ||
| <Search ref="search" label="Search assets" /> |
There was a problem hiding this comment.
Same here:
<Search ref="search" :label="__('Search assets')" />| <template> | ||
| <div class="flex-1 max-w-sm" :class="{ 'max-w-60!': activeFilterBadgeCount > 2 }"> | ||
| <label for="listings-search" class="sr-only">{{ __('Search entries') }}</label> | ||
| <label :for="id" class="sr-only">{{ label ? __(label) : __('Search') }}</label> |
There was a problem hiding this comment.
Once the call sites pass an already-translated string, don't wrap label again:
{{ label || __('Search') }}Follows the CP convention: call sites pass an already-translated string and the component renders it as given. Search.vue was running __() on the prop, so once the call sites used __() the string would be translated twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013YZV7JD2cyMHSeSu2fUXCK
|
Thanks — you're right on all three, and the second one was a real bug rather than a style point.
I checked the convention before changing it rather than taking it on faith: no component under Tests: the 5 in One thing I tried and abandoned, in case you'd want it done properly: a regression test that seeds Pushed as a normal commit on top rather than a rebase, so your line comments stay anchored. |
Two small accessibility fixes in
ui/Listing/, found during a WCAG 2.1 AA audit of the CP. Filing as a PR rather than issues since both are a few lines.1. The search field says "Search entries" on listings that hold no entries
Search.vuehardcoded the label:On
/cp/assets/browse/assetsand/cp/usersa screen reader user is told they are searching entries while looking at files or people. WCAG 2.1 SC 2.4.6 (AA).The same line hardcodes
id="listings-search", which is also bound to the input. Two listings rendered on one page (the asset selector opened over a listing, for example) produce duplicate ids and a label pointing at whichever one wins.Changed to:
labelprop, defaulting to the listing-agnostic'Search'— accurate everywhere, and no worse than today on any screenuseId()for the input id, matching whatSwitch.vueand friends already doPassed
label="Search assets"at the two asset call sites. The generic default covers the rest; individual listings can opt in as their nouns are known.Note:
'Search entries'is no longer referenced. Left the translation key in place rather than touching lang files in an a11y PR — say the word if you'd like it removed.2. Empty
<th>cellsThe select/reorder column and the actions column rendered header cells with no content, so screen readers announce an empty column header when moving across a row. axe-core
empty-table-headerfired on 9 screens.Added
sr-onlynames:Select/Reorderfor the first column when no toggle-all is rendered, andActionsfor the last.Tests
Added
resources/js/tests/components/ListingSearch.test.jswith 5 cases:Search4 of the 5 fail against the unpatched components; the association test passes either way and stays as a regression guard.
Full unit suite: 568 passed, 9 failed. Those 9 are pre-existing
LivePreviewfailures on a clean6.xcheckout — identical without this branch. Net effect: +5 tests, no change to existing results.Notes
AI tools were used to draft this; the changes, the tests, and the before/after runs were reviewed by hand. Happy to split this into two PRs if you'd rather review them separately.