[EuiColorPicker] Ensure EuiFormRow label is applied to EuiColorPicker#9436
Conversation
mgadewoll
force-pushed
the
colorpicker/improve-a11y-labelling-in-form-row
branch
from
dfaa024 to
d85373b
Compare
|
💭 I think the "before" and "after" screenshots are swapped 😄 not a big deal. |
You're right! Thanks for the catch. It's corrected now. |
There was a problem hiding this comment.
Screen readers now announce a meaningful input label 🎉 Through testing I noticed we have an issue with the hint text now.
💭 Non-blocking idea: relying on snapshot tests for asserting these different cases is not ideal. I would improve the testing suite and directly assert the name and ariaDescribedById text, both standalone and within EuiFormRow, and passing custom hint text.
Testing notes
I tested the Playground and InFormRow stories from staging.
NVDA works, Jaws works but has some weird behaviors, VoiceOver has regressed.
I didn't test custom hint text passed through aria-describedby because classNames(ariaDescribedById, ariaDescribedby) is trivial.
❌ VoiceOver + Safari (MacOS)
❌ Standalone with aria-label
✅ reads aria-label
✅ reads aria-describedby hint text
✅ reads closeLabel when closed
❌ reads openLabel when open
When the popover is open, it does not read the openLabel even when refocused.
Kapture.2026-03-17.at.12.40.43.mp4
In production it's not working ideally but the openLabel/closeLabel dynamic change is announced by the VoiceOver when refocusing the input:
Kapture.2026-03-17.at.12.34.29.mp4
Should we use aria-expanded instead?
❌ Inside EuiFormRow
✅ reads aria-label
✅ reads aria-describedby hint text
❌ reads closeLabel when closed
❌ reads openLabel when open
This is even weirder, the popover is not open and it reads closeLabel:
Kapture.2026-03-17.at.12.43.45.mp4
⚠️ Jaws + Chrome (Windows in ParallelsVM)
🔊 Turn on the sound (sorry for the quiet sound but the Mac is under my desk)
⚠️ Standalone with aria-label
✅ reads aria-label
✅ reads aria-describedby hint text
✅ reads openLabel when open
closeLabel when closed - with a caveat that when we close the popover, it doesn't read the closeLabel again
Kapture.2026-03-17.at.13.10.22.mp4
To be fair, in production it says both "Press the ESC key to close the popover" and Jaws-specific "To close the dialog press...":
Kapture.2026-03-17.at.13.07.13.mp4
but it's less confusing then duplicating the same instruction.
⚠️ Inside EuiFormRow
✅ reads aria-label
✅ reads aria-describedby hint text
✅ reads openLabel when open
closeLabel when closed
Kapture.2026-03-17.at.13.12.42.mp4
Same issues as above.
✅ NVDA + Chrome (Windows in ParallelsVM)
✅ Standalone with aria-label
✅ reads aria-label
✅ reads aria-describedby hint text
✅ reads openLabel when open
✅ reads closeLabel when closed
Kapture.2026-03-17.at.12.56.08.mp4
✅ Inside EuiFormRow
✅ reads aria-label
✅ reads aria-describedby hint text
✅ reads openLabel when open
✅ reads closeLabel when closed
Kapture.2026-03-17.at.12.58.13.mp4
I'm not sure it's a regression per se. I can see the same behavior though on production (still using Screen.Recording.2026-03-17.at.14.39.30.movI've done a bit of checking on the base behavior with native HTML elements in VoiceOver.
Screen.Recording.2026-03-17.at.14.21.11.movIn our implementation the VO navigation somehow returns to the inner placeholder element instead of the input, which is weird. Screen.Recording.2026-03-17.at.14.29.44.movWhen testing it with the base HTML elements it did the same when moving focus manually for every other time 🤷♀️ Screen.Recording.2026-03-17.at.14.55.54.mov
This could be due to what I noticed on stale content linking:
I quickly tested it, it still works as expected in NVDA and JAWS, we should be ok to update it. |
|
Thanks for digging deeper, Lene! 🙌🏻 It definitely looks like VO quirk, |
- this works more reliable in VO to prevent stale hint announcements on navigation
| aria-label={ariaLabel} | ||
| aria-labelledby={ariaLabelledby} | ||
| aria-describedby={classNames(ariaDescribedById, ariaDescribedby)} | ||
| aria-describedby={classNames( |
There was a problem hiding this comment.
This change is to prevent stale aria-describedby announcements on navigation in VO (as mentioned here)
There was a problem hiding this comment.
Pull request overview
This PR improves EuiColorPicker accessibility by allowing EuiFormRow-provided labeling (and consumer-provided aria-label) to correctly apply to the underlying input, while moving the existing open/close guidance text into screen reader descriptions.
Changes:
- Updated
EuiColorPickerto stop overriding the input’s accessible name and instead expose open/close guidance viaaria-describedby+ SR-only content. - Added Storybook coverage for
EuiColorPickerinsideEuiFormRowand fixed an incorrect import path. - Extended
@elastic/eslint-plugin-eui’sno-unnamed-interactive-elementrule to includeEuiColorPicker, with accompanying tests and changelog entries.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/eui/src/components/color_picker/color_picker.tsx | Routes aria-label/aria-labelledby to the input and moves open/close instructions to aria-describedby via SR-only elements. |
| packages/eui/src/components/color_picker/color_picker.stories.tsx | Adds an EuiFormRow story and adjusts story args/imports to support QA of labeling behavior. |
| packages/eui/src/components/color_picker/snapshots/color_picker.test.tsx.snap | Updates snapshots for the new aria-describedby + SR-only markup. |
| packages/eui/changelogs/upcoming/9436.md | Adds an EUI changelog entry for the accessibility fix. |
| packages/eslint-plugin/src/rules/a11y/no_unnamed_interactive_element.ts | Includes EuiColorPicker in the unnamed interactive element lint rule. |
| packages/eslint-plugin/src/rules/a11y/no_unnamed_interactive_element.test.ts | Adds test cases ensuring EuiColorPicker is flagged when missing accessible labeling. |
| packages/eslint-plugin/changelogs/upcoming/9436.md | Adds an eslint-plugin changelog entry for the rule update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| aria-label={ariaLabel} | ||
| // if an id is provided it might be used in combination with `htmlFor` on a label, | ||
| // so we don't want to override it with a fallback `aria-label` | ||
| aria-label={id ? undefined : _ariaLabel ?? ariaLabel} |
There was a problem hiding this comment.
This results in: EuiFormRow label > custom aria-label > internal fallback label.
Interaction taken:
- focus input
- press
arrowDown - tab into the popover
- press
Escape
| scenario | NVDA | JAWS | VO |
|---|---|---|---|
with EuiFormRow |
 |
 |
Screen.Recording.2026-03-17.at.17.00.05.mov |
with fallback aria-label |
 |
 |
Screen.Recording.2026-03-17.at.16.59.21.mov |
Fyi, the VO behavior is slightly better when the next element is not the same type (leaving the frame and going back to the same input element doesn't count) but it still reads the generic semantic information anyway. 🤷♀️
There was a problem hiding this comment.
I realized just now that the order was still not quite correct.
If a custom aria-label is passed, that would be on purpose and hence should override the form label, imho.
Updated in 2f6923b.
The new order is:
Custom aria-label > EuiFormRow label > internal fallback label.
There was a problem hiding this comment.
Definitely agreed about the order!
💚 Build SucceededHistory
cc @mgadewoll |
💚 Build Succeeded
History
cc @mgadewoll |
weronikaolejniczak
left a comment
weronikaolejniczak
left a comment
There was a problem hiding this comment.
🟢 Code LGTM, Safari + VO works as expected now, and there's no regression for JAWS and NVDA. Thank you for digging deeper and figuring out a smart solution to this, Lene 🙏🏻
Testing notes
🟢 Safari + VoiceOver
| 🟢 [Standalone] Fallback value | 🟢 [Standalone] Custom aria-label |
🟢 [Form row] Label | 🟢 [Form row] Custom aria-label |
|---|---|---|---|
Kapture.2026-03-19.at.15.55.49.mp4 |
Kapture.2026-03-19.at.15.57.38.mp4 |
Kapture.2026-03-19.at.15.58.28.mp4 |
Kapture.2026-03-19.at.15.59.29.mp4 |
🟢 JAWS + Chrome
| 🟢 [Standalone] Fallback value | 🟢 [Standalone] Custom aria-label |
🟢 [Form row] Label | 🟢 [Form row] Custom aria-label |
|---|---|---|---|
Kapture.2026-03-19.at.16.20.59.mp4 |
Kapture.2026-03-19.at.16.22.14.mp4 |
Kapture.2026-03-19.at.16.25.43.mp4 |
Kapture.2026-03-19.at.16.24.39.mp4 |
🟢 NVDA + Chrome
NVDA actually prioritizes that aria-label instead of the id + htmlFor.
| 🟢 [Standalone] Fallback value | 🟢 [Standalone] Custom aria-label |
🟢 [Form row] Label | 🟢 [Form row] Custom aria-label |
|---|---|---|---|
Kapture.2026-03-19.at.16.15.28.mp4 |
Kapture.2026-03-19.at.16.16.35.mp4 |
Kapture.2026-03-19.at.16.17.47.mp4 |
Kapture.2026-03-19.at.16.18.46.mp4 |
Summary
closes #9388
This PR updates
EuiColorPickerto ensure that usages with a wrappingEuiFormRowcan correctly pass downaria-labelto the input.To ensure this, the PR changes the usage of
openLabelandcloseLabelfrom being applied viaaria-labeltoaria-describedby. This ensures that a dynamicaria-labelattribute can be passed while we still keep the previous labels as hint texts for additional context.Additional changes
no-unnamed-interactive-elementin@elastic/eslint-plugin-euito include checkingEuiColorPickerfor missing accessible labelsWhy are we making this change?
Screenshots #
Screen.Recording.2026-03-09.at.15.49.15.mov
Screen.Recording.2026-03-09.at.15.47.52.mov
Impact to users
🟢 No code updates are required on consumer side.
ℹ️ Due to the DOM changes, snapshot tests will fail.
QA
EuiColorPickerhas the previous open/close labels applied asaria-describedbyaria-labelcan be passed toEuiColorPickerEuiColorPickerinput has the correct label inherited when nested within aEuiFormRowGeneral checklist
@defaultif default values are missing) and playground toggles