Skip to content

Commit 309dc8f

Browse files
viktoravelinoviktoravelino
committed
fix(a11y): name command palette, dialogs, and crash screen (LE-2043)
Pre-existing gaps documented by the round-2 FINDING tests. cmdk hardcodes aria-labelledby and role="separator" after spreading caller props, so the first two needed the primitive reworked rather than a prop. - CommandInput drops cmdk's aria-labelledby when given aria-label, so the caller's name wins instead of resolving to an empty label element - CommandSeparator renders as presentational; separator is not an allowed child of CommandList's listbox - CommandDialog requires a label, naming the dialog and the search input - popoverObject names its Command, the last unnamed combobox call site - EditFlowSettings hides the lock switch when no setLocked is passed, so shareModal stops shipping a focusable inert control - BaseModal ariaLabel now applies to the dialog paths; templatesModal uses it and announces as "Templates" instead of "Dialog" - Crash screen: h1 title, link-wrapped report button, role="alert" Findings 3 and 8 are covered by #14250 and #14110 and left to those PRs.
1 parent 22478d6 commit 309dc8f

10 files changed

Lines changed: 300 additions & 133 deletions

File tree

‎src/frontend/src/components/common/crashErrorComponent/__tests__/crashErrorComponent.a11y.test.tsx‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
import { render, screen } from "@testing-library/react";
2+
import userEvent from "@testing-library/user-event";
3+
import { axe } from "@/utils/a11y-test";
4+
import CrashErrorComponent from "../index";
5+
6+
const renderCrashScreen = (resetErrorBoundary = jest.fn()) =>
7+
render(
8+
<CrashErrorComponent
9+
error={{ message: "boom", stack: "at boom" }}
10+
resetErrorBoundary={resetErrorBoundary}
11+
/>,
12+
);
13+
14+
describe("CrashErrorComponent accessibility", () => {
15+
it("should_have_no_axe_violations", async () => {
16+
const { container } = renderCrashScreen();
17+
18+
expect(await axe(container)).toHaveNoViolations();
19+
});
20+
21+
// The crash screen replaces the whole app, so it owns the page's heading
22+
// structure. Rendering the title as a <p> left the page with zero headings
23+
// (WCAG 1.3.1 / 2.4.6).
24+
it("should_expose_the_title_as_the_page_heading", () => {
25+
renderCrashScreen();
26+
27+
expect(
28+
screen.getByRole("heading", { level: 1, name: /unexpected error/i }),
29+
).toBeInTheDocument();
30+
});
31+
32+
// Nothing announced the failure to a screen reader: the boundary swaps the
33+
// tree without moving focus, so the region has to assert itself (WCAG 4.1.3).
34+
it("should_announce_the_failure_through_an_alert_region", () => {
35+
renderCrashScreen();
36+
37+
expect(screen.getByRole("alert")).toContainElement(
38+
screen.getByRole("heading", { level: 1 }),
39+
);
40+
});
41+
42+
// The report action used to be a <button> nested inside an <a href>, which
43+
// is a nested-interactive violation and gives the anchor no reachable name
44+
// of its own (WCAG 4.1.2).
45+
it("should_render_the_report_action_as_a_single_link", () => {
46+
renderCrashScreen();
47+
48+
const report = screen.getByRole("link", { name: /report on github/i });
49+
expect(report).toHaveAttribute(
50+
"href",
51+
"https://github.com/langflow-ai/langflow/issues/new",
52+
);
53+
expect(report.querySelector("button")).toBeNull();
54+
expect(
55+
screen.queryByRole("button", { name: /report on github/i }),
56+
).not.toBeInTheDocument();
57+
});
58+
59+
it("should_reset_the_error_boundary_from_the_restart_button", async () => {
60+
const user = userEvent.setup();
61+
const resetErrorBoundary = jest.fn();
62+
renderCrashScreen(resetErrorBoundary);
63+
64+
await user.click(screen.getByRole("button", { name: /restart/i }));
65+
66+
expect(resetErrorBoundary).toHaveBeenCalled();
67+
});
68+
});

‎src/frontend/src/components/common/crashErrorComponent/index.tsx‎

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,19 @@ export default function CrashErrorComponent({
1313
<div className="z-50 flex h-screen w-screen items-center justify-center bg-foreground bg-opacity-50">
1414
<div className="flex h-screen w-screen flex-col bg-background text-start shadow-lg">
1515
<main className="m-auto grid w-1/2 justify-center gap-5 text-center">
16-
<Card className="p-8">
16+
<Card className="p-8" role="alert">
1717
<CardHeader>
1818
<div className="m-auto">
19-
<XCircle strokeWidth={1.5} className="h-16 w-16" />
19+
<XCircle
20+
strokeWidth={1.5}
21+
className="h-16 w-16"
22+
aria-hidden="true"
23+
/>
2024
</div>
2125
<div>
22-
<p className="mb-4 text-xl text-foreground">
26+
<h1 className="mb-4 text-xl text-foreground">
2327
{t("crash.title")}
24-
</p>
28+
</h1>
2529
</div>
2630
</CardHeader>
2731

@@ -50,15 +54,15 @@ export default function CrashErrorComponent({
5054
{t("crash.restartButton")}
5155
</Button>
5256

53-
<a
54-
href="https://github.com/langflow-ai/langflow/issues/new"
55-
target="_blank"
56-
rel="noopener noreferrer"
57-
>
58-
<Button className="ml-3" ignoreTitleCase variant={"outline"}>
57+
<Button className="ml-3" variant="outline" asChild>
58+
<a
59+
href="https://github.com/langflow-ai/langflow/issues/new"
60+
target="_blank"
61+
rel="noopener noreferrer"
62+
>
5963
{t("crash.reportButton")}
60-
</Button>
61-
</a>
64+
</a>
65+
</Button>
6266
</div>
6367
</CardFooter>
6468
</Card>

‎src/frontend/src/components/core/editFlowSettingsComponent/index.tsx‎

Lines changed: 27 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -192,34 +192,38 @@ export const EditFlowSettings: React.FC<
192192
<Form.Message match="valueMissing" className="field-invalid">
193193
{t("flow.pleaseEnterDescription")}
194194
</Form.Message>
195-
<div className="mt-3">
196-
<div className="flex items-center gap-2">
197-
<div>
198-
<div className="flex items-center gap-2">
199-
<Form.Label className="text-mmd font-medium">
200-
{t("flow.lockFlow")}
201-
</Form.Label>
195+
{/* Callers that only display the flow (no setLocked) would otherwise
196+
render a focusable switch that cannot change anything. */}
197+
{setLocked && (
198+
<div className="mt-3">
199+
<div className="flex items-center gap-2">
200+
<div>
201+
<div className="flex items-center gap-2">
202+
<Form.Label className="text-mmd font-medium">
203+
{t("flow.lockFlow")}
204+
</Form.Label>
202205

203-
<ForwardedIconComponent
204-
name={locked ? "Lock" : "Unlock"}
205-
className="text-muted-foreground !w-5 !h-5"
206-
/>
206+
<ForwardedIconComponent
207+
name={locked ? "Lock" : "Unlock"}
208+
className="text-muted-foreground !w-5 !h-5"
209+
/>
210+
</div>
211+
212+
<p className="text-xs text-muted-foreground/70 mt-1 font-normal">
213+
{t("flow.lockFlowDescription")}
214+
</p>
207215
</div>
208216

209-
<p className="text-xs text-muted-foreground/70 mt-1 font-normal">
210-
{t("flow.lockFlowDescription")}
211-
</p>
217+
<Switch
218+
checked={!!locked}
219+
onCheckedChange={(v) => setLocked(v)}
220+
disabled={readOnly}
221+
className="data-[state=checked]:bg-primary ml-auto"
222+
data-testid="lock-flow-switch"
223+
/>
212224
</div>
213-
214-
<Switch
215-
checked={!!locked}
216-
onCheckedChange={(v) => setLocked?.(v)}
217-
disabled={readOnly}
218-
className="data-[state=checked]:bg-primary ml-auto"
219-
data-testid="lock-flow-switch"
220-
/>
221225
</div>
222-
</div>
226+
)}
223227
</Form.Field>
224228
</>
225229
);

‎src/frontend/src/components/core/parameterRenderComponent/components/inputComponent/components/popoverObject/index.tsx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { PopoverAnchor } from "@radix-ui/react-popover";
22
import { useEffect } from "react";
3+
import { useTranslation } from "react-i18next";
34
import ForwardedIconComponent from "@/components/common/genericIconComponent";
45
import {
56
Command,
@@ -43,6 +44,7 @@ const CustomInputPopoverObject = ({
4344
showOptions,
4445
inspectionPanel,
4546
}) => {
47+
const { t } = useTranslation();
4648
const PopoverContentInput =
4749
editNode || inspectionPanel ? PopoverContent : PopoverContentWithoutPortal;
4850

@@ -122,6 +124,7 @@ const CustomInputPopoverObject = ({
122124
align="center"
123125
>
124126
<Command
127+
label={optionsPlaceholder || t("input.searchOptions")}
125128
filter={(value, search) => {
126129
if (
127130
value.toLowerCase().includes(search.toLowerCase()) ||

‎src/frontend/src/components/ui/__tests__/command.a11y.test.tsx‎

Lines changed: 74 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,18 @@ const renderInlineCommand = () =>
3535
</Command>,
3636
);
3737

38+
const renderInlineCommandWithoutInputLabel = () =>
39+
render(
40+
<Command label="Component search">
41+
<CommandInput placeholder="Search…" />
42+
<CommandList>
43+
<CommandGroup heading="Inputs">
44+
<CommandItem>Chat Input</CommandItem>
45+
</CommandGroup>
46+
</CommandList>
47+
</Command>,
48+
);
49+
3850
describe("Command accessibility", () => {
3951
it("should_have_no_axe_violations_when_rendered_inline", async () => {
4052
const { container } = renderInlineCommand();
@@ -57,28 +69,32 @@ describe("Command accessibility", () => {
5769
expect(screen.getByRole("group", { name: "Inputs" })).toBeInTheDocument();
5870
});
5971

60-
// FINDING (documented, not fixed): cmdk always stamps its own
61-
// `aria-labelledby` (pointing at the `<Command label>` element) onto the
62-
// input, and aria-labelledby outranks aria-label in the accessible-name
63-
// computation. A caller passing `aria-label` to CommandInput therefore has
64-
// it silently ignored whenever the wrapping `<Command>` sets `label`. The
65-
// input is still named — just not with the name the call site asked for.
66-
it("should_name_the_input_from_the_command_label_not_the_input_aria_label", () => {
72+
// cmdk stamps its own `aria-labelledby` (pointing at the `<Command label>`
73+
// element) onto the input, and aria-labelledby would outrank aria-label in
74+
// the accessible-name computation. CommandInput drops that reference when a
75+
// caller passes `aria-label`, so the name the call site asked for wins.
76+
it("should_name_the_input_from_its_own_aria_label", () => {
6777
renderInlineCommand();
6878

6979
const input = screen.getByRole("combobox");
70-
expect(input).toHaveAttribute("aria-label", "Search components");
71-
expect(input).toHaveAccessibleName("Component search");
80+
expect(input).not.toHaveAttribute("aria-labelledby");
81+
expect(input).toHaveAccessibleName("Search components");
7282
});
7383

74-
// FINDING (documented, not fixed): the corollary of the above. When the
75-
// wrapping `<Command>` has no `label`, cmdk's `aria-labelledby` resolves to
76-
// an empty element, and because it still outranks `aria-label` the search
77-
// box ends up with NO accessible name at all. In other words `<Command
78-
// label>` is the only way to name a CommandInput — passing `aria-label` to
79-
// the input is a no-op. Every call site that relies on `aria-label` alone
80-
// ships an unnamed combobox (WCAG 4.1.2).
81-
it("should_leave_the_input_unnamed_when_the_command_has_no_label", () => {
84+
// Without an `aria-label` the input keeps cmdk's own labelling, so
85+
// `<Command label>` still names it.
86+
it("should_fall_back_to_the_command_label_when_the_input_has_no_aria_label", () => {
87+
renderInlineCommandWithoutInputLabel();
88+
89+
expect(screen.getByRole("combobox")).toHaveAccessibleName(
90+
"Component search",
91+
);
92+
});
93+
94+
// Previously the input ended up with no accessible name at all here: cmdk's
95+
// aria-labelledby pointed at the empty `<Command>` label element and still
96+
// outranked aria-label (WCAG 4.1.2).
97+
it("should_name_the_input_when_the_command_has_no_label", () => {
8298
render(
8399
<Command>
84100
<CommandInput aria-label="Search components" />
@@ -88,9 +104,9 @@ describe("Command accessibility", () => {
88104
</Command>,
89105
);
90106

91-
const input = screen.getByRole("combobox");
92-
expect(input).toHaveAttribute("aria-label", "Search components");
93-
expect(input).toHaveAccessibleName("");
107+
expect(screen.getByRole("combobox")).toHaveAccessibleName(
108+
"Search components",
109+
);
94110
});
95111

96112
it("should_expose_combobox_state_on_the_input", () => {
@@ -128,7 +144,7 @@ describe("Command accessibility", () => {
128144

129145
it("should_have_no_axe_violations_when_rendered_in_a_dialog", async () => {
130146
render(
131-
<CommandDialog open>
147+
<CommandDialog open label="Command palette">
132148
<CommandInput aria-label="Search components" />
133149
<CommandList>
134150
<CommandItem>Chat Input</CommandItem>
@@ -143,30 +159,27 @@ describe("Command accessibility", () => {
143159
).toHaveNoViolations();
144160
});
145161

146-
// FINDING (documented, not fixed): CommandDialog passes no DialogTitle, so
147-
// DialogContent injects its visually-hidden "Dialog" fallback and the
148-
// command palette announces as literally "Dialog". Asserting the current
149-
// behaviour means a future real title trips this test instead of silently
150-
// passing, at which point the expectation should be updated.
151-
it("should_expose_the_command_dialog_with_its_current_fallback_name", () => {
162+
// The palette used to announce as the literal string "Dialog": CommandDialog
163+
// passed no DialogTitle, so DialogContent injected its visually-hidden
164+
// fallback. `label` now names both the dialog and the search input.
165+
it("should_name_the_command_dialog_from_its_label", () => {
152166
render(
153-
<CommandDialog open>
154-
<CommandInput aria-label="Search components" />
167+
<CommandDialog open label="Command palette">
155168
<CommandList>
156169
<CommandItem>Chat Input</CommandItem>
157170
</CommandList>
158171
</CommandDialog>,
159172
);
160173

161-
expect(screen.getByRole("dialog", { name: "Dialog" })).toBeInTheDocument();
174+
expect(
175+
screen.getByRole("dialog", { name: "Command palette" }),
176+
).toBeInTheDocument();
162177
});
163178

164-
// FINDING (documented, not fixed): CommandSeparator renders
165-
// `role="separator"` as a direct child of CommandList's `role="listbox"`.
166-
// A listbox may only own `option` / `group` children, so axe reports
167-
// `aria-required-children`. Fixing it belongs in command.tsx (the separator
168-
// needs `role="presentation"`), so this test pins the current behaviour.
169-
it("should_report_a_separator_inside_the_listbox_as_a_disallowed_child", async () => {
179+
// cmdk's own Separator renders `role="separator"` as a direct child of
180+
// CommandList's `role="listbox"`, which may only own `option` / `group`
181+
// children (axe aria-required-children). Ours is presentational instead.
182+
it("should_render_the_separator_as_presentational_inside_the_listbox", async () => {
170183
const { container } = render(
171184
<Command label="Component search">
172185
<CommandInput aria-label="Search components" />
@@ -182,10 +195,30 @@ describe("Command accessibility", () => {
182195
</Command>,
183196
);
184197

185-
const results = await axe(container, {
186-
rules: { region: { enabled: false } },
187-
});
188-
const violationIds = results.violations.map((violation) => violation.id);
189-
expect(violationIds).toContain("aria-required-children");
198+
expect(screen.queryByRole("separator")).not.toBeInTheDocument();
199+
expect(
200+
await axe(container, { rules: { region: { enabled: false } } }),
201+
).toHaveNoViolations();
202+
});
203+
204+
it("should_hide_the_separator_while_a_search_is_active", async () => {
205+
const user = userEvent.setup();
206+
const { container } = render(
207+
<Command label="Component search">
208+
<CommandInput aria-label="Search components" />
209+
<CommandList>
210+
<CommandGroup heading="Inputs">
211+
<CommandItem>Chat Input</CommandItem>
212+
</CommandGroup>
213+
<CommandSeparator />
214+
</CommandList>
215+
</Command>,
216+
);
217+
218+
expect(container.querySelector("[cmdk-separator]")).toBeInTheDocument();
219+
220+
await user.type(screen.getByRole("combobox"), "chat");
221+
222+
expect(container.querySelector("[cmdk-separator]")).not.toBeInTheDocument();
190223
});
191224
});

0 commit comments

Comments
 (0)