[LG-3993] fix(Select): VO properly announces value and label #3218
+74
−5
Conversation
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
tsck
requested review from
Copilot and
shaneeza
and removed request for
a team
🦋 Changeset detectedLatest commit: 1d263e8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the Select component's VoiceOver (VO) accessibility by ensuring that both the aria-label and the selected value are announced together when an aria-label is provided, mimicking the behavior of native select elements.
Key changes:
- Added logic to compose aria-label with the selected value/placeholder for screen readers
- Implemented conditional composition that respects existing label hierarchy (visible label > aria-labelledby > aria-label)
- Added comprehensive test coverage for the new accessibility behavior
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/select/src/Select/Select.tsx | Added composedAriaLabel logic to combine aria-label with selected value for better screen reader announcements |
| packages/select/src/Select/Select.spec.tsx | Added four new test cases covering aria-label composition scenarios |
| packages/select/src/Select.stories.tsx | Added aria-label to LiveExample for manual accessibility testing |
| .changeset/pink-comics-go.md | Documented the accessibility fix in the changeset |
tsck
commented
Oct 13, 2025
| selectedOption !== null ? selectedOption.props.children : placeholder; | ||
|
|
||
| return `${ariaLabel}, ${selectedText}`; | ||
| }, [ariaLabel, ariaLabelledby, label, placeholder, selectedOption]); |
There was a problem hiding this comment.
This feels a bit hacky, but I'm not sure how else we would achieve this. Definitely open to ideas here if there's a better way to do it.
There was a problem hiding this comment.
I was thinking we would set aria-current in addition to the label? AFAIK that should also be announced
tsck
requested review from
a team and
TheSonOfThomp
and removed request for
a team and
shaneeza
|
Size Change: +20 B (0%) Total Size: 1.6 MB
ℹ️ View Unchanged
|
|
I noticed we don't have a default Also, can you update the internal |
…ribute for screen readers and ensuring proper announcement of `aria-label` with selected values.
Not sure I follow what needs to be updated on the internal
I asked Claude about this and it gave the following response: SummaryNo, a default role is not needed on the Select trigger — the current implementation is correct for accessibility. Here's why: Current Implementation ✅The Select component uses a native
Historical ContextI found that This was the correct decision because:
Optional EnhancementIf you want to be even more explicit about the relationship, you could consider adding Verdict: The Select component's accessibility implementation is correct as-is. The trigger doesn't need an explicit role attribute since it's a native button element. |
|
@tsck The DatePicker select elements are updating the aria label automatically based on the value. We should update that otherwise the select will be saying the year twice. |
|
Fair enough, Claude 😆 |
…r components for improved accessibility and consistency.
@TheSonOfThomp Oh gotcha! Updated |
tsck
requested review from
TheSonOfThomp
and removed request for
TheSonOfThomp
TheSonOfThomp
requested changes
Oct 16, 2025
| aria-labelledby={labelId} | ||
| aria-label={!label && !ariaLabelledby ? ariaLabel : undefined} | ||
| aria-label={composedAriaLabel} | ||
| aria-current={ariaCurrent} |
There was a problem hiding this comment.
aria-current should be set on the current item itself, not the outer element. (https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-current)
If we want to apply something on this element, I believe it should be aria-activedescendant (though I'm not sure that's necessary https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-activedescendant
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.
✍️ Proposed changes
Fixes
Selectso that when anaria-labelis added, VO announces both the value and the label when there is a value.🎟 Jira ticket: LG-3993
✅ Checklist
For bug fixes, new features & breaking changes
pnpm changesetand documented my changes🧪 How to test changes