Associate search input with visible heading label - #2027
Associate search input with visible heading label#2027jacobo-dominguez-wgu wants to merge 2 commits into
Conversation
|
Thanks for the pull request, @jacobo-dominguez-wgu! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
0e9d74d to
47e0b96
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2027 +/- ##
=======================================
Coverage 93.53% 93.53%
=======================================
Files 363 363
Lines 5905 5905
Branches 1367 1367
=======================================
Hits 5523 5523
Misses 367 367
Partials 15 15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR improves accessibility in the Courseware Search modal by making the visible “Search this course” heading the programmatic label for the search input, removing the prior redundant hidden label behavior, and updating the placeholder to be a more informative hint.
Changes:
- Associate the search input with the visible heading using
aria-labelledbyand a stable headingid. - Remove the
SearchField.Labelrendering to avoid duplicate SR label output. - Update i18n placeholder text and add/adjust tests to validate the accessible name and label association.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/course-home/courseware-search/messages.ts | Updates placeholder message to a more descriptive hint for the search input. |
| src/course-home/courseware-search/CoursewareSearchForm.jsx | Removes SearchField.Label and adds aria-labelledby support via a new labelledBy prop. |
| src/course-home/courseware-search/CoursewareSearch.jsx | Adds id to the visible heading and wires it into the form via labelledBy. |
| src/course-home/courseware-search/CoursewareSearchForm.test.jsx | Adds tests for aria-labelledby and guarding against reintroducing the duplicate hidden label. |
| src/course-home/courseware-search/CoursewareSearch.test.jsx | Adds an integration-style assertion that the input’s accessible name includes the visible heading text. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Thanks for the PR!
It does seem there are some a11y things to address here, I just don't fully agree the changes in this PR are the best way to address them.
I'd like to get @kblemel's thoughts on the screenreader label aspects of this.
| searchBarPlaceholderText: { | ||
| id: 'learn.coursewareSearch.searchBarPlaceholderText', | ||
| defaultMessage: 'Search', | ||
| description: 'Placeholder text for the Courseware Search input control', | ||
| defaultMessage: 'Find topics across this course', | ||
| description: 'Placeholder hint shown inside the Courseware Search input describing what can be searched.', | ||
| }, |
There was a problem hiding this comment.
Do we need to change the placeholder text here? Having "Find topics across this course" as placeholder text feels odd to me, and this would require new translations.
There was a problem hiding this comment.
The suggestion from a11y internal team was: If placeholder text is used, it should provide a helpful example or hint rather than repeat the accessible label. 'Find topics across this course' was AI generated but I am open to hear options for placeholder text.
Description
Fixes an accessibility issue in the Courseware Search modal where the search input had a visually-hidden
"Search"label that duplicated the placeholder, while the visible"Search this course"heading was not programmatically associated with the input. Screen reader users heard redundant label information and missed the more descriptive visible title.Problem
"Search") rendered by<SearchField.Label />that repeated the placeholder text — screen readers announced the same label twice.<h1>Search this course</h1>above the input was not connected to it viafor/idoraria-labelledby, so assistive tech users never received that context."Search") was also non-informative — it just repeated the hidden label instead of hinting at what could be searched.Fix
Associate the visible heading with the input via
aria-labelledby, remove the redundant hidden label, and repurpose the placeholder as a real hint describing what the search covers.Testing
Screenshots
AI usage notice
Used Claude Opus 4.7 to assist on this creation of this pr.