-
Notifications
You must be signed in to change notification settings - Fork 9.7k
fix(a11y): resolve IBM Equal Access violations on sidebar, canvas, and note contrast #14212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: LE-1968
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| /** | ||
| * Regression test for IBM Equal Access `text_contrast_sufficient` violation: | ||
| * preset-colored note backgrounds (amber/rose/lime/blue/neutral) previously fell | ||
| * back to `text-muted-foreground` in light mode with no override, which fails | ||
| * WCAG AA contrast against light pastel backgrounds (e.g. 3.98:1 on amber vs the | ||
| * 4.5:1 required). The fix adds a `text-foreground` override for presets in light | ||
| * mode, alongside the pre-existing `dark:` overrides. | ||
| */ | ||
| import { render } from "@testing-library/react"; | ||
| import { COLOR_OPTIONS } from "@/constants/constants"; | ||
| import type { NoteDataType } from "@/types/flow"; | ||
| import NoteNode from "../index"; | ||
|
|
||
| jest.mock("@xyflow/react", () => ({ | ||
| NodeResizer: () => null, | ||
| })); | ||
|
|
||
| jest.mock("@/contexts/permissionsContext", () => ({ | ||
| useIsFlowReadOnly: () => false, | ||
| })); | ||
|
|
||
| jest.mock("@/controllers/API/queries/flows/use-get-note-translations", () => ({ | ||
| useGetNoteTranslationsQuery: () => ({ data: undefined }), | ||
| })); | ||
|
|
||
| jest.mock("@/stores/flowStore", () => ({ | ||
| __esModule: true, | ||
| default: (selector: (state: Record<string, unknown>) => unknown) => | ||
| selector({ | ||
| currentFlow: { id: "flow-1", data: { nodes: [] } }, | ||
| setNode: jest.fn(), | ||
| }), | ||
| syncNoteTranslations: jest.fn(), | ||
| })); | ||
|
|
||
| jest.mock("../NoteToolbarComponent", () => ({ | ||
| __esModule: true, | ||
| default: () => null, | ||
| })); | ||
|
|
||
| let lastNodeDescriptionProps: Record<string, unknown> = {}; | ||
| jest.mock("../../GenericNode/components/NodeDescription", () => ({ | ||
| __esModule: true, | ||
| default: (props: Record<string, unknown>) => { | ||
| lastNodeDescriptionProps = props; | ||
| return null; | ||
| }, | ||
| })); | ||
|
|
||
| describe("NoteNode text contrast on preset backgrounds", () => { | ||
| beforeEach(() => { | ||
| lastNodeDescriptionProps = {}; | ||
| }); | ||
|
|
||
| Object.keys(COLOR_OPTIONS) | ||
| .filter((color) => COLOR_OPTIONS[color] !== null) | ||
| .forEach((color) => { | ||
| it(`applies a light-mode text-foreground override for the "${color}" preset`, () => { | ||
| const data = { | ||
| id: "note-1", | ||
| node: { | ||
| description: "Some note text", | ||
| template: { backgroundColor: color }, | ||
| }, | ||
| } as unknown as NoteDataType; | ||
|
|
||
| render(<NoteNode data={data} />); | ||
|
|
||
| expect(lastNodeDescriptionProps.inputClassName).toContain( | ||
| "text-foreground", | ||
| ); | ||
| expect(lastNodeDescriptionProps.mdClassName).toContain( | ||
| "text-foreground", | ||
| ); | ||
| // Dark-mode override must be preserved alongside the new light-mode fix. | ||
| expect(lastNodeDescriptionProps.inputClassName).toContain( | ||
| "dark:text-background", | ||
| ); | ||
| expect(lastNodeDescriptionProps.mdClassName).toContain( | ||
| "dark:!text-background", | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| it("leaves the transparent/no-background preset without a forced text color", () => { | ||
| const data = { | ||
| id: "note-1", | ||
| node: { | ||
| description: "Some note text", | ||
| template: { backgroundColor: "transparent" }, | ||
| }, | ||
| } as unknown as NoteDataType; | ||
|
|
||
| render(<NoteNode data={data} />); | ||
|
|
||
| expect(lastNodeDescriptionProps.inputClassName).not.toContain( | ||
| "text-foreground", | ||
| ); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,7 @@ import { | |||||||||||||||
| type MouseEvent, | ||||||||||||||||
| useCallback, | ||||||||||||||||
| useEffect, | ||||||||||||||||
| useMemo, | ||||||||||||||||
| useRef, | ||||||||||||||||
| useState, | ||||||||||||||||
| } from "react"; | ||||||||||||||||
|
|
@@ -116,6 +117,17 @@ export default function Page({ | |||||||||||||||
| const edges = useFlowStore((state) => state.edges); | ||||||||||||||||
| const isEmptyFlow = useRef(nodes.length === 0); | ||||||||||||||||
|
|
||||||||||||||||
| const nodesWithAriaLabel = useMemo( | ||||||||||||||||
| () => | ||||||||||||||||
| nodes.map((node) => ({ | ||||||||||||||||
| ...node, | ||||||||||||||||
| ariaLabel: t("flow.nodeAriaLabel", { | ||||||||||||||||
| name: node.data?.node?.display_name ?? node.data?.type, | ||||||||||||||||
| }), | ||||||||||||||||
|
Comment on lines
+124
to
+126
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Fall back when The Note creation path sets Proposed fix- name: node.data?.node?.display_name ?? node.data?.type,
+ name:
+ node.data?.node?.display_name || node.data?.type || node.type,📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||
| })), | ||||||||||||||||
| [nodes, t], | ||||||||||||||||
| ); | ||||||||||||||||
|
|
||||||||||||||||
| const previewLabel = useVersionPreviewStore((s) => s.previewLabel); | ||||||||||||||||
| const isPreviewActive = previewLabel !== null; | ||||||||||||||||
| const isWelcomeOpen = useFlowBuilderWelcomeStore((state) => state.isOpen); | ||||||||||||||||
|
|
@@ -918,7 +930,8 @@ export default function Page({ | |||||||||||||||
| onClick={handleGroupNode} | ||||||||||||||||
| /> | ||||||||||||||||
| <ReactFlow<AllNodeType, EdgeType> | ||||||||||||||||
| nodes={nodes} | ||||||||||||||||
| aria-label={t("flow.canvasLabel")} | ||||||||||||||||
| nodes={nodesWithAriaLabel} | ||||||||||||||||
| edges={edges} | ||||||||||||||||
| onNodesChange={onNodesChangeWithHelperLines} | ||||||||||||||||
| onEdgesChange={onEdgesChange} | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,6 +131,10 @@ export const SidebarDraggableComponent = forwardRef( | |
| }} | ||
| key={itemName} | ||
| data-tooltip-id={itemName} | ||
| role="button" | ||
| aria-label={t("sidebar.addComponentToCanvas", { | ||
| name: display_name, | ||
| })} | ||
| tabIndex={0} | ||
|
Comment on lines
+134
to
138
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "Locate file"
fd -a 'sidebarDraggableComponent\.tsx$' . || true
echo "Inspect outline"
path="$(fd 'sidebarDraggableComponent\.tsx$' . | head -n1)"
if [ -n "${path:-}" ]; then
ast-grep outline "$path" --match 'SidebarDraggableComponent' --view expanded || true
echo "--- relevant lines ---"
sed -n '1,230p' "$path" | nl -ba
fi
echo "Search related attributes"
if [ -n "${path:-}" ]; then
rg -n "role=\"button\"|tabIndex|onKeyDown|aria-disabled|isDisabled|disabled|SelectTrigger" "$path" || true
fiRepository: langflow-ai/langflow Length of output: 538 🏁 Script executed: #!/bin/bash
set -euo pipefail
path="./src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/sidebarDraggableComponent.tsx"
echo "File line count"
wc -l "$path"
echo "Relevant lines 1-260"
awk '{printf "%5d\t%s\n", NR, $0}' "$path" | sed -n '1,260p'
echo "Search within file"
rg -n "role=\"button\"|ariaLabel|tabIndex|onKeyDown|aria-disabled|isDisabled|disabled|SelectTrigger|Button" "$path" || trueRepository: langflow-ai/langflow Length of output: 11329 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "Use add component hook implementation"
rg -n "export function useAddComponent|function useAddComponent|const useAddComponent|m:\\/\\/.*useAddComponent|useAddComponent" src/frontend/src/hooks src/frontend/src -g '*.ts' -g '*.tsx' || true
hook_file="$(rg -l "useAddComponent" src/frontend/src/hooks src/frontend/src/components src/frontend/src/stores 2>/dev/null | head -n1 || true)"
if [ -n "${hook_file:-}" ]; then
echo "--- hook file: $hook_file ---"
sed -n '1,220p' "$hook_file" | awk '{printf "%5d\t%s\n", NR, $0}'
fi
echo "Interactive controls in parent/sidebar context"
parent="$(dirname "$(fd 'sidebarDraggableComponent\.tsx$' . | head -n1)")"
rg -n "SidebarDraggableComponent|disabled|component\\.disabled|isDisabled|disabledTooltip" "$parent"/.. src/frontend/src/pages src/frontend/src/components src/frontend/src/stores -g '*.ts' -g '*.tsx' || trueRepository: langflow-ai/langflow Length of output: 50377 🏁 Script executed: #!/bin/bash
set -euo pipefail
path="./src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/sidebarDraggableComponent.tsx"
echo "Remaining relevant lines 260-276"
sed -n '260,276p' "$path" | awk '{printf "%5d\t%s\n", NR+259, $0}'
echo "Check for aria-disabled or activation guard in file"
rg -n "aria-disabled|disabled\\?|!disabled|handleKeyDown|role=\"button\"" "$path" || true
echo "Static check of activation functions referenced around role button"
python3 - <<'PY'
from pathlib import Path
import re
p = Path("./src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/sidebarDraggableComponent.tsx")
text = p.read_text()
checks = {
"role_button": r'<div[^>]*role\s*=\s*"\bbutton\b"',
"tabindex_0_wrapper": r'<div[^>]*tabIndex\s*=\s*\{0\}[^>]*onKeyDown\s*=\s*\{handleKeyDown\}[^>]*>',
"handle_keydown_add_component": r'function handleKeyDown.*?[^}]?\n[^\n]*addComponent\(\s*apiClass\s*,\s*itemName\s*\)',
"disabled_reference_in_keydown": re.compile(r'handleKeyDown\([^)]+\)\s*\{(?P<body>.*?)[\}]', re.S),
"child_tabindex_minus_1": r'tabIndex\s*=\s*\{-1\}',
"conditional_add_button": r'!\s*disabled\s*\{\s*<Button',
}
for name, pattern in checks.items():
m = re.search(pattern, text, flags=re.S if isinstance(pattern, str) else re.S)
body = m.group("body") if isinstance(pattern, str) and name == "disabled_reference_in_keydown" else None
print(f"{name}: {bool(m)}" + (f" | body={repr(body.strip())}" if body else ""))
PYRepository: langflow-ai/langflow Length of output: 1429 🏁 Script executed: #!/bin/bash
set -euo pipefail
path="./src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/sidebarDraggableComponent.tsx"
echo "Relevant lines around keyboard handler and wrapper"
sed -n '95,145p' "$path" | awk '{printf "%5d\t%s\n", NR+94, $0}'
echo "Search for disabled gating around handler or wrapper"
rg -n "handleKeyDown|disabled|aria-disabled|role=\"button\"" "$path" || true
echo "Read-only structural validation"
python3 - <<'PY'
from pathlib import Path
p = Path("./src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/sidebarDraggableComponent.tsx")
text = p.read_text()
print("has_handleKeyDown_addComponent_no_disabled_guard=", "handleKeyDown" in text and "addComponent(apiClass, itemName)" in text and "disabled" not in text.split("const handleKeyDown",1)[1].split("};",1)[0])
print("has_wrapper_role_button=", 'role="button"' in text and 'tabIndex={0}' in text and 'onKeyDown={handleKeyDown}' in text)
print("has_child_tabindex_minus_1=", 'tabIndex={-1}' in text)
print("has_conditional_add_button=", '{"} {<Button' in text or '{!disabled && (' in text)
PYRepository: langflow-ai/langflow Length of output: 2640 Do not expose this composite as a single button. The wrapper is focusable and traps keyboard activation on the Add/Select actions via 🤖 Prompt for AI Agents |
||
| onKeyDown={handleKeyDown} | ||
| className="rounded-md outline-none ring-ring focus-visible:ring-1" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the changed placeholder contrast contract.
The test never asserts
placeholderClassName, so a regression in the newtext-foreground/70 dark:!text-backgroundbehavior atNoteNode/index.tsxLine 239 would pass unnoticed. Assert it for both opaque presets and the transparent preset.Proposed test additions
expect(lastNodeDescriptionProps.mdClassName).toContain( "dark:!text-background", ); + expect(lastNodeDescriptionProps.placeholderClassName).toContain( + "text-foreground/70", + ); + expect(lastNodeDescriptionProps.placeholderClassName).toContain( + "dark:!text-background", + ); ... expect(lastNodeDescriptionProps.inputClassName).not.toContain( "text-foreground", ); + expect(lastNodeDescriptionProps.placeholderClassName).not.toContain( + "text-foreground", + );As per coding guidelines, frontend tests must “verify the tests actually cover the new or changed behavior rather than acting as placeholders.”
📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Coding guidelines