-
Notifications
You must be signed in to change notification settings - Fork 9.7k
fix(a11y): add accessible names to icon-only buttons, fix placeholder #13666
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
Changes from 3 commits
0b74b7a
54a8903
3a4b6b1
c98ed09
bac8a7b
e4f420f
366aa29
c6556d7
1773809
048e397
60d1bbe
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,107 @@ | ||
| import { render, screen } from "@testing-library/react"; | ||
| import AppHeader from "../index"; | ||
|
|
||
| // i18n — returns the key so aria-label values are predictable | ||
| jest.mock("react-i18next", () => ({ | ||
| useTranslation: () => ({ t: (key: string) => key }), | ||
| })); | ||
|
|
||
| jest.mock("@/components/common/genericIconComponent", () => ({ | ||
| __esModule: true, | ||
| default: ({ name }: { name: string }) => ( | ||
| <span data-testid={`icon-${name}`}>{name}</span> | ||
| ), | ||
| })); | ||
|
|
||
| jest.mock("@/components/ui/button", () => ({ | ||
| Button: ({ | ||
| children, | ||
| "aria-label": ariaLabel, | ||
| "data-testid": testId, | ||
| unstyled, | ||
| ...props | ||
| }: any) => ( | ||
| <button aria-label={ariaLabel} data-testid={testId} {...props}> | ||
| {children} | ||
| </button> | ||
| ), | ||
| })); | ||
|
|
||
| jest.mock("@/components/ui/separator", () => ({ | ||
| Separator: () => <hr />, | ||
| })); | ||
|
|
||
| jest.mock("@/components/common/shadTooltipComponent", () => ({ | ||
| __esModule: true, | ||
| default: ({ children }: any) => <>{children}</>, | ||
| })); | ||
|
|
||
| jest.mock("@/components/common/modelProviderCountComponent", () => ({ | ||
| __esModule: true, | ||
| default: () => null, | ||
| })); | ||
|
|
||
| jest.mock("@/alerts/alertDropDown", () => ({ | ||
| __esModule: true, | ||
| default: ({ children }: any) => <>{children}</>, | ||
| })); | ||
|
|
||
| jest.mock("@/assets/LangflowLogo.svg?react", () => ({ | ||
| __esModule: true, | ||
| default: () => <svg data-testid="langflow-logo" />, | ||
| })); | ||
|
|
||
| jest.mock("@/customization/components/custom-AccountMenu", () => ({ | ||
| __esModule: true, | ||
| default: () => null, | ||
| })); | ||
|
|
||
| jest.mock("@/customization/components/custom-langflow-counts", () => ({ | ||
| __esModule: true, | ||
| default: () => null, | ||
| })); | ||
|
|
||
| jest.mock("@/customization/components/custom-org-selector", () => ({ | ||
| CustomOrgSelector: () => null, | ||
| })); | ||
|
|
||
| jest.mock("@/customization/hooks/use-custom-navigate", () => ({ | ||
| useCustomNavigate: () => jest.fn(), | ||
| })); | ||
|
|
||
| jest.mock("@/customization/hooks/use-custom-theme", () => ({ | ||
| __esModule: true, | ||
| default: () => ({ dark: false }), | ||
| })); | ||
|
|
||
| jest.mock("@/stores/alertStore", () => ({ | ||
| __esModule: true, | ||
| default: () => ({ | ||
| notificationCenter: false, | ||
| setNotificationCenter: jest.fn(), | ||
| }), | ||
| })); | ||
|
|
||
| jest.mock("./components/FlowMenu", () => ({ FlowMenu: () => null }), { | ||
| virtual: true, | ||
| }); | ||
| jest.mock("../components/FlowMenu", () => ({ | ||
| __esModule: true, | ||
| default: () => null, | ||
| })); | ||
|
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. Test is over-mocked and diverges from the frontend testing pattern This suite mocks most of Please keep unit mocking minimal (external boundaries only) and add/port coverage to a Playwright test that asserts accessible names on the rendered UI. As per coding guidelines, “Frontend test files should follow the naming convention *.test.ts or *.test.tsx using Playwright” and “Warn when mocks are used instead of testing real behavior and interactions … Recommend integration tests when unit tests become overly mocked.” 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| describe("AppHeader — accessible button names", () => { | ||
| beforeEach(() => { | ||
| render(<AppHeader />); | ||
| }); | ||
|
|
||
| it("home/back button has an aria-label", () => { | ||
| const btn = screen.getByTestId("icon-ChevronLeft"); | ||
| expect(btn.closest("button")).toHaveAttribute("aria-label", "header.home"); | ||
| }); | ||
|
|
||
| it("notification bell button has an aria-label", () => { | ||
| const btn = screen.getByTestId("notification_button"); | ||
| expect(btn).toHaveAttribute("aria-label", "header.notifications"); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,7 @@ export default function AppHeader(): JSX.Element { | |
| onClick={() => navigate("/")} | ||
| className="mr-1 flex h-8 w-8 items-center" | ||
| data-testid="icon-ChevronLeft" | ||
| aria-label={t("header.home")} | ||
| > | ||
| <LangflowLogo className="h-5 w-5" /> | ||
| </Button> | ||
|
|
@@ -100,6 +101,7 @@ export default function AppHeader(): JSX.Element { | |
| ) | ||
| } | ||
| data-testid="notification_button" | ||
| aria-label={t("header.notifications")} | ||
|
Collaborator
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. bug: duplicate Suggested change: remove this duplicate and keep the existing |
||
| > | ||
| <div className="hit-area-hover group relative items-center rounded-md px-2 py-2 text-muted-foreground"> | ||
| <span className={getNotificationBadge()} /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| import { render, screen } from "@testing-library/react"; | ||
| import { AddFolderButton } from "../add-folder-button"; | ||
|
|
||
| jest.mock("react-i18next", () => ({ | ||
| useTranslation: () => ({ t: (key: string) => key }), | ||
| })); | ||
|
|
||
| jest.mock("@/components/common/genericIconComponent", () => ({ | ||
| __esModule: true, | ||
| default: ({ name }: { name: string }) => ( | ||
| <span data-testid={`icon-${name}`}>{name}</span> | ||
| ), | ||
| })); | ||
|
|
||
| jest.mock("@/components/common/shadTooltipComponent", () => ({ | ||
| __esModule: true, | ||
| default: ({ children }: { children: React.ReactNode }) => <>{children}</>, | ||
| })); | ||
|
|
||
| jest.mock("@/components/ui/button", () => ({ | ||
| Button: ({ | ||
| children, | ||
| "aria-label": ariaLabel, | ||
| disabled, | ||
| loading, | ||
| ...props | ||
| }: any) => ( | ||
| <button | ||
| aria-label={ariaLabel} | ||
| disabled={disabled} | ||
| data-loading={loading} | ||
| {...props} | ||
| > | ||
| {children} | ||
| </button> | ||
| ), | ||
| })); | ||
|
|
||
| describe("AddFolderButton", () => { | ||
| const defaultProps = { onClick: jest.fn(), disabled: false, loading: false }; | ||
|
|
||
| it("renders the add project button", () => { | ||
| render(<AddFolderButton {...defaultProps} />); | ||
| expect(screen.getByTestId("add-project-button")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("has an aria-label for screen readers", () => { | ||
| render(<AddFolderButton {...defaultProps} />); | ||
| const btn = screen.getByTestId("add-project-button"); | ||
| expect(btn).toHaveAttribute("aria-label", "folder.createNewProject"); | ||
| }); | ||
|
|
||
| it("is disabled when disabled prop is true", () => { | ||
| render(<AddFolderButton {...defaultProps} disabled />); | ||
| expect(screen.getByTestId("add-project-button")).toBeDisabled(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import { render, screen } from "@testing-library/react"; | ||
| import { UploadFolderButton } from "../upload-folder-button"; | ||
|
|
||
| jest.mock("react-i18next", () => ({ | ||
| useTranslation: () => ({ t: (key: string) => key }), | ||
| })); | ||
|
|
||
| jest.mock("@/components/common/genericIconComponent", () => ({ | ||
| __esModule: true, | ||
| default: ({ name }: { name: string }) => ( | ||
| <span data-testid={`icon-${name}`}>{name}</span> | ||
| ), | ||
| })); | ||
|
|
||
| jest.mock("@/components/common/shadTooltipComponent", () => ({ | ||
| __esModule: true, | ||
| default: ({ children }: { children: React.ReactNode }) => <>{children}</>, | ||
| })); | ||
|
|
||
| jest.mock("@/components/ui/button", () => ({ | ||
| Button: ({ children, "aria-label": ariaLabel, disabled, ...props }: any) => ( | ||
| <button aria-label={ariaLabel} disabled={disabled} {...props}> | ||
| {children} | ||
| </button> | ||
| ), | ||
| })); | ||
|
Comment on lines
+4
to
+33
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. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify frontend test framework usage and mock density (read-only).
# Expected: if guideline is enforced, frontend *.test.ts/tsx should primarily use Playwright APIs.
echo "== Frontend test files =="
fd -e ts -e tsx '.test$' src/frontend/src
echo
echo "== Files importing Jest/RTL in frontend tests =="
rg -n --type=ts --type=tsx '(`@testing-library/react`|jest\.mock|describe\(|it\()' src/frontend/src --iglob '**/*.test.ts' --iglob '**/*.test.tsx'
echo
echo "== Files importing Playwright in frontend tests =="
rg -n --type=ts --type=tsx '(`@playwright/test`|test\(|expect\()' src/frontend/src --iglob '**/*.test.ts' --iglob '**/*.test.tsx'Repository: langflow-ai/langflow Length of output: 266 🏁 Script executed: #!/bin/bash
# Find all test files in frontend with correct syntax
echo "== All test files in frontend =="
find src/frontend -name "*.test.ts" -o -name "*.test.tsx" 2>/dev/null | head -20
echo
echo "== Check specific file existence =="
ls -la "src/frontend/src/components/core/folderSidebarComponent/components/sideBarFolderButtons/components/__tests__/upload-folder-button.test.tsx" 2>&1
echo
echo "== Checking package.json for testing framework dependencies =="
cat package.json | grep -A 10 -B 2 '"jest"\|"playwright"\|"vitest"' 2>/dev/null || echo "Not found in top-level search"Repository: langflow-ai/langflow Length of output: 2229 🏁 Script executed: #!/bin/bash
# Search for test framework usage without type flags
echo "== Jest usage in frontend tests =="
grep -r "jest.mock\|describe(\|it(" src/frontend --include="*.test.ts*" 2>/dev/null | head -5
echo
echo "== Playwright usage in frontend tests =="
grep -r "`@playwright/test`\|test(" src/frontend --include="*.test.ts*" 2>/dev/null | head -5
echo
echo "== Looking for jest/vitest in package.json =="
grep -i "jest\|vitest\|playwright" package*.json 2>/dev/null | head -20Repository: langflow-ai/langflow Length of output: 1565 🏁 Script executed: #!/bin/bash
# Read package.json to check testing framework dependencies
echo "== Frontend package.json test dependencies =="
cat src/frontend/package.json | grep -A 30 '"devDependencies"' | head -40Repository: langflow-ai/langflow Length of output: 1197 🏁 Script executed: #!/bin/bash
# Read the specific test file to assess mock usage
echo "== Content of upload-folder-button.test.tsx =="
cat src/frontend/src/components/core/folderSidebarComponent/components/sideBarFolderButtons/components/__tests__/upload-folder-button.test.tsxRepository: langflow-ai/langflow Length of output: 1655 Reduce internal component mocks to test real interaction behavior. The test mocks internal UI components ( 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| describe("UploadFolderButton", () => { | ||
| const defaultProps = { onClick: jest.fn(), disabled: false }; | ||
|
|
||
| it("renders the upload project button", () => { | ||
| render(<UploadFolderButton {...defaultProps} />); | ||
| expect(screen.getByTestId("upload-project-button")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("has an aria-label for screen readers", () => { | ||
| render(<UploadFolderButton {...defaultProps} />); | ||
| const btn = screen.getByTestId("upload-project-button"); | ||
| expect(btn).toHaveAttribute("aria-label", "folder.uploadFlow"); | ||
| }); | ||
|
|
||
| it("is disabled when disabled prop is true", () => { | ||
| render(<UploadFolderButton {...defaultProps} disabled />); | ||
| expect(screen.getByTestId("upload-project-button")).toBeDisabled(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -477,6 +477,7 @@ | |
| "modal.api.exposeApiDescription": "Select which component fields to expose as inputs in this flow's API schema.", | ||
| "header.notifications": "Notifications and errors", | ||
| "header.notificationsLabel": "Notifications", | ||
| "header.home": "Go to home", | ||
| "account.version": "Version", | ||
| "account.latest": "(latest)", | ||
| "account.updateAvailable": "(update available)", | ||
|
|
@@ -707,7 +708,13 @@ | |
| "mainPage.timeElapsed.hour_other": "{{count}} hours", | ||
| "mainPage.timeElapsed.minute_one": "{{count}} minute", | ||
| "mainPage.timeElapsed.minute_other": "{{count}} minutes", | ||
| "flows.viewList": "List view", | ||
|
Collaborator
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. bug: these new a11y i18n keys were added only to Suggested change: add the same keys to all locale files, using English placeholders if translations are not ready. |
||
| "flows.viewGrid": "Grid view", | ||
| "flows.downloadSelected": "Download selected flows", | ||
| "flows.selectFlow": "Select {{name}}", | ||
| "flows.moreOptions": "More options for {{name}}", | ||
| "folder.options": "Options", | ||
| "folder.optionsFor": "Options for {{name}}", | ||
| "folder.rename": "Rename", | ||
| "folder.download": "Download", | ||
| "folder.delete": "Delete", | ||
|
|
@@ -1345,6 +1352,7 @@ | |
| "paginator.ofPages": "of {{maxIndex}} pages", | ||
| "paginator.previousPage": "Go to previous page", | ||
| "paginator.nextPage": "Go to next page", | ||
| "paginator.pageLabel": "Page", | ||
| "alerts.modelsRefreshed": "{{count}} model(s) refreshed", | ||
| "mcp.servers.toolsCount": "{{count}} tool(s)", | ||
| "misc.fetchErrorDescription": "Check if the server is running and try again.", | ||
|
|
||
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.
Use a semantic trigger mock that forwards props.
Suggested test-fidelity fix
As per coding guidelines: "Ensure mocks are used appropriately for external dependencies, not core logic" and "Warn when mocks are used instead of testing real behavior and interactions."
Also applies to: 70-76
🤖 Prompt for AI Agents
Source: Coding guidelines