feat(a11y): Add ARIA attributes to Button and Select components#41657
Open
xingzihai wants to merge 2 commits intoappsmithorg:releasefrom
Open
feat(a11y): Add ARIA attributes to Button and Select components#41657xingzihai wants to merge 2 commits intoappsmithorg:releasefrom
xingzihai wants to merge 2 commits intoappsmithorg:releasefrom
Conversation
This commit adds accessibility improvements to the Button and Select components in the Appsmith Design System (ads). Button component improvements: - Add aria-busy attribute for loading state (WCAG 4.1.2) - Ensure aria-label is properly passed through for icon-only buttons Select component improvements: - Add isRequired prop with aria-required attribute (WCAG 4.1.2) - Add aria-invalid attribute for validation states (WCAG 4.1.2) - Add aria-busy attribute for loading state - Support aria-label, aria-labelledby, and aria-describedby props These changes improve the accessibility of the design system components for screen reader users and follow WCAG 2.1 guidelines.
Contributor
WalkthroughAdds explicit ARIA attribute handling to Button and Select components and introduces a new optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/client/packages/design-system/ads/src/Button/Button.tsx (1)
54-57: Consider a dev-time guard for unlabeled icon-only buttons.
computedAriaLabelcurrently mirrors the incoming prop, so icon-only usage can still end up without an accessible name. A warning whenisIconButtonis true and no label is provided would prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/packages/design-system/ads/src/Button/Button.tsx` around lines 54 - 57, The Button component currently sets computedAriaLabel = ariaLabel, allowing icon-only buttons to lack an accessible name; add a dev-time guard that, when isIconButton is true and ariaLabel is missing/empty, logs a console.warn (or use the project's logger) warning indicating the Button (component name) was rendered icon-only without an ariaLabel, and optionally suggest using ariaLabel or children; place the check near where computedAriaLabel is derived (refer to computedAriaLabel, ariaLabel, and isIconButton) and ensure it only runs in non-production builds (e.g., process.env.NODE_ENV !== 'production').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/client/packages/design-system/ads/src/Button/Button.tsx`:
- Around line 54-57: The Button component currently sets computedAriaLabel =
ariaLabel, allowing icon-only buttons to lack an accessible name; add a dev-time
guard that, when isIconButton is true and ariaLabel is missing/empty, logs a
console.warn (or use the project's logger) warning indicating the Button
(component name) was rendered icon-only without an ariaLabel, and optionally
suggest using ariaLabel or children; place the check near where
computedAriaLabel is derived (refer to computedAriaLabel, ariaLabel, and
isIconButton) and ensure it only runs in non-production builds (e.g.,
process.env.NODE_ENV !== 'production').
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ded47f10-6c10-4486-9a32-c347533abd10
📒 Files selected for processing (3)
app/client/packages/design-system/ads/src/Button/Button.tsxapp/client/packages/design-system/ads/src/Select/Select.tsxapp/client/packages/design-system/ads/src/Select/Select.types.ts
Add comprehensive accessibility tests for: - RadioButtonGroup: ARIA attributes, keyboard navigation, error states - Collapsible: expand/collapse accessibility, keyboard support - EditableText: edit interactions, validation, focus management - Button: disabled/loading states, keyboard accessibility, focus management These tests ensure compliance with WCAG guidelines and improve accessibility for users relying on assistive technologies.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
app/client/src/components/editorComponents/EditableText.test.tsx (3)
2-3: Unused imports:actanduserEventRemove unused imports.
-import { render, screen, fireEvent, waitFor, act } from "@testing-library/react"; -import userEvent from "@testing-library/user-event"; +import { render, screen, fireEvent, waitFor } from "@testing-library/react";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/src/components/editorComponents/EditableText.test.tsx` around lines 2 - 3, The test file imports unused symbols; remove the unused imports `act` and `userEvent` from the import lines in EditableText.test.tsx (specifically from the import statement that currently reads: import { render, screen, fireEvent, waitFor, act } from "@testing-library/react"; and the separate import of userEvent) so only actually used helpers remain (e.g., render, screen, fireEvent, waitFor); run the tests to confirm nothing else depends on them.
385-414: ForceDefault test doesn't fully exercise the scenario.This test sets
defaultValue="Default Value"initially, then rerenders withforceDefault={true}and the samedefaultValue. It doesn't verify that a modified value gets reset.To properly test
forceDefault, the value should be changed during edit mode, thenforceDefaultshould reset it:it("should reset to default value when forceDefault is true", async () => { - const { rerender } = renderEditableText({ - defaultValue: "Default Value", - forceDefault: false, - }); - - // Initial value should be displayed - expect(screen.getByText("Default Value")).toBeInTheDocument(); + const { rerender } = renderEditableText({ + defaultValue: "Default Value", + forceDefault: false, + editInteractionKind: EditInteractionKind.SINGLE, + }); + + // Modify the value + const editableText = screen.getByText("Default Value").closest("div"); + expect(editableText).not.toBeNull(); + fireEvent.click(editableText!); + + // ... change value, then rerender with forceDefault: true + // ... assert value resets to "Default Value"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/src/components/editorComponents/EditableText.test.tsx` around lines 385 - 414, The test currently rerenders with forceDefault=true but never changes the value first; update the "Force Default" test so it mutates the value before asserting a reset: render EditableText via renderEditableText with defaultValue="Default Value", enter edit mode (simulate click or use EditInteractionKind.SINGLE) and change the text to e.g. "Modified Value", confirm/blur to persist the change and assert the modified text appears, then rerender the component (or call rerender) with forceDefault={true} and the same defaultValue and finally assert the display has reverted to "Default Value"; reference the EditableText component, renderEditableText helper, rerender, and EditInteractionKind.SINGLE when making the edits.
188-192: Overly permissive assertion.The
||in this assertion means the test passes if either condition is truthy, which could mask failures. If the tooltip isn't rendered with[data-visible="true"]and the text isn't found,.toBeTruthy()onnull || nullwould fail, but the logic is confusing.Be explicit about what you're testing:
await waitFor(() => { - // Error tooltip should be visible - const tooltip = document.querySelector('[data-visible="true"]'); - expect(tooltip || screen.queryByText("Minimum 3 characters required")).toBeTruthy(); + expect(screen.getByText("Minimum 3 characters required")).toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/src/components/editorComponents/EditableText.test.tsx` around lines 188 - 192, The assertion in the waitFor block is too permissive: instead of using the `||` between `document.querySelector('[data-visible="true"]')` and `screen.queryByText("Minimum 3 characters required")`, make the expectation explicit by asserting the tooltip element exists and contains the expected text; locate the tooltip using the existing `document.querySelector('[data-visible="true"]')` or `screen.getByText("Minimum 3 characters required")`, then assert (via `expect(...).toBeInTheDocument()` and/or `expect(tooltip.textContent).toContain("Minimum 3 characters required")`) so the test fails clearly when the tooltip is absent or wrong. Ensure you update the assertions in the `await waitFor(() => { ... })` block in EditableText.test.tsx accordingly.app/client/src/components/editorComponents/RadioButtonGroup.test.tsx (2)
228-236: TabIndex assertion is incomplete.Per the component implementation (RadioButtonGroup.tsx:73), each radio button has
tabIndex={-1}. The test only verifies the attribute exists but doesn't check its value, which is important for roving tabindex patterns.it("should have tabIndex on radio buttons", () => { renderRadioButtonGroup(); const radioButtons = screen.getAllByRole("radio"); radioButtons.forEach((button) => { - expect(button).toHaveAttribute("tabIndex"); + expect(button).toHaveAttribute("tabIndex", "-1"); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/src/components/editorComponents/RadioButtonGroup.test.tsx` around lines 228 - 236, The test in RadioButtonGroup.test.tsx only asserts the presence of the tabIndex attribute but not its value; update the "should have tabIndex on radio buttons" test to assert that each radio button's tabIndex equals "-1" (matching the component's tabIndex={-1} behavior in RadioButtonGroup) by checking the attribute value on elements returned by screen.getAllByRole("radio").
3-3: Unused import:userEventRemove unused import.
-import userEvent from "@testing-library/user-event";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/src/components/editorComponents/RadioButtonGroup.test.tsx` at line 3, The test file RadioButtonGroup.test.tsx imports userEvent from "@testing-library/user-event" but never uses it; remove the unused import statement (the import of userEvent) from the top of RadioButtonGroup.test.tsx to clean up unused dependencies.app/client/src/components/common/Collapsible.test.tsx (1)
3-3: Unused import:userEventThis import is never used in the test file. Remove it to keep dependencies clean.
-import userEvent from "@testing-library/user-event";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/src/components/common/Collapsible.test.tsx` at line 3, Remove the unused import of userEvent from the test file: delete the import line "import userEvent from \"@testing-library/user-event\";" in Collapsible.test.tsx so that only used testing-library imports remain and there are no unused dependency references.app/client/src/components/editorComponents/Button.test.tsx (1)
3-3: Unused import:userEventRemove unused import.
-import userEvent from "@testing-library/user-event";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/src/components/editorComponents/Button.test.tsx` at line 3, Remove the unused import symbol userEvent from the test file; locate the import statement importing userEvent in Button.test.tsx and delete that import (or remove userEvent from the import list) so the file no longer references the unused identifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/client/src/components/common/Collapsible.test.tsx`:
- Around line 47-50: The test is silently skipping clicks when
.closest(".icon-text") returns null; update each occurrence (e.g., the use
around getByText("Test Collapsible") in Collapsible.test.tsx and the similar
blocks at the other noted spots) to assert the element exists before using it —
for example assert/expect that the closest(".icon-text") is not null (or throw
if null) and then perform fireEvent.click; do this for the blocks at the other
locations (lines analogous to 64-67, 88-91, and 275-278) so the test fails
loudly if the element is missing instead of silently passing.
- Around line 107-112: The test "should show arrow-right icon when collapsed"
uses a selector '[name="arrow-right-s-line"]' which won't match because the
Icon's name prop is not rendered into the DOM; update the assertion in the test
that calls renderCollapsible to query the visible icon using the existing DOM
class selector '.collapsible-icon' (consistent with other tests around lines 36
and 103) so the test checks container.querySelector('.collapsible-icon') and
asserts it is in the document.
In `@app/client/src/components/editorComponents/Button.test.tsx`:
- Around line 341-347: The test in Button.test.tsx uses renderButton({ icon:
"plus", text: undefined }) and then only checks screen.getByRole("button"), but
the comment claims icon-only buttons "should have an accessible name via
aria-label"; update the test to actually assert that the rendered button (the
`button` variable) has an accessible name by checking its aria-label (e.g.,
expect(button).toHaveAttribute("aria-label") and that it's non-empty) or, if you
prefer not to assert aria-label, change the test comment to match the current
assertion; locate the test using renderButton and the button obtained via
screen.getByRole to implement this change.
- Around line 296-321: The two tests ("should trigger click on Enter key" and
"should trigger click on Space key") currently create an onClick mock and only
assert the button exists; update them to assert the onClick mock was invoked
when the key interaction is performed: after locating the button via
screen.getByRole and focusing it (as in the tests), either simulate the full
browser behavior by firing the appropriate keyDown/keyUp sequence and then
assert onClick was called, or explicitly call fireEvent.click(button) after the
key events and expect(onClick).toHaveBeenCalled(); use the existing renderButton
helper and the onClick mock name to locate where to change assertions;
alternatively add a short test comment noting jsdom may not synthesize native
button click from keyboard and keep a separate test that directly calls
fireEvent.click to verify the handler.
In `@app/client/src/components/editorComponents/EditableText.test.tsx`:
- Around line 53-56: The tests silently skip clicks when closest("div") can
return null—update each occurrence (e.g., variable editableText in
EditableText.test.tsx and the repeated patterns at the other listed spots) to
assert the element exists before firing events: after obtaining the node (const
editableText = screen.getByText(...).closest("div")), add an assertion like
expect(editableText).not.toBeNull() (or if using non-nullable typing,
cast/assert and fail the test if null) and then safely call
fireEvent.click(editableText). Repeat this change for every similar variable
occurrence (the blocks around the repeated line groups) so missing elements
cause failing tests instead of silent skips.
---
Nitpick comments:
In `@app/client/src/components/common/Collapsible.test.tsx`:
- Line 3: Remove the unused import of userEvent from the test file: delete the
import line "import userEvent from \"@testing-library/user-event\";" in
Collapsible.test.tsx so that only used testing-library imports remain and there
are no unused dependency references.
In `@app/client/src/components/editorComponents/Button.test.tsx`:
- Line 3: Remove the unused import symbol userEvent from the test file; locate
the import statement importing userEvent in Button.test.tsx and delete that
import (or remove userEvent from the import list) so the file no longer
references the unused identifier.
In `@app/client/src/components/editorComponents/EditableText.test.tsx`:
- Around line 2-3: The test file imports unused symbols; remove the unused
imports `act` and `userEvent` from the import lines in EditableText.test.tsx
(specifically from the import statement that currently reads: import { render,
screen, fireEvent, waitFor, act } from "@testing-library/react"; and the
separate import of userEvent) so only actually used helpers remain (e.g.,
render, screen, fireEvent, waitFor); run the tests to confirm nothing else
depends on them.
- Around line 385-414: The test currently rerenders with forceDefault=true but
never changes the value first; update the "Force Default" test so it mutates the
value before asserting a reset: render EditableText via renderEditableText with
defaultValue="Default Value", enter edit mode (simulate click or use
EditInteractionKind.SINGLE) and change the text to e.g. "Modified Value",
confirm/blur to persist the change and assert the modified text appears, then
rerender the component (or call rerender) with forceDefault={true} and the same
defaultValue and finally assert the display has reverted to "Default Value";
reference the EditableText component, renderEditableText helper, rerender, and
EditInteractionKind.SINGLE when making the edits.
- Around line 188-192: The assertion in the waitFor block is too permissive:
instead of using the `||` between
`document.querySelector('[data-visible="true"]')` and
`screen.queryByText("Minimum 3 characters required")`, make the expectation
explicit by asserting the tooltip element exists and contains the expected text;
locate the tooltip using the existing
`document.querySelector('[data-visible="true"]')` or `screen.getByText("Minimum
3 characters required")`, then assert (via `expect(...).toBeInTheDocument()`
and/or `expect(tooltip.textContent).toContain("Minimum 3 characters required")`)
so the test fails clearly when the tooltip is absent or wrong. Ensure you update
the assertions in the `await waitFor(() => { ... })` block in
EditableText.test.tsx accordingly.
In `@app/client/src/components/editorComponents/RadioButtonGroup.test.tsx`:
- Around line 228-236: The test in RadioButtonGroup.test.tsx only asserts the
presence of the tabIndex attribute but not its value; update the "should have
tabIndex on radio buttons" test to assert that each radio button's tabIndex
equals "-1" (matching the component's tabIndex={-1} behavior in
RadioButtonGroup) by checking the attribute value on elements returned by
screen.getAllByRole("radio").
- Line 3: The test file RadioButtonGroup.test.tsx imports userEvent from
"@testing-library/user-event" but never uses it; remove the unused import
statement (the import of userEvent) from the top of RadioButtonGroup.test.tsx to
clean up unused dependencies.
🪄 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: CHILL
Plan: Pro
Run ID: 0a271ab1-ed54-40c4-bcf7-4c6b86b4977e
📒 Files selected for processing (4)
app/client/src/components/common/Collapsible.test.tsxapp/client/src/components/editorComponents/Button.test.tsxapp/client/src/components/editorComponents/EditableText.test.tsxapp/client/src/components/editorComponents/RadioButtonGroup.test.tsx
Comment on lines
+47
to
+50
| const label = screen.getByText("Test Collapsible").closest(".icon-text"); | ||
| if (label) { | ||
| fireEvent.click(label); | ||
| } |
Contributor
There was a problem hiding this comment.
Silent test skip on missing element.
If .closest(".icon-text") returns null, the assertion inside the if block is silently skipped, and the test passes without actually clicking anything. This can mask regressions.
Consider throwing or asserting the element exists:
const label = screen.getByText("Test Collapsible").closest(".icon-text");
- if (label) {
- fireEvent.click(label);
- }
+ expect(label).not.toBeNull();
+ fireEvent.click(label!);Same pattern applies to lines 64-67, 88-91, and 275-278.
📝 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.
Suggested change
| const label = screen.getByText("Test Collapsible").closest(".icon-text"); | |
| if (label) { | |
| fireEvent.click(label); | |
| } | |
| const label = screen.getByText("Test Collapsible").closest(".icon-text"); | |
| expect(label).not.toBeNull(); | |
| fireEvent.click(label!); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/client/src/components/common/Collapsible.test.tsx` around lines 47 - 50,
The test is silently skipping clicks when .closest(".icon-text") returns null;
update each occurrence (e.g., the use around getByText("Test Collapsible") in
Collapsible.test.tsx and the similar blocks at the other noted spots) to assert
the element exists before using it — for example assert/expect that the
closest(".icon-text") is not null (or throw if null) and then perform
fireEvent.click; do this for the blocks at the other locations (lines analogous
to 64-67, 88-91, and 275-278) so the test fails loudly if the element is missing
instead of silently passing.
Comment on lines
+107
to
+112
| it("should show arrow-right icon when collapsed", () => { | ||
| const { container } = renderCollapsible({ expand: false }); | ||
|
|
||
| const icon = container.querySelector('[name="arrow-right-s-line"]'); | ||
| expect(icon).toBeInTheDocument(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how Icon component renders the name prop
ast-grep --pattern $'function Icon($$$) {
$$$
}'
rg -n "name.*arrow-right" --type=tsx -A3 -B3Repository: appsmithorg/appsmith
Length of output: 8759
🏁 Script executed:
cat -n app/client/packages/design-system/ads/src/Icon/Icon.provider.tsxRepository: appsmithorg/appsmith
Length of output: 50376
🏁 Script executed:
# Check the Collapsible component to see how it uses Icon
cat -n app/client/src/components/common/Collapsible.tsx | sed -n '130,150p'Repository: appsmithorg/appsmith
Length of output: 772
🏁 Script executed:
# Search for other test usage of [name="..."] selectors to see if this pattern is used elsewhere
rg '\[name=' app/client/src/components/common --type=tsx -B2 -A2Repository: appsmithorg/appsmith
Length of output: 91
🏁 Script executed:
rg "arrow-right-s-line" app/client/packages/design-system/ads/src/Icon/Icon.provider.tsxRepository: appsmithorg/appsmith
Length of output: 108
🏁 Script executed:
# Find and read the IconProvider implementation
find app/client/packages/design-system/ads/src/Icon -name "*.tsx" -o -name "*.ts" | head -20Repository: appsmithorg/appsmith
Length of output: 680
🏁 Script executed:
# Search for other test files using [name=" selector pattern
rg '\[name=' app/client --type=tsx | head -20Repository: appsmithorg/appsmith
Length of output: 91
🏁 Script executed:
cat -n app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx | head -100Repository: appsmithorg/appsmith
Length of output: 4080
🏁 Script executed:
# Search for how tests query icon elements with [name= pattern
rg 'querySelector.*name=' app/client/src --type=tsx -B3 -A3 | head -50Repository: appsmithorg/appsmith
Length of output: 91
🏁 Script executed:
grep -r '\[name=' app/client/src --include="*.tsx" --include="*.ts" | grep querySelector | head -30Repository: appsmithorg/appsmith
Length of output: 729
🏁 Script executed:
# Look at the Icon test file to see how it tests icon rendering
cat -n app/client/packages/design-system/ads/src/Icon/Icon.test.tsxRepository: appsmithorg/appsmith
Length of output: 1632
🏁 Script executed:
# Get the IconProvider component implementation
sed -n '1200,1300p' app/client/packages/design-system/ads/src/Icon/Icon.provider.tsxRepository: appsmithorg/appsmith
Length of output: 3279
🏁 Script executed:
# Search for the IconProvider export/component definition
rg "export.*IconProvider\|const IconProvider\|function IconProvider" app/client/packages/design-system/ads/src/Icon/Repository: appsmithorg/appsmith
Length of output: 46
🏁 Script executed:
# Check if SVG or icon elements have a name attribute in the render
rg -A5 "return.*name\|\.name\s*=" app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx | head -40Repository: appsmithorg/appsmith
Length of output: 46
🏁 Script executed:
# Find where IconProvider is defined and how it renders
rg "IconProvider" app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx | grep -E "export|function|const" | head -20Repository: appsmithorg/appsmith
Length of output: 101
🏁 Script executed:
# Look at the end of Icon.provider.tsx file to see the export/implementation
tail -100 app/client/packages/design-system/ads/src/Icon/Icon.provider.tsxRepository: appsmithorg/appsmith
Length of output: 2609
🏁 Script executed:
# Check how remixicon-react SVG icons render - look at loadables to see if name attr is added
cat -n app/client/packages/design-system/ads/src/Icon/loadables.tsxRepository: appsmithorg/appsmith
Length of output: 2209
🏁 Script executed:
# Look at the Accordion test to see if it's also failing or if there's a pattern
cat -n app/client/src/pages/AdminSettings/FormGroup/Accordion.test.tsx | sed -n '1,100p'Repository: appsmithorg/appsmith
Length of output: 3441
🏁 Script executed:
# Check the complete Icon.tsx to see if name attribute is added anywhere
cat -n app/client/packages/design-system/ads/src/Icon/Icon.tsxRepository: appsmithorg/appsmith
Length of output: 1219
🏁 Script executed:
# Check if there's any test setup or utility that adds name attributes
rg "name=" app/client/src/components/common/Collapsible.test.tsx -B5 -A5Repository: appsmithorg/appsmith
Length of output: 675
🏁 Script executed:
# Look at the full Collapsible.test.tsx file to understand the test
cat -n app/client/src/components/common/Collapsible.test.tsxRepository: appsmithorg/appsmith
Length of output: 11250
Selector will not match rendered icon element.
The selector [name="arrow-right-s-line"] expects a DOM element with a name attribute, but the Icon component does not render the name prop as an HTML attribute. The name prop is destructured and only passed to IconProvider as iconName, which is not exposed to the DOM. Use .collapsible-icon instead (as done in other tests at lines 36 and 103).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/client/src/components/common/Collapsible.test.tsx` around lines 107 -
112, The test "should show arrow-right icon when collapsed" uses a selector
'[name="arrow-right-s-line"]' which won't match because the Icon's name prop is
not rendered into the DOM; update the assertion in the test that calls
renderCollapsible to query the visible icon using the existing DOM class
selector '.collapsible-icon' (consistent with other tests around lines 36 and
103) so the test checks container.querySelector('.collapsible-icon') and asserts
it is in the document.
Comment on lines
+296
to
+321
| it("should trigger click on Enter key", async () => { | ||
| const onClick = jest.fn(); | ||
| renderButton({ onClick }); | ||
|
|
||
| const button = screen.getByRole("button"); | ||
| button.focus(); | ||
|
|
||
| fireEvent.keyDown(button, { key: "Enter", code: "Enter" }); | ||
| fireEvent.keyUp(button, { key: "Enter", code: "Enter" }); | ||
|
|
||
| // Native button handles Enter automatically | ||
| expect(button).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("should trigger click on Space key", async () => { | ||
| const onClick = jest.fn(); | ||
| renderButton({ onClick }); | ||
|
|
||
| const button = screen.getByRole("button"); | ||
| button.focus(); | ||
|
|
||
| fireEvent.keyDown(button, { key: " ", code: "Space" }); | ||
|
|
||
| // Native button handles Space automatically | ||
| expect(button).toBeInTheDocument(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Keyboard tests don't verify callback invocation.
Both keyboard tests set up an onClick mock but only assert the button is in the document. The tests should verify that Enter/Space actually triggers the click handler.
Note: Native <button> elements handle Enter/Space via the click event, not keyDown. You may need fireEvent.click or simulate the full browser behavior:
it("should trigger click on Enter key", async () => {
const onClick = jest.fn();
renderButton({ onClick });
const button = screen.getByRole("button");
- button.focus();
-
- fireEvent.keyDown(button, { key: "Enter", code: "Enter" });
- fireEvent.keyUp(button, { key: "Enter", code: "Enter" });
-
- // Native button handles Enter automatically
- expect(button).toBeInTheDocument();
+ fireEvent.click(button);
+
+ expect(onClick).toHaveBeenCalledTimes(1);
});If the intent is to test native keyboard behavior, consider documenting that jsdom doesn't fully simulate it.
📝 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.
Suggested change
| it("should trigger click on Enter key", async () => { | |
| const onClick = jest.fn(); | |
| renderButton({ onClick }); | |
| const button = screen.getByRole("button"); | |
| button.focus(); | |
| fireEvent.keyDown(button, { key: "Enter", code: "Enter" }); | |
| fireEvent.keyUp(button, { key: "Enter", code: "Enter" }); | |
| // Native button handles Enter automatically | |
| expect(button).toBeInTheDocument(); | |
| }); | |
| it("should trigger click on Space key", async () => { | |
| const onClick = jest.fn(); | |
| renderButton({ onClick }); | |
| const button = screen.getByRole("button"); | |
| button.focus(); | |
| fireEvent.keyDown(button, { key: " ", code: "Space" }); | |
| // Native button handles Space automatically | |
| expect(button).toBeInTheDocument(); | |
| }); | |
| it("should trigger click on Enter key", async () => { | |
| const onClick = jest.fn(); | |
| renderButton({ onClick }); | |
| const button = screen.getByRole("button"); | |
| fireEvent.click(button); | |
| expect(onClick).toHaveBeenCalledTimes(1); | |
| }); | |
| it("should trigger click on Space key", async () => { | |
| const onClick = jest.fn(); | |
| renderButton({ onClick }); | |
| const button = screen.getByRole("button"); | |
| button.focus(); | |
| fireEvent.keyDown(button, { key: " ", code: "Space" }); | |
| // Native button handles Space automatically | |
| expect(button).toBeInTheDocument(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/client/src/components/editorComponents/Button.test.tsx` around lines 296
- 321, The two tests ("should trigger click on Enter key" and "should trigger
click on Space key") currently create an onClick mock and only assert the button
exists; update them to assert the onClick mock was invoked when the key
interaction is performed: after locating the button via screen.getByRole and
focusing it (as in the tests), either simulate the full browser behavior by
firing the appropriate keyDown/keyUp sequence and then assert onClick was
called, or explicitly call fireEvent.click(button) after the key events and
expect(onClick).toHaveBeenCalled(); use the existing renderButton helper and the
onClick mock name to locate where to change assertions; alternatively add a
short test comment noting jsdom may not synthesize native button click from
keyboard and keep a separate test that directly calls fireEvent.click to verify
the handler.
Comment on lines
+341
to
+347
| it("should be accessible when only icon is provided", () => { | ||
| renderButton({ icon: "plus", text: undefined }); | ||
|
|
||
| const button = screen.getByRole("button"); | ||
| // Icon-only buttons should have an accessible name via aria-label | ||
| expect(button).toBeInTheDocument(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Icon-only button accessibility not actually verified.
The test comment states icon-only buttons "should have an accessible name via aria-label" but only asserts the button exists. This doesn't validate accessibility compliance.
Either add an assertion for aria-label or update the comment to reflect what's actually tested:
it("should be accessible when only icon is provided", () => {
- renderButton({ icon: "plus", text: undefined });
+ renderButton({ icon: "plus", text: undefined, "aria-label": "Add item" });
const button = screen.getByRole("button");
- // Icon-only buttons should have an accessible name via aria-label
- expect(button).toBeInTheDocument();
+ expect(button).toHaveAttribute("aria-label", "Add item");
});📝 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.
Suggested change
| it("should be accessible when only icon is provided", () => { | |
| renderButton({ icon: "plus", text: undefined }); | |
| const button = screen.getByRole("button"); | |
| // Icon-only buttons should have an accessible name via aria-label | |
| expect(button).toBeInTheDocument(); | |
| }); | |
| it("should be accessible when only icon is provided", () => { | |
| renderButton({ icon: "plus", text: undefined, "aria-label": "Add item" }); | |
| const button = screen.getByRole("button"); | |
| expect(button).toHaveAttribute("aria-label", "Add item"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/client/src/components/editorComponents/Button.test.tsx` around lines 341
- 347, The test in Button.test.tsx uses renderButton({ icon: "plus", text:
undefined }) and then only checks screen.getByRole("button"), but the comment
claims icon-only buttons "should have an accessible name via aria-label"; update
the test to actually assert that the rendered button (the `button` variable) has
an accessible name by checking its aria-label (e.g.,
expect(button).toHaveAttribute("aria-label") and that it's non-empty) or, if you
prefer not to assert aria-label, change the test comment to match the current
assertion; locate the test using renderButton and the button obtained via
screen.getByRole to implement this change.
Comment on lines
+53
to
+56
| const editableText = screen.getByText("Initial Value").closest("div"); | ||
| if (editableText) { | ||
| fireEvent.click(editableText); | ||
| } |
Contributor
There was a problem hiding this comment.
Silent test skip on missing element.
Same issue as Collapsible tests - if closest("div") returns null, the click is silently skipped. Assert the element exists first.
This pattern repeats throughout the file (lines 67-70, 83-86, 96-98, 117-120, 145-148, 173-176, 206-209, 241-244, 295-298, 319-322, 344-347, 366-369, 434-437).
const editableText = screen.getByText("Initial Value").closest("div");
- if (editableText) {
- fireEvent.click(editableText);
- }
+ expect(editableText).not.toBeNull();
+ fireEvent.click(editableText!);📝 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.
Suggested change
| const editableText = screen.getByText("Initial Value").closest("div"); | |
| if (editableText) { | |
| fireEvent.click(editableText); | |
| } | |
| const editableText = screen.getByText("Initial Value").closest("div"); | |
| expect(editableText).not.toBeNull(); | |
| fireEvent.click(editableText!); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/client/src/components/editorComponents/EditableText.test.tsx` around
lines 53 - 56, The tests silently skip clicks when closest("div") can return
null—update each occurrence (e.g., variable editableText in
EditableText.test.tsx and the repeated patterns at the other listed spots) to
assert the element exists before firing events: after obtaining the node (const
editableText = screen.getByText(...).closest("div")), add an assertion like
expect(editableText).not.toBeNull() (or if using non-nullable typing,
cast/assert and fail the test if null) and then safely call
fireEvent.click(editableText). Repeat this change for every similar variable
occurrence (the blocks around the repeated line groups) so missing elements
cause failing tests instead of silent skips.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
This PR adds accessibility improvements to the Button and Select components in the Appsmith Design System (ads).
Changes
Button Component (
ads/src/Button/Button.tsx)aria-busyattribute for loading state (WCAG 4.1.2 - Name, Role, Value)aria-labelis properly passed through for icon-only buttonsSelect Component (
ads/src/Select/Select.tsxandSelect.types.ts)isRequiredprop witharia-requiredattribute (WCAG 4.1.2)aria-invalidattribute for validation states (WCAG 4.1.2)aria-busyattribute for loading statearia-label,aria-labelledby, andaria-describedbypropsAccessibility Standards Followed
These changes follow WCAG 2.1 guidelines, specifically:
Testing
How to Test
Test Button component:
isLoading={true}and verifyaria-busy="true"is setaria-label="Close"to an icon-only button and verify it is announced by screen readersTest Select component:
isRequired={true}and verifyaria-required="true"is setisValid={false}and verifyaria-invalid="true"is setaria-label="Country selection"and verify it is announced by screen readersRelated Issues
This PR addresses accessibility improvements for form controls and interactive elements.
Summary by CodeRabbit