fix(core/blind): add visible focus outline to blind header#2597
fix(core/blind): add visible focus outline to blind header#2597SaiYugandhar03 wants to merge 10 commits into
Conversation
🦋 Changeset detectedLatest commit: 6974499 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the layout and DOM structure of the blind component's header. Feedback highlights a critical accessibility issue where nesting the custom-header and header-actions slots inside the <button> element creates invalid nested interactive controls; moving these slots outside the button is recommended, along with adjusting the .blind-header width to auto. Additionally, the removal of --ix-icon-button-color causes a loss of contrast for icon buttons in colored variants, which should be restored. Finally, an axe-based component test should be added to verify accessibility coverage as required by the style guide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| <div class={'blind-header-content'}> | ||
| <ix-icon | ||
| class="collapse-icon" | ||
| name={iconChevronDownSmall} | ||
| color={ | ||
| this.variant === 'filled' || this.variant === 'outline' | ||
| ? 'color-std-text' | ||
| : `color-${this.variant}--contrast` | ||
| } | ||
| ref={(ref: HTMLElement | undefined) => (this.chevronRef = ref)} | ||
| ></ix-icon> | ||
| <div | ||
| class="blind-header-title" | ||
| id={`ix-blind-header-title-${this.blindId}`} | ||
| > | ||
| {this.label !== undefined ? ( | ||
| <Fragment> | ||
| {this.icon && ( | ||
| <ix-icon | ||
| class="blind-header-title-icon" | ||
| name={this.icon} | ||
| color={ | ||
| this.variant === 'filled' || | ||
| this.variant === 'outline' | ||
| ? 'color-std-text' | ||
| : `color-${this.variant}--contrast` | ||
| } | ||
| ></ix-icon> | ||
| )} | ||
| <div class={'blind-header-title-row'}> | ||
| <div class="blind-header-title-col"> | ||
| <ix-typography | ||
| title={this.label} | ||
| format="label-lg" | ||
| bold | ||
| > | ||
| {this.label} | ||
| </div> | ||
| </ix-typography> | ||
| </div> | ||
|
|
||
| {this.sublabel && ( | ||
| <div class="blind-header-title-col"> | ||
| <ix-typography title={this.sublabel}> | ||
| <div class="blind-header-title-sublabel"> | ||
| {this.sublabel} | ||
| <div | ||
| class="blind-header-title-label" | ||
| title={this.label} | ||
| > | ||
| {this.label} | ||
| </div> | ||
| </ix-typography> | ||
| </div> | ||
| )} | ||
| </div> | ||
| <div class="header-actions"> | ||
| <slot name="header-actions"></slot> | ||
| </div> | ||
| </Fragment> | ||
| ) : null} | ||
|
|
||
| {this.sublabel && ( | ||
| <div class="blind-header-title-col"> | ||
| <ix-typography title={this.sublabel}> | ||
| <div class="blind-header-title-sublabel"> | ||
| {this.sublabel} | ||
| </div> | ||
| </ix-typography> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </Fragment> | ||
| ) : null} | ||
| </div> | ||
| <div class="blind-header-custom"> | ||
| <slot name="custom-header"></slot> | ||
| </div> | ||
| <div class="header-actions"> | ||
| <slot name="header-actions"></slot> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
Nested Interactive Controls (Accessibility Issue)
Wrapping the entire header content—including the custom-header and header-actions slots—inside the <button class="blind-header"> element creates nested interactive controls.
According to the HTML5 specification and WCAG 4.1.2, a <button> element must not contain any interactive descendants (such as other buttons, links, or form controls). Since consumers can slot arbitrary interactive elements (like <ix-icon-button> or custom links) into header-actions or custom-header, this nesting will break keyboard navigation and screen reader behavior.
Solution
Move the custom-header and header-actions slots outside of the <button> element, placing them as siblings to the button inside the .blind-header-wrapper flex container. This preserves the layout while ensuring semantic correctness and full accessibility.
<div class="blind-header-content">
<ix-icon
class="collapse-icon"
name={iconChevronDownSmall}
color={
this.variant === 'filled' || this.variant === 'outline'
? 'color-std-text'
: `color-${this.variant}--contrast`
}
ref={(ref: HTMLElement | undefined) => (this.chevronRef = ref)}
></ix-icon>
<div
class="blind-header-title"
id={`ix-blind-header-title-${this.blindId}`}
>
{this.label !== undefined ? (
<Fragment>
{this.icon && (
<ix-icon
class="blind-header-title-icon"
name={this.icon}
color={
this.variant === 'filled' ||
this.variant === 'outline'
? 'color-std-text'
: `color-${this.variant}--contrast`
}
></ix-icon>
)}
<div class={'blind-header-title-row'}>
<div class="blind-header-title-col">
<ix-typography
title={this.label}
format="label-lg"
bold
>
<div
class="blind-header-title-label"
title={this.label}
>
{this.label}
</div>
</ix-typography>
</div>
{this.sublabel && (
<div class="blind-header-title-col">
<ix-typography title={this.sublabel}>
<div class="blind-header-title-sublabel">
{this.sublabel}
</div>
</ix-typography>
</div>
)}
</div>
</Fragment>
) : null}
</div>
</div>
</button>
<div class="blind-header-custom">
<slot name="custom-header"></slot>
</div>
<div class="header-actions">
<slot name="header-actions"></slot>
</div>
| min-height: 3rem; | ||
| height: 3rem; | ||
| width: calc(100% - 2 * var(--theme-blind--border-thickness)); | ||
| width: 100%; |
There was a problem hiding this comment.
To support moving the custom header and action slots outside of the <button> element (to avoid nested interactive controls), change the width of .blind-header from 100% to auto. This allows the flexbox layout of .blind-header-wrapper to distribute space correctly.
width: auto;| } @else { | ||
| .blind-header-title-label, | ||
| .blind-header-title-sublabel { | ||
| color: var(--theme-color-#{$variant}--contrast); | ||
| } | ||
|
|
||
| .header-actions { | ||
| --ix-icon-button-color: var(--theme-color-#{$variant}--contrast); | ||
| } | ||
| } |
There was a problem hiding this comment.
Loss of Icon Button Contrast in Colored Variants
Removing the .header-actions block that sets --ix-icon-button-color will cause icon buttons inside the header actions slot to lose their correct contrast color when using colored variants (such as alarm, critical, warning, etc.).
For example, on an alarm (red) background, the icon button should use the contrast color (--theme-color-alarm--contrast) to remain visible. Without this, it will default to a dark color, violating WCAG contrast ratio guidelines.
Please restore these custom property definitions so they apply to .header-actions under the respective variants.
} @else {
.blind-header-title-label,
.blind-header-title-sublabel {
color: var(--theme-color-#{$variant}--contrast);
}
.header-actions {
--ix-icon-button-color: var(--theme-color-#{$variant}--contrast);
}
}| type="button" | ||
| aria-labelledby={`ix-blind-header-title-${this.blindId}`} | ||
| aria-controls={`ix-blind-content-section-${this.blindId}`} | ||
| aria-expanded={a11yBoolean(!this.collapsed)} |
There was a problem hiding this comment.
According to the Repository Style Guide (Accessibility section), component changes—especially those improving accessibility and focus handling—must have accessibility coverage verified with an axe-based component test.
Please add an axe-based accessibility test in packages/core/src/components/blind/test/blind.ct.ts following the pattern used in other components (e.g., using @axe-core/playwright or similar helper).
References
- For component changes, check that accessibility coverage exists with an axe-based component test. If an accessibility-relevant change lacks appropriate automated coverage, raise it as a review finding. (link)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdates ChangesBlind accessibility improvements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/components/blind/test/blind.ct.ts (1)
22-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a word-boundary regex for hydration class matching.
Line 22 should use
\bboundaries to avoid partial class-name matches.Suggested fix
- await expect(blindElement).toHaveClass(/hydrated/); + await expect(blindElement).toHaveClass(/\bhydrated\b/);As per coding guidelines, "Use word boundary
\bin regex patterns to avoid false matches when checking classes likehydrated".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/blind/test/blind.ct.ts` at line 22, The regex pattern in the expect statement for the blindElement class check is missing word boundaries, which could cause false matches with class names like "not-hydrated" or "pre-hydrated". Update the regex pattern from /hydrated/ to /\bhydrated\b/ to add word-boundary anchors that ensure only the exact word "hydrated" is matched and not as a partial substring within other class names.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/blind/blind.scss`:
- Line 55: In the blind.scss file, the else branch at line 55 uses `}else {`
which is missing the required `@` symbol for SCSS control structures. Change
`}else {` to `}`@else` {` to properly mark it as an SCSS conditional statement
rather than a CSS selector. This will ensure the else block is correctly parsed
as part of the conditional logic for the non-filled and non-outline variants.
In `@packages/core/src/components/blind/test/blind.ct.ts`:
- Around line 12-17: The regressionTest function is running the Axe scan via
analyze() immediately after mounting the ix-blind component, without verifying
that the component has been hydrated first. Add a hydration assertion between
the mount call and the analyze call using the pattern await
expect(page.locator('ix-blind')).toHaveClass(/\bhydrated\b/) to ensure the
component is fully hydrated before running accessibility checks.
---
Outside diff comments:
In `@packages/core/src/components/blind/test/blind.ct.ts`:
- Line 22: The regex pattern in the expect statement for the blindElement class
check is missing word boundaries, which could cause false matches with class
names like "not-hydrated" or "pre-hydrated". Update the regex pattern from
/hydrated/ to /\bhydrated\b/ to add word-boundary anchors that ensure only the
exact word "hydrated" is matched and not as a partial substring within other
class names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c050eca-ee95-4032-a076-89c726d4ad67
📒 Files selected for processing (2)
packages/core/src/components/blind/blind.scsspackages/core/src/components/blind/test/blind.ct.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/components/blind/test/blind.ct.ts (1)
22-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse word-boundary regex for hydration class checks.
Use
\baroundhydratedto avoid partial class-name matches.As per coding guidelines, "Use word boundary
\bin regex patterns to avoid false matches when checking classes likehydrated".Suggested fix
- await expect(blindElement).toHaveClass(/hydrated/); + await expect(blindElement).toHaveClass(/\bhydrated\b/);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/blind/test/blind.ct.ts` at line 22, The regex pattern /hydrated/ in the toHaveClass expectation can match partial class names like "not-hydrated" or "hydrated-component", causing false positives. Update the pattern to /\bhydrated\b/ by adding word-boundary anchors around the word "hydrated" to ensure only exact class-name matches are detected, following the coding guidelines for regex pattern validation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/blind/blind.scss`:
- Line 55: The closing brace and `@else` statement on line 55 in the blind.scss
file are not properly spaced, causing a Stylelint violation. Add a space between
the closing brace and the `@else` keyword in the `}`@else` {` syntax to make it `}
`@else` {` and satisfy the configured SCSS lint rule.
---
Outside diff comments:
In `@packages/core/src/components/blind/test/blind.ct.ts`:
- Line 22: The regex pattern /hydrated/ in the toHaveClass expectation can match
partial class names like "not-hydrated" or "hydrated-component", causing false
positives. Update the pattern to /\bhydrated\b/ by adding word-boundary anchors
around the word "hydrated" to ensure only exact class-name matches are detected,
following the coding guidelines for regex pattern validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fb43c989-520c-4930-bab6-8c4769032659
📒 Files selected for processing (2)
packages/core/src/components/blind/blind.scsspackages/core/src/components/blind/test/blind.ct.ts
danielleroux
left a comment
danielleroux
left a comment
There was a problem hiding this comment.
Following points are open:
- Changeset is missing
- Adapt pr title/description because there was no improvement to screenreader, there is only a update of the missing outline effect
SaiYugandhar03
changed the title
|
2 participants
💡 What is the current behavior?
The blind header could be toggled but did not show a clear keyboard focus state around the header wrapper
Jira ticket: 4338
🆕 What is the new behavior?
The blind header now displays a visible focus outline around the wrapper
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support
Summary by CodeRabbit
Summary
ix-blindheader label/sublabel and action icon colors across variants for more consistent contrast and theme alignment.aria-hidden="true").ix-blindto ensure no violations are introduced.blindaccessibility improvements.