A11y: Improve boolean control contrast in forced colors mode#34204
Conversation
📝 WalkthroughWalkthroughRefactored the Boolean control component's styling by extracting styles into a helper function, shifting focus/forced-colors handling from input-level to label-level with focus-within selectors, and replacing explicit layout properties with srOnlyStyles spreading. Added two E2E tests validating forced-colors behavior and hover interactions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/addons/docs/src/blocks/controls/Boolean.test.tsx (1)
12-65: Consider adding a dark theme test case.The test thoroughly validates the forced-colors styling contract for the light theme. While the theme-dependent boxShadow logic (line 103-105 in Boolean.tsx) is overridden to
'none'in forced-colors mode anyway, adding a quick test withthemes.darkwould provide additional confidence that the forced-colors styles remain consistent regardless of theme base.💡 Optional: Add dark theme coverage
+ it('emits consistent forced-colors styles for dark theme', () => { + const styles = getBooleanControlStyles(convert(themes.dark)); + const selectedStyles = asCssObject( + styles['input:checked ~ span:last-of-type, input:not(:checked) ~ span:first-of-type'] + ); + const selectedForcedColors = asCssObject(selectedStyles['@media (forced-colors: active)']); + + expect(selectedForcedColors).toMatchObject({ + forcedColorAdjust: 'none', + background: 'Highlight', + color: 'HighlightText', + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/docs/src/blocks/controls/Boolean.test.tsx` around lines 12 - 65, Add a parallel assertion block that runs the same forced-colors checks against the dark theme: call getBooleanControlStyles(convert(themes.dark)) (same helpers: asCssObject, accessors for '@media (forced-colors: active)', '&:focus-within', styles.input, styles.span, and the selectedStyles selector) and assert the same properties (background, outline, forcedColorAdjust, color, boxShadow, absence of textDecoration, etc.); you can duplicate the existing it(...) body into a second it(...) describing "emits explicit forced-colors styles for the selected state (dark theme)" or parameterize the test to iterate [themes.light, themes.dark] while keeping references to getBooleanControlStyles and the selectedStyles selector.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/addons/docs/src/blocks/controls/Boolean.test.tsx`:
- Around line 12-65: Add a parallel assertion block that runs the same
forced-colors checks against the dark theme: call
getBooleanControlStyles(convert(themes.dark)) (same helpers: asCssObject,
accessors for '@media (forced-colors: active)', '&:focus-within', styles.input,
styles.span, and the selectedStyles selector) and assert the same properties
(background, outline, forcedColorAdjust, color, boxShadow, absence of
textDecoration, etc.); you can duplicate the existing it(...) body into a second
it(...) describing "emits explicit forced-colors styles for the selected state
(dark theme)" or parameterize the test to iterate [themes.light, themes.dark]
while keeping references to getBooleanControlStyles and the selectedStyles
selector.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc008b5b-17dc-4330-afeb-acb5caf9ed61
📒 Files selected for processing (2)
code/addons/docs/src/blocks/controls/Boolean.test.tsxcode/addons/docs/src/blocks/controls/Boolean.tsx
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Could you please mention the list of browsers used for testing, and if possible, provide screenshots? I've had a bunch of High Contrast Mode PRs in the past where I couldn't reproduce the fix as each browser may choose to use different system colours by default. It helps if I know which browsers were buggy initially! Thanks ❤️
Sidnioulz
changed the title
Sidnioulz
added
bug
windows
ci:normal
a11y: contrast
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks for the PR! This is a great improvement :)
I'm seeing a side effect when emulation is off: hovering over the inactive option adds an odd border/shadow, which complicates the UI and makes toggle animations look off. We don't need a contrast ratio on hover (SC 1.4.11, G183).
Could you please address that bug and switch the tests to using Playwright? 🙏
valentinpalkovic
moved this from Empathy Queue (prioritized)
to In Progress
in Core Team Projects
Yes, that was a real side effect. I removed the hover shadow from the inactive option in the default mode so the toggle no longer picks up that extra pseudo-border and the selected state remains visually cleaner. I also tested this in Chrome on Windows 10, and I no longer see this side effect. Please let me know if the issue still reproduces for you after the latest commit. Thanks! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/e2e-tests/addon-controls.spec.ts (1)
129-157: Decouple selected/unselected assertions from default story args.This test currently binds “selected” to
trueOption. If the story default forprimarychanges, the test may fail for the wrong reason. Prefer deriving selected/unselected frominput.isChecked()first.♻️ Proposed refactor
- const falseOption = label.locator('span').first(); - const trueOption = label.locator('span').last(); + const falseOption = label.locator('span').first(); + const trueOption = label.locator('span').last(); + const isChecked = await input.isChecked(); + const selectedOption = isChecked ? trueOption : falseOption; + const unselectedOption = isChecked ? falseOption : trueOption; @@ - await expect(trueOption).toHaveCSS('forced-color-adjust', 'none'); + await expect(selectedOption).toHaveCSS('forced-color-adjust', 'none'); @@ - const selectedBackground = await trueOption.evaluate( + const selectedBackground = await selectedOption.evaluate( (el) => getComputedStyle(el).backgroundColor ); - const unselectedBackground = await falseOption.evaluate( + const unselectedBackground = await unselectedOption.evaluate( (el) => getComputedStyle(el).backgroundColor );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/e2e-tests/addon-controls.spec.ts` around lines 129 - 157, The test assumes trueOption is the selected option; instead query the radio state via input.isChecked() and derive selectedOption/unselectedOption accordingly (use the existing input locator and the label.locator('span').first()/last() to map): call await input.isChecked() and set selected = isChecked ? trueOption : falseOption and unselected = isChecked ? falseOption : trueOption, then replace assertions that reference trueOption/falseOption for selected/unselected visuals with selected/unselected variables (e.g., selectedBackground/unselectedBackground and the box-shadow/forced-color-adjust expectations) so the test passes regardless of the story's default primary value while keeping the focus/outline checks unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/e2e-tests/addon-controls.spec.ts`:
- Around line 129-157: The test assumes trueOption is the selected option;
instead query the radio state via input.isChecked() and derive
selectedOption/unselectedOption accordingly (use the existing input locator and
the label.locator('span').first()/last() to map): call await input.isChecked()
and set selected = isChecked ? trueOption : falseOption and unselected =
isChecked ? falseOption : trueOption, then replace assertions that reference
trueOption/falseOption for selected/unselected visuals with selected/unselected
variables (e.g., selectedBackground/unselectedBackground and the
box-shadow/forced-color-adjust expectations) so the test passes regardless of
the story's default primary value while keeping the focus/outline checks
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d888e47a-00fa-4cfb-94cf-31dd115778d3
📒 Files selected for processing (2)
code/addons/docs/src/blocks/controls/Boolean.tsxcode/e2e-tests/addon-controls.spec.ts
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
|
Hi @Sidnioulz, following up on this PR. |
github-project-automation
Bot
moved this from In Progress
to Done
in Core Team Projects
4 participants
Closes #31695
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Boolean Controldocs orExample/Button -> Primary.forced-colors: activein browser devtools.TrueandFalseand confirm the highlighted state moves correctly.Documentation
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests