fix(action-card): make action card focusable and keyboard activatable#2598
fix(action-card): make action card focusable and keyboard activatable#2598lakshmi-priya-b wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThe Changesaction-card button wrapper for accessibility
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 docstrings
🧪 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 |
|
✅ 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 introduces accessibility and keyboard interaction improvements to the IxActionCard component, making it focusable and interactive via the Enter and Space keys when not passive. The review feedback highlights a potential issue where keyboard events from nested interactive elements could bubble up and trigger unintended clicks, suggesting a target check to prevent this. Additionally, in accordance with the repository style guide, the reviewer requests the addition of a changeset for these user-facing changes and an axe-based component test to verify accessibility coverage.
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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/action-card/action-card.scss`:
- Around line 44-50: The CSS selectors in the action-card.scss file are using
the class selector notation (`:host(.passive)`) but the passive state is applied
as an attribute rather than a class. Update the selectors `:host(.passive)` on
line 44 and `:host(:not(.passive))` on line 49 to use attribute selector
notation instead, changing them to `:host([passive])` and
`:host(:not([passive]))` respectively. This will ensure the passive styling and
interaction rules apply correctly based on the passive attribute.
In `@packages/core/src/components/action-card/test/action-card.ct.ts`:
- Around line 12-39: Add a mandatory `renders` test before the accessibility
describe block that verifies the ix-action-card component hydrates correctly
without any attributes. In both the 'default' and 'passive' accessibility tests,
add a hydration assertion after mounting the ix-action-card component but before
calling makeAxeBuilder().analyze(), ensuring the component is fully hydrated
before performing the accessibility scan. The hydration check should verify that
the mounted element contains the expected hydrated state before running axe
accessibility tests.
- Around line 13-38: The existing tests only validate accessibility violations
but do not test the keyboard interaction behavior that is the core change in
this PR. Add new regression tests after the existing 'default' and 'passive'
accessibility test cases to verify keyboard activation behavior. Create a
behavior test for the default mode that mounts an ix-action-card component,
simulates pressing Enter or Space keys, and asserts that the activation is
triggered (by checking for a click event or state change). Then create another
behavior test for passive mode that verifies pressing Enter or Space keys does
NOT trigger any activation. Follow the test ordering guideline by placing these
behavior tests (user interactions) after the accessibility violation tests.
🪄 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: 87102973-f05c-4787-949e-aafb64fb183b
📒 Files selected for processing (3)
packages/core/src/components/action-card/action-card.scsspackages/core/src/components/action-card/action-card.tsxpackages/core/src/components/action-card/test/action-card.ct.ts
| :host(.passive) & { | ||
| pointer-events: none; | ||
| cursor: default; | ||
| } | ||
|
|
||
| :host(:not(.passive)) &:active ix-card { | ||
| background-color: var(--theme-color-ghost--active); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Passive-state CSS selectors won’t match the component’s passive attribute.
Line 44 and Line 49 target :host(.passive), but the passive state is used as an attribute (passive), so these branches won’t apply reliably and passive styling/interaction gating can regress.
🔧 Proposed fix
- :host(.passive) & {
+ :host([passive]) & {
pointer-events: none;
cursor: default;
}
- :host(:not(.passive)) &:active ix-card {
+ :host(:not([passive])) &:active ix-card {
background-color: var(--theme-color-ghost--active);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| :host(.passive) & { | |
| pointer-events: none; | |
| cursor: default; | |
| } | |
| :host(:not(.passive)) &:active ix-card { | |
| background-color: var(--theme-color-ghost--active); | |
| :host([passive]) & { | |
| pointer-events: none; | |
| cursor: default; | |
| } | |
| :host(:not([passive])) &:active ix-card { | |
| background-color: var(--theme-color-ghost--active); | |
| } |
🤖 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/action-card/action-card.scss` around lines 44 -
50, The CSS selectors in the action-card.scss file are using the class selector
notation (`:host(.passive)`) but the passive state is applied as an attribute
rather than a class. Update the selectors `:host(.passive)` on line 44 and
`:host(:not(.passive))` on line 49 to use attribute selector notation instead,
changing them to `:host([passive])` and `:host(:not([passive]))` respectively.
This will ensure the passive styling and interaction rules apply correctly based
on the passive attribute.
| regressionTest.describe('accessibility', () => { | ||
| regressionTest('default', async ({ mount, makeAxeBuilder }) => { | ||
| await mount(` | ||
| <ix-action-card | ||
| heading="Scan for new devices" | ||
| subheading="Secondary text" | ||
| ></ix-action-card> | ||
| `); | ||
|
|
||
| const accessibilityScanResults = await makeAxeBuilder().analyze(); | ||
|
|
||
| expect(accessibilityScanResults.violations).toEqual([]); | ||
| }); | ||
|
|
||
| regressionTest('passive', async ({ mount, makeAxeBuilder }) => { | ||
| await mount(` | ||
| <ix-action-card | ||
| heading="Scan for new devices" | ||
| subheading="Secondary text" | ||
| passive | ||
| ></ix-action-card> | ||
| `); | ||
|
|
||
| const accessibilityScanResults = await makeAxeBuilder().analyze(); | ||
|
|
||
| expect(accessibilityScanResults.violations).toEqual([]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add the required renders test and hydration assertions.
This file currently runs axe scans only. It is missing the mandatory renders test, and hydration is not asserted before accessibility checks.
✅ Proposed test additions
regressionTest.describe('accessibility', () => {
regressionTest('default', async ({ mount, makeAxeBuilder, page }) => {
await mount(`
<ix-action-card
heading="Scan for new devices"
subheading="Secondary text"
></ix-action-card>
`);
+ await expect(page.locator('ix-action-card')).toHaveClass(/\bhydrated\b/);
+
const accessibilityScanResults = await makeAxeBuilder().analyze();
expect(accessibilityScanResults.violations).toEqual([]);
});
@@
await mount(`
<ix-action-card
heading="Scan for new devices"
subheading="Secondary text"
passive
></ix-action-card>
`);
+ await expect(page.locator('ix-action-card')).toHaveClass(/\bhydrated\b/);
+
const accessibilityScanResults = await makeAxeBuilder().analyze();
expect(accessibilityScanResults.violations).toEqual([]);
});
+
+ regressionTest('renders', async ({ mount, page }) => {
+ await mount(`
+ <ix-action-card
+ heading="Scan for new devices"
+ subheading="Secondary text"
+ ></ix-action-card>
+ `);
+
+ await expect(page.locator('ix-action-card')).toHaveClass(/\bhydrated\b/);
+ await expect(page.getByRole('button')).toBeVisible();
+ });
});As per coding guidelines, “Every test file should include a 'renders' test verifying basic hydration” and “Always verify components are hydrated before making further assertions … /\bhydrated\b/”; as per path instructions for packages/core/src/components/**/test/*.ct.ts, tests must include both accessibility and renders hydration coverage.
🤖 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/action-card/test/action-card.ct.ts` around lines
12 - 39, Add a mandatory `renders` test before the accessibility describe block
that verifies the ix-action-card component hydrates correctly without any
attributes. In both the 'default' and 'passive' accessibility tests, add a
hydration assertion after mounting the ix-action-card component but before
calling makeAxeBuilder().analyze(), ensuring the component is fully hydrated
before performing the accessibility scan. The hydration check should verify that
the mounted element contains the expected hydrated state before running axe
accessibility tests.
Sources: Coding guidelines, Path instructions
| regressionTest('default', async ({ mount, makeAxeBuilder }) => { | ||
| await mount(` | ||
| <ix-action-card | ||
| heading="Scan for new devices" | ||
| subheading="Secondary text" | ||
| ></ix-action-card> | ||
| `); | ||
|
|
||
| const accessibilityScanResults = await makeAxeBuilder().analyze(); | ||
|
|
||
| expect(accessibilityScanResults.violations).toEqual([]); | ||
| }); | ||
|
|
||
| regressionTest('passive', async ({ mount, makeAxeBuilder }) => { | ||
| await mount(` | ||
| <ix-action-card | ||
| heading="Scan for new devices" | ||
| subheading="Secondary text" | ||
| passive | ||
| ></ix-action-card> | ||
| `); | ||
|
|
||
| const accessibilityScanResults = await makeAxeBuilder().analyze(); | ||
|
|
||
| expect(accessibilityScanResults.violations).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add keyboard behavior regression tests for Enter/Space and passive mode.
The PR’s core change is keyboard activatability, but this file does not assert Enter/Space click activation or passive-mode non-activation, so the behavior can regress undetected.
⌨️ Proposed behavior tests
+regressionTest('keyboard activates on Enter and Space', async ({ mount, page }) => {
+ await mount(`
+ <ix-action-card heading="Scan for new devices" subheading="Secondary text"></ix-action-card>
+ `);
+
+ const host = page.locator('ix-action-card');
+ await expect(host).toHaveClass(/\bhydrated\b/);
+
+ const button = page.getByRole('button');
+ await button.focus();
+
+ const clickSpy = page.waitForEvent('console'); // replace with component click-event listener helper used in this repo
+ await page.keyboard.press('Enter');
+ await page.keyboard.press('Space');
+ await clickSpy;
+});
+
+regressionTest('passive is not keyboard-focusable', async ({ mount, page }) => {
+ await mount(`
+ <ix-action-card heading="Scan for new devices" subheading="Secondary text" passive></ix-action-card>
+ `);
+
+ await expect(page.locator('ix-action-card')).toHaveClass(/\bhydrated\b/);
+ await expect(page.getByRole('button')).toHaveAttribute('tabindex', '-1');
+});As per coding guidelines, “Follow this test order within each file … 4. Behavior tests (user interactions, state changes).”
🤖 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/action-card/test/action-card.ct.ts` around lines
13 - 38, The existing tests only validate accessibility violations but do not
test the keyboard interaction behavior that is the core change in this PR. Add
new regression tests after the existing 'default' and 'passive' accessibility
test cases to verify keyboard activation behavior. Create a behavior test for
the default mode that mounts an ix-action-card component, simulates pressing
Enter or Space keys, and asserts that the activation is triggered (by checking
for a click event or state change). Then create another behavior test for
passive mode that verifies pressing Enter or Space keys does NOT trigger any
activation. Follow the test ordering guideline by placing these behavior tests
(user interactions) after the accessibility violation tests.
Source: Coding guidelines
1 participant
💡 What is the current behavior?
Action cards are clickable using a mouse, but they are not keyboard accessible.
GitHub Issue Number: #
🆕 What is the new behavior?
Action cards now support keyboard interaction and expose interactive semantics.
🏁 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
New Features
Bug Fixes
Tests