fix(playground): add accessible names to icon-only controls and fix invalid ARIA on animated/status elements - #14110
fix(playground): add accessible names to icon-only controls and fix invalid ARIA on animated/status elements#14110olayinkaadelakun wants to merge 3 commits into
Conversation
…nvalid ARIA on animated/status elements
…props, and use shared axe helper in a11y tests
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAccessibility labels, decorative-icon handling, conditional animated-text semantics, localization keys, and Jest axe coverage were added for theme controls, session renaming, and the playground modal. ChangesFrontend accessibility and localization
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (5 passed)
✨ 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 |
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
github-actions
Bot
added
bug
![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
🧹 Nitpick comments (3)
src/frontend/src/components/ui/__tests__/textAnimation.a11y.test.tsx (1)
5-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative test case to verify omitted accessibility attributes.
As per coding guidelines, frontend test files must include coverage for positive, negative, edge, and error cases. This test file currently only covers the positive case (where
per="char"appropriately appliesrole="img"). Please add a negative test case for the underlyingTextEffectcomponent withper="line"to verify that therole="img"attribute is intentionally omitted.🧪 Proposed test addition
-import { TextEffectPerChar } from "../textAnimation"; +import { TextEffect, TextEffectPerChar } from "../textAnimation"; -describe("TextEffectPerChar accessibility", () => { +describe("TextEffect accessibility", () => { it("has no detectable axe violations", async () => { const { container } = render( <TextEffectPerChar>Test your flow with a chat prompt</TextEffectPerChar>, ); const results = await axe(container); expect(results).toHaveNoViolations(); }); it("exposes the full string as one accessible name via role=img", () => { render( <TextEffectPerChar>Test your flow with a chat prompt</TextEffectPerChar>, ); expect( screen.getByRole("img", { name: "Test your flow with a chat prompt" }), ).toBeInTheDocument(); }); + + it("does not apply role=img when per is 'line'", () => { + render( + <TextEffect per="line">Test your flow</TextEffect>, + ); + + expect(screen.queryByRole("img")).not.toBeInTheDocument(); + }); });🤖 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 `@src/frontend/src/components/ui/__tests__/textAnimation.a11y.test.tsx` around lines 5 - 25, Add a negative accessibility test in the “TextEffectPerChar accessibility” suite that renders the underlying TextEffect component with per="line" and verifies no role="img" element is exposed. Keep the existing positive and axe tests unchanged.Source: Coding guidelines
src/frontend/src/components/core/appHeaderComponent/components/ThemeButtons/__tests__/ThemeButtons.a11y.test.tsx (1)
14-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the decorative-icon semantics.
The test verifies button names but not the other changed behavior: each icon should remain hidden from the accessibility tree. Add an assertion for the rendered icon’s
aria-hidden="true"attribute so this regression is detected.
As per coding guidelines, frontend tests should verify meaningful behavior for new functionality rather than only smoke-testing it.🤖 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 `@src/frontend/src/components/core/appHeaderComponent/components/ThemeButtons/__tests__/ThemeButtons.a11y.test.tsx` around lines 14 - 25, Add an accessibility assertion to the existing “names the light, dark, and system theme buttons” test for the rendered decorative icons, verifying each has aria-hidden="true". Use the existing ThemeButtons render and accessible queries, targeting the icon elements without changing the button-name assertions.Source: Coding guidelines
src/frontend/src/modals/IOModal/__tests__/playground-modal.a11y.test.tsx (1)
38-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRun the axe scans against the real modal structure.
Mocking
BaseModalas plain<div>elements removes dialog semantics, focus handling, and ARIA relationships from both axe scans. Keep the focused button tests, but add integration-level axe coverage using the real modal primitive.As per coding guidelines, warn when excessive mocks obscure what is actually being tested and prefer integration tests when unit tests are overly mocked.
Also applies to: 211-217, 253-260
🤖 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 `@src/frontend/src/modals/IOModal/__tests__/playground-modal.a11y.test.tsx` around lines 38 - 64, Reduce the mocking in the accessibility tests around the BaseModal mock so axe scans exercise the real modal primitive and preserve its dialog semantics, focus handling, and ARIA relationships. Keep the existing focused-button tests and add integration-level axe coverage using the actual BaseModal, while retaining child-component mocks only where they do not obscure modal behavior.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
`@src/frontend/src/modals/IOModal/components/IOFieldView/components/__tests__/session-selector.a11y.test.tsx`:
- Around line 85-100: Update the “switches to editing mode and exposes named
cancel/confirm buttons” test to also query the rename textbox by its expected
accessible name and assert that it is present, alongside the existing named
button assertions.
In `@src/frontend/src/modals/IOModal/playground-modal.tsx`:
- Line 405: Update the sidebar toggle button in the IOModal playground to
conditionally use “Show sidebar” when collapsed and “Hide sidebar” when expanded
for both its tooltip and aria-label. Extend the relevant test to click the
toggle and assert the resulting “Show sidebar” accessible label.
---
Nitpick comments:
In
`@src/frontend/src/components/core/appHeaderComponent/components/ThemeButtons/__tests__/ThemeButtons.a11y.test.tsx`:
- Around line 14-25: Add an accessibility assertion to the existing “names the
light, dark, and system theme buttons” test for the rendered decorative icons,
verifying each has aria-hidden="true". Use the existing ThemeButtons render and
accessible queries, targeting the icon elements without changing the button-name
assertions.
In `@src/frontend/src/components/ui/__tests__/textAnimation.a11y.test.tsx`:
- Around line 5-25: Add a negative accessibility test in the “TextEffectPerChar
accessibility” suite that renders the underlying TextEffect component with
per="line" and verifies no role="img" element is exposed. Keep the existing
positive and axe tests unchanged.
In `@src/frontend/src/modals/IOModal/__tests__/playground-modal.a11y.test.tsx`:
- Around line 38-64: Reduce the mocking in the accessibility tests around the
BaseModal mock so axe scans exercise the real modal primitive and preserve its
dialog semantics, focus handling, and ARIA relationships. Keep the existing
focused-button tests and add integration-level axe coverage using the actual
BaseModal, while retaining child-component mocks only where they do not obscure
modal behavior.
🪄 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: 31c32499-427a-4eac-880f-d40a61071442
📒 Files selected for processing (15)
src/frontend/src/components/core/appHeaderComponent/components/ThemeButtons/__tests__/ThemeButtons.a11y.test.tsxsrc/frontend/src/components/core/appHeaderComponent/components/ThemeButtons/index.tsxsrc/frontend/src/components/ui/__tests__/textAnimation.a11y.test.tsxsrc/frontend/src/components/ui/textAnimation.tsxsrc/frontend/src/locales/de.jsonsrc/frontend/src/locales/en.jsonsrc/frontend/src/locales/es.jsonsrc/frontend/src/locales/fr.jsonsrc/frontend/src/locales/ja.jsonsrc/frontend/src/locales/pt.jsonsrc/frontend/src/locales/zh-Hans.jsonsrc/frontend/src/modals/IOModal/__tests__/playground-modal.a11y.test.tsxsrc/frontend/src/modals/IOModal/components/IOFieldView/components/__tests__/session-selector.a11y.test.tsxsrc/frontend/src/modals/IOModal/components/IOFieldView/components/session-selector.tsxsrc/frontend/src/modals/IOModal/playground-modal.tsx
| it("switches to editing mode and exposes named cancel/confirm buttons", async () => { | ||
| const user = userEvent.setup(); | ||
| renderSessionSelector(); | ||
|
|
||
| await user.click(screen.getByRole("combobox", { name: "Options" })); | ||
| await user.click(screen.getByRole("option", { name: /rename/i })); | ||
|
|
||
| const cancelButton = screen.getByRole("button", { | ||
| name: "Cancel rename", | ||
| }); | ||
| const confirmButton = screen.getByRole("button", { | ||
| name: "Confirm rename", | ||
| }); | ||
| expect(cancelButton).toBeInTheDocument(); | ||
| expect(confirmButton).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Assert the rename input’s accessible name.
The test verifies the new button labels but queries the changed textbox without its name elsewhere. Add an assertion such as:
+ expect(
+ screen.getByRole("textbox", { name: "Rename session" }),
+ ).toBeInTheDocument();As per coding guidelines, frontend tests must verify the new or changed behavior rather than act as placeholders.
📝 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.
| it("switches to editing mode and exposes named cancel/confirm buttons", async () => { | |
| const user = userEvent.setup(); | |
| renderSessionSelector(); | |
| await user.click(screen.getByRole("combobox", { name: "Options" })); | |
| await user.click(screen.getByRole("option", { name: /rename/i })); | |
| const cancelButton = screen.getByRole("button", { | |
| name: "Cancel rename", | |
| }); | |
| const confirmButton = screen.getByRole("button", { | |
| name: "Confirm rename", | |
| }); | |
| expect(cancelButton).toBeInTheDocument(); | |
| expect(confirmButton).toBeInTheDocument(); | |
| }); | |
| it("switches to editing mode and exposes named cancel/confirm buttons", async () => { | |
| const user = userEvent.setup(); | |
| renderSessionSelector(); | |
| await user.click(screen.getByRole("combobox", { name: "Options" })); | |
| await user.click(screen.getByRole("option", { name: /rename/i })); | |
| expect( | |
| screen.getByRole("textbox", { name: "Rename session" }), | |
| ).toBeInTheDocument(); | |
| const cancelButton = screen.getByRole("button", { | |
| name: "Cancel rename", | |
| }); | |
| const confirmButton = screen.getByRole("button", { | |
| name: "Confirm rename", | |
| }); | |
| expect(cancelButton).toBeInTheDocument(); | |
| expect(confirmButton).toBeInTheDocument(); | |
| }); |
🤖 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
`@src/frontend/src/modals/IOModal/components/IOFieldView/components/__tests__/session-selector.a11y.test.tsx`
around lines 85 - 100, Update the “switches to editing mode and exposes named
cancel/confirm buttons” test to also query the rename textbox by its expected
accessible name and assert that it is present, alongside the existing named
button assertions.
Source: Coding guidelines
| variant="ghost" | ||
| className="flex h-8 w-8 items-center justify-center !p-0" | ||
| onClick={() => setSidebarOpen(!sidebarOpen)} | ||
| aria-label={t("modal.io.hideSidebar")} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Switch the label when the sidebar is collapsed.
After the first click, this button opens the sidebar but remains named “Hide sidebar.” Make both its tooltip and accessible label conditional, and assert the resulting “Show sidebar” state in the test.
Proposed fix
- content={t("modal.io.hideSidebar")}
+ content={t(
+ sidebarOpen
+ ? "modal.io.hideSidebar"
+ : "modal.io.showSidebar",
+ )}
...
- aria-label={t("modal.io.hideSidebar")}
+ aria-label={t(
+ sidebarOpen
+ ? "modal.io.hideSidebar"
+ : "modal.io.showSidebar",
+ )}📝 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.
| aria-label={t("modal.io.hideSidebar")} | |
| content={t( | |
| sidebarOpen | |
| ? "modal.io.hideSidebar" | |
| : "modal.io.showSidebar", | |
| )} | |
| ... | |
| aria-label={t( | |
| sidebarOpen | |
| ? "modal.io.hideSidebar" | |
| : "modal.io.showSidebar", | |
| )} |
🤖 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 `@src/frontend/src/modals/IOModal/playground-modal.tsx` at line 405, Update the
sidebar toggle button in the IOModal playground to conditionally use “Show
sidebar” when collapsed and “Hide sidebar” when expanded for both its tooltip
and aria-label. Extend the relevant test to click the toggle and assert the
resulting “Show sidebar” accessible label.
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-1.12.0 #14110 +/- ##
==================================================
- Coverage 61.30% 60.51% -0.79%
==================================================
Files 2342 2333 -9
Lines 237114 234413 -2701
Branches 33340 33041 -299
==================================================
- Hits 145354 141866 -3488
- Misses 89963 90834 +871
+ Partials 1797 1713 -84
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
github-actions
Bot
added
bug
2 participants
Summary
Fixes several accessibility violations on
/playground/:id(found via a Lighthouse a11y audit and manual Chrome DevTools accessibility-tree inspection) and the sharedIOModal/playground-modal component underlying it — the same code renders whether the playground is opened as its own route or as an overlay from the flow canvas.Changes
Unlabeled icon-only controls (Lighthouse
button-name/ manual "no associated label" findings):playground-modal.tsx— sidebar-collapse toggle button had noaria-label; both "Built with Langflow" button variants (sidebar-open and sidebar-collapsed) either lacked a label or had an unhidden decorative SVG child exposed to assistive techsession-selector.tsx— the rename cancel/confirm buttons, the row's "options" combobox trigger, and the rename<Input>itself all had no accessible nameThemeButtons/index.tsx(rendered in the playground's publish-options sidebar panel) — Light/Dark/System theme buttons were icon-only with noaria-labelInvalid ARIA usage (
aria-labelon a generic-role element):textAnimation.tsx— the "New chat" empty-state text is rendered via per-character animated<span>s, witharia-labelset on the wrapping element to expose the full string as one accessible name. That wrapper defaults to a plain<span>/<div>(implicit rolegeneric), andaria-labelisn't valid on generic-role elements per the ARIA-in-HTML spec. Fixed withrole="img"— the standard technique for collapsing animated child nodes into one accessible text alternative.Sidebar-toggle label bug:
playground-modal.tsx— the sidebar-toggle'saria-label/tooltip content was hardcoded to "Hide sidebar" and never flipped to "Show sidebar" after collapsing, even though the icon on the same button correctly ternaries. Both strings now tracksidebarOpenstate.Inert
aria-hiddenprop:ThemeButtons/index.tsx,session-selector.tsx,playground-modal.tsx—aria-hidden="true"had been added to 6IconComponent/ForwardedIconComponentusages, but that's not a prop the component recognizes (it only destructures camelCaseariaHidden); the kebab-case attribute was silently dropped. Harmless today only because the component already defaults unlabeled icons to hidden. Replaced withariaHidden.Test coverage added
Four new
*.a11y.test.tsxfiles (axe scans + accessible-name/role assertions), all using the repo's shared@/utils/a11y-testhelper (disables the jsdom-unreliablecolor-contrastrule) for consistency with the existing ~18 other*.a11y.test.tsxfiles:ThemeButtons/__tests__/ThemeButtons.a11y.test.tsxcomponents/ui/__tests__/textAnimation.a11y.test.tsxmodals/IOModal/__tests__/playground-modal.a11y.test.tsx— includes a regression test locking in the sidebar-toggle label flipIOFieldView/components/__tests__/session-selector.a11y.test.tsxSummary by CodeRabbit
Accessibility
Localization
Tests