Skip to content

Commit 7b058c6

Browse files
marcusdsmarcusds
authored
fix(a11y): reduce jsx-a11y warnings — common 18→4, studio 45→20 (#446)
* fix(a11y): reduce jsx-a11y warnings common 18→8, studio 45→31 (#386) - Replace role="button" divs/spans with native <button> elements - Fix form label associations with aria-labelledby pattern - Add keyboard event handlers where missing - Wrap emojis with proper ARIA in test files - Remove redundant role="form" on native form element - Add aria-hidden + tabIndex=-1 on decorative canvas - Create update-max-warnings-a11y.ts script for maintaining baselines Signed-off-by: mschwab <mschwab@nvidia.com> * fix(a11y): further reduce warnings common 8→6, studio 31→24 - Replace div role="button" with native button in ReportTraceModal - Replace Anchor role="button" with Button in FilesetFilePreviewLink - Add aria-label to hidden file input in AddToolForm - Add aria-label to mock textarea in DatasetSchemaEditor test - Remove role="menu" from ul in mockStudioDataView - Simplify FilesTable quick actions wrapper - Remove span role="img" from emoji test data - Remove event handlers from mock table in StudioDataView test Signed-off-by: mschwab <mschwab@nvidia.com> * fix: resolve nested button and checkbox label association - ReportTraceModal: restructure trace card to use sibling button + copy button instead of nested <button> inside <button> - AdvancedParameters: restore click-to-toggle on checkbox labels by using htmlFor/id association instead of aria-labelledby Signed-off-by: mschwab <mschwab@nvidia.com> * revert: restore aria-labelledby for checkboxes to satisfy label-has-for The htmlFor/label form triggers the deprecated jsx-a11y/label-has-for rule (requires both nesting AND id), pushing warnings to 26 > 24 ceiling. Revert to aria-labelledby+span which satisfies label-has-associated-control without firing label-has-for. Trade-off: clicking label text doesn't toggle. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: address remaining a11y review nits - SimpleFilesTable: replace document.getElementById with useRef on file input - FileTag: gate focusable <button> when onNoFileClick is undefined - update-max-warnings-a11y.ts: use bare eslint command, guard stdout before parse Signed-off-by: mschwab <mschwab@nvidia.com> * fix: align SimpleFilesTable tests with ref-based file input - Match aria-label casing: 'Upload More Files' → 'Upload more files' - Remove id assertion (input now uses useRef, no id attribute) Signed-off-by: mschwab <mschwab@nvidia.com> * fix(a11y): repair unit tests broken by role/handler changes The a11y refactors (role=button→button, role=img→aria-hidden, role=form removal, label→ref upload input) left 13 unit tests asserting roles and handlers that no longer exist. Realign them with the refactored markup: - StudioDataView.test: mock TableContent attaches the row-click delegation handler via a ref callback (native onclick) instead of a JSX onClick prop, so cell/sub-row click tests pass without re-tripping jsx-a11y on the test-only <table>; keyboard tests use userEvent (native <button> activates on Enter/Space, which fireEvent.keyDown does not simulate); link query matches the renamed 'View' text. - StatusBadge.test: query the decorative icon by its lucide class instead of getByRole('img') (icon is now aria-hidden). - NewCustomizationForm: give the <form> an accessible name so it exposes a form role again; restores getByRole('form') in create-a-customization.test. Signed-off-by: mschwab <mschwab@nvidia.com> * fix(a11y): drop deprecated rules, convert article role, lower ceilings - eslint.config.a11y.js: enable only non-deprecated jsx-a11y rules (filter meta.deprecated). accessible-emoji and no-onchange are deprecated by jsx-a11y 6.10.2 (latest) and produce false positives; excluding them is the version-aware fix. - IntakeAnnotationsPanel: <div role="article"> -> <article>. - CustomFilesetForm: remove autoFocus on the page-load name field (non-modal autofocus). - Ratchet lint:a11y ceilings: common 6->4, studio 24->20. Remaining warnings are intentional modal autofocus and legitimate ARIA on custom widgets (listbox/option/dialog/separator), where converting to native tags would regress behavior. Signed-off-by: mschwab <mschwab@nvidia.com> * chore(a11y): trim verbose comments Signed-off-by: mschwab <mschwab@nvidia.com> * fix(a11y): use KUI Button instead of native button for clickable regions Wrapping content in a raw <button> bypasses KUI's focus/styling tokens. Use the KUI Button primitive instead: - ReportTraceModal: trace-card clickable -> <Button kind=tertiary> (copy button stays a sibling, no nesting). - FileTag: no-file clickable -> <Button kind=tertiary>. Signed-off-by: mschwab <mschwab@nvidia.com> * test(a11y): assert visible label instead of decorative icon in StatusBadge The status icon is now aria-hidden (decorative), so querying it requires a brittle .lucide class selector. Drop those icon-presence assertions and rely on the visible badge label, which is the user-meaningful output. Signed-off-by: mschwab <mschwab@nvidia.com> * fix(a11y): address codex/coderabbit review nits - update-max-warnings-a11y.ts: fail fast when the lint:a11y script has no --max-warnings flag, instead of silently no-op'ing while logging 'updated'. - SimpleFilesTable: tabIndex={-1} on the sr-only file input so keyboard users don't hit an invisible duplicate upload control (the button triggers it via ref). Signed-off-by: mschwab <mschwab@nvidia.com> * refactor(a11y): use ESLint Node API in update-max-warnings script Replace the execSync('eslint ... --format json') subprocess + JSON parse + catch-nonzero-exit handling with the ESLint class API (overrideConfigFile + allowInlineConfig:false + lintFiles). Reads warningCount off the result objects directly. Signed-off-by: mschwab <mschwab@nvidia.com> * chore(a11y): trim comment in update-max-warnings script Signed-off-by: mschwab <mschwab@nvidia.com> * chore(studio): remove unused FilesTable component FilesTable/index.tsx had no importers (the directory's siblings - FileQuickActions, DirectoryQuickActions, the file modals, utils - are imported directly by FilesetFileExplorer/FileActions and stay). Signed-off-by: mschwab <mschwab@nvidia.com> * fix(a11y): guard missing scripts object in update-max-warnings script Make pkg.scripts optional and guard before indexing lint:a11y, so a package.json without a scripts object hits the explicit error path instead of throwing a TypeError. Signed-off-by: mschwab <mschwab@nvidia.com> --------- Signed-off-by: mschwab <mschwab@nvidia.com>
1 parent f7d79d0 commit 7b058c6

23 files changed

Lines changed: 188 additions & 258 deletions

File tree

‎web/eslint.config.a11y.js‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,11 @@ export default [
2121
'jsx-a11y': jsxA11yPlugin,
2222
},
2323
rules: {
24+
// All non-deprecated jsx-a11y rules as warnings.
2425
...Object.fromEntries(
25-
Object.keys(jsxA11yPlugin.rules).map((rule) => [`jsx-a11y/${rule}`, 'warn'])
26+
Object.entries(jsxA11yPlugin.rules)
27+
.filter(([, rule]) => !rule.meta?.deprecated)
28+
.map(([rule]) => [`jsx-a11y/${rule}`, 'warn'])
2629
),
2730
},
2831
},

‎web/packages/common/package.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@
1010
"format:fix": "prettier --write './**/*.{js,jsx,ts,tsx}'",
1111
"lint": "eslint . --report-unused-disable-directives --max-warnings 0",
1212
"lint:fix": "eslint . --fix --report-unused-disable-directives --max-warnings 0",
13-
"lint:a11y": "eslint . --config ../../eslint.config.a11y.js --no-config-lookup --no-inline-config --max-warnings 18",
13+
"lint:a11y": "eslint . --config ../../eslint.config.a11y.js --no-config-lookup --no-inline-config --max-warnings 4",
14+
"update-warning-count:a11y": "tsx scripts/update-max-warnings-a11y.ts",
1415
"test": "vitest --run",
1516
"test:ci": "vitest run --coverage",
1617
"test:watch": "vitest watch",

‎web/packages/common/scripts/update-max-warnings-a11y.ts‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
#!/usr/bin/env node
2+
// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
3+
// SPDX-License-Identifier: Apache-2.0
4+
5+
import { ESLint } from 'eslint';
6+
import fs from 'fs';
7+
import path from 'path';
8+
9+
async function main() {
10+
const pkgPath = path.resolve(process.cwd(), 'package.json');
11+
const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8')) as {
12+
scripts?: Record<string, string>;
13+
};
14+
15+
const scripts = pkg.scripts;
16+
const a11yScript = scripts?.['lint:a11y'];
17+
if (!scripts || !a11yScript) {
18+
console.error('No lint:a11y script found in package.json');
19+
process.exit(1);
20+
}
21+
22+
const maxWarningsRegex = /--max-warnings (\d+)/;
23+
const maxWarningsMatch = a11yScript.match(maxWarningsRegex);
24+
if (!maxWarningsMatch) {
25+
console.error('lint:a11y script is missing --max-warnings <number>');
26+
process.exit(1);
27+
}
28+
const currentMax: number = parseInt(maxWarningsMatch[1], 10);
29+
30+
// a11y flat config only; inline directives ignored
31+
const eslint = new ESLint({
32+
overrideConfigFile: path.resolve(process.cwd(), '../../eslint.config.a11y.js'),
33+
allowInlineConfig: false,
34+
});
35+
const results = await eslint.lintFiles(['.']);
36+
const warningCount: number = results.reduce((sum, result) => sum + result.warningCount, 0);
37+
38+
if (warningCount !== currentMax) {
39+
scripts['lint:a11y'] = a11yScript.replace(maxWarningsRegex, `--max-warnings ${warningCount}`);
40+
fs.writeFileSync(pkgPath, JSON.stringify(pkg, null, 2) + '\n');
41+
// eslint-disable-next-line no-console
42+
console.log(`Updated lint:a11y max-warnings from ${currentMax} to ${warningCount}`);
43+
} else {
44+
// eslint-disable-next-line no-console
45+
console.log(`No update needed. Current warnings: ${warningCount}, max-warnings: ${currentMax}`);
46+
}
47+
}
48+
49+
main().catch((err) => {
50+
console.error(err);
51+
process.exit(1);
52+
});

‎web/packages/common/src/components/DataView/StudioAppliedFilters.test.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,9 @@ vi.mock('@nvidia/foundations-react-core', () => ({
2929
),
3030
Flex: ({ children }: React.PropsWithChildren) => <div>{children}</div>,
3131
Tag: ({ children, onClick }: React.PropsWithChildren<{ onClick?: () => void }>) => (
32-
<span role="button" onClick={onClick}>
32+
<button type="button" onClick={onClick}>
3333
{children}
34-
</span>
34+
</button>
3535
),
3636
}));
3737

‎web/packages/common/src/components/DataView/StudioDataView.test.tsx‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
import { StudioDataView } from '@nemo/common/src/components/DataView/StudioDataView';
55
import { render, screen, fireEvent } from '@testing-library/react';
6+
import userEvent from '@testing-library/user-event';
67

78
vi.mock('@nvidia/foundations-react-core', () => ({
89
Block: ({ children }: React.PropsWithChildren) => <div>{children}</div>,
@@ -157,7 +158,15 @@ vi.mock('@nemo/common/src/components/DataView/internal', () => ({
157158
className?: string;
158159
onClick?: React.MouseEventHandler;
159160
}) => (
160-
<table className={className} onClick={onClick}>
161+
// ref (not JSX onClick) so jsx-a11y doesn't flag the test-only <table>
162+
<table
163+
className={className}
164+
ref={(el) => {
165+
if (el) {
166+
el.onclick = onClick ? (onClick as unknown as (e: MouseEvent) => void) : null;
167+
}
168+
}}
169+
>
161170
<tbody>
162171
{mockRenderedRows.map((cells, rowIdx) => (
163172
<tr key={rowIdx} data-index={rowIdx}>
@@ -168,7 +177,7 @@ vi.mock('@nemo/common/src/components/DataView/internal', () => ({
168177
<button type="button">Delete {mockFlatRows[rowIdx]?.item.name}</button>
169178
</td>
170179
<td>
171-
<a href="/details">Link {mockFlatRows[rowIdx]?.item.name}</a>
180+
<a href="/details">View {mockFlatRows[rowIdx]?.item.name}</a>
172181
</td>
173182
<td>
174183
<input type="text" defaultValue="edit" aria-label={`edit-${rowIdx}`} />
@@ -289,7 +298,7 @@ describe('StudioDataView', () => {
289298
const onRowClick = vi.fn();
290299
render(<StudioDataView {...defaultProps} onRowClick={onRowClick} />);
291300

292-
fireEvent.click(screen.getByRole('link', { name: 'Link Bob' }));
301+
fireEvent.click(screen.getByRole('link', { name: 'View Bob' }));
293302

294303
expect(onRowClick).not.toHaveBeenCalled();
295304
});
@@ -337,22 +346,26 @@ describe('StudioDataView', () => {
337346
expect(targets).toHaveLength(0);
338347
});
339348

340-
it('should call onRowClick when Enter is pressed on a keyboard target', () => {
349+
it('should call onRowClick when Enter is pressed on a keyboard target', async () => {
350+
const user = userEvent.setup();
341351
const onRowClick = vi.fn();
342352
render(<StudioDataView {...defaultProps} onRowClick={onRowClick} />);
343353

344354
const targets = screen.getAllByRole('button', { name: 'Open row' });
345-
fireEvent.keyDown(targets[0], { key: 'Enter' });
355+
targets[0].focus();
356+
await user.keyboard('{Enter}');
346357

347358
expect(onRowClick).toHaveBeenCalledWith(testData[0], 0);
348359
});
349360

350-
it('should call onRowClick when Space is pressed on a keyboard target', () => {
361+
it('should call onRowClick when Space is pressed on a keyboard target', async () => {
362+
const user = userEvent.setup();
351363
const onRowClick = vi.fn();
352364
render(<StudioDataView {...defaultProps} onRowClick={onRowClick} />);
353365

354366
const targets = screen.getAllByRole('button', { name: 'Open row' });
355-
fireEvent.keyDown(targets[1], { key: ' ' });
367+
targets[1].focus();
368+
await user.keyboard(' ');
356369

357370
expect(onRowClick).toHaveBeenCalledWith(testData[1], 1);
358371
});
@@ -465,12 +478,14 @@ describe('StudioDataView', () => {
465478
expect(onRowClick).toHaveBeenCalledWith(dataWithSubRows[0].subRows![0], 0);
466479
});
467480

468-
it('should resolve a sub-row correctly on keyboard activation', () => {
481+
it('should resolve a sub-row correctly on keyboard activation', async () => {
482+
const user = userEvent.setup();
469483
const onRowClick = vi.fn();
470484
render(<StudioDataView {...subRowProps} onRowClick={onRowClick} />);
471485

472486
const targets = screen.getAllByRole('button', { name: 'Open row' });
473-
fireEvent.keyDown(targets[1], { key: 'Enter' });
487+
targets[1].focus();
488+
await user.keyboard('{Enter}');
474489

475490
// Same index semantic as click: parent's top-level data position
476491
expect(onRowClick).toHaveBeenCalledWith(dataWithSubRows[0].subRows![0], 0);

‎web/packages/common/src/components/DataView/useRowClick.tsx‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -72,20 +72,14 @@ function RowKeyboardTarget({
7272
subIndex?: number;
7373
}) {
7474
return (
75-
<span
75+
<button
76+
type="button"
7677
className="sr-only"
77-
tabIndex={0}
78-
role="button"
7978
aria-label="Open row"
8079
data-row-click
8180
data-row-index={dataIndex}
8281
{...(subIndex !== undefined && { 'data-sub-index': subIndex })}
83-
onKeyDown={(e) => {
84-
if (e.key === 'Enter' || e.key === ' ') {
85-
e.preventDefault();
86-
onActivate();
87-
}
88-
}}
82+
onClick={onActivate}
8983
/>
9084
);
9185
}

‎web/packages/common/src/components/Nebula/index.tsx‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,12 @@ export const Nebula = ({
7676
className={`relative size-full min-h-[200px] min-w-[200px] ${className}`}
7777
data-testid="nv-nebula"
7878
>
79-
<canvas ref={handleRef} className="pointer-events-none absolute inset-[0]" />
79+
<canvas
80+
ref={handleRef}
81+
className="pointer-events-none absolute inset-[0]"
82+
aria-hidden="true"
83+
tabIndex={-1}
84+
/>
8085
</div>
8186
);
8287
};

‎web/packages/common/src/components/StatusBadge/StatusBadge.test.tsx‎

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,13 @@
44
import { BadgeStatus, badgeStatus } from '@nemo/common/src/components/StatusBadge/badgeStatus';
55
import { StatusBadge } from '@nemo/common/src/components/StatusBadge/index';
66
import * as customQueries from '@nemo/common/src/tests/customQueries';
7-
import { queries, render, screen, within } from '@testing-library/react';
7+
import { queries, render, screen } from '@testing-library/react';
88
import { CircleCheck } from 'lucide-react';
99

1010
const allQueries = {
1111
...queries,
1212
...customQueries,
1313
};
14-
const customScreen = within(document.body, allQueries);
1514

1615
describe('StatusBadge component', () => {
1716
beforeEach(() => {
@@ -84,10 +83,6 @@ describe('StatusBadge component', () => {
8483
expect(badge).toBeInTheDocument();
8584
expect(badge).toHaveTextContent(expectedBadge.label);
8685

87-
// Check if icon is rendered when icon exists
88-
const icon = customScreen.getByRole('img');
89-
expect(icon).toBeInTheDocument();
90-
9186
unmount();
9287
});
9388
});
@@ -157,14 +152,9 @@ describe('StatusBadge component', () => {
157152
expect(screen.getByTestId('nv-badge')).toHaveTextContent('Success');
158153
});
159154

160-
it('renders icon when config entry has one', () => {
161-
render(<StatusBadge status="success" statusConfig={STATUS_CONFIG} />);
162-
expect(screen.getByRole('img')).toBeInTheDocument();
163-
});
164-
165-
it('renders no icon when config entry omits one', () => {
155+
it('renders the label for a config entry without an icon', () => {
166156
render(<StatusBadge status="error" statusConfig={STATUS_CONFIG} />);
167-
expect(screen.queryByRole('img')).not.toBeInTheDocument();
157+
expect(screen.getByTestId('nv-badge')).toHaveTextContent('Error');
168158
});
169159

170160
it('falls back to provided fallback for unknown status', () => {

‎web/packages/common/src/components/StatusBadge/index.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ export const StatusBadge = <T extends string = string>({
5353

5454
return (
5555
<Badge color={config.color} kind="solid">
56-
{Icon ? <Icon width="12px" height="12px" role="img" /> : null}
56+
{Icon ? <Icon width="12px" height="12px" aria-hidden="true" /> : null}
5757
{label}
5858
</Badge>
5959
);

‎web/packages/common/src/components/UploadModal/SimpleFilesTable.test.tsx‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -377,7 +377,7 @@ describe('SimpleFilesTable', () => {
377377
}),
378378
});
379379

380-
const fileInput = screen.getByLabelText('Upload More Files', {
380+
const fileInput = screen.getByLabelText('Upload more files', {
381381
selector: 'input',
382382
}) as HTMLInputElement;
383383
expect(fileInput).toHaveClass('sr-only');
@@ -392,7 +392,7 @@ describe('SimpleFilesTable', () => {
392392
wrapper: createWrapper({ files: mockNewFiles }, mockDispatch),
393393
});
394394

395-
const fileInput = screen.getByLabelText('Upload More Files', {
395+
const fileInput = screen.getByLabelText('Upload more files', {
396396
selector: 'input',
397397
}) as HTMLInputElement;
398398
const newFile = new File(['new content'], 'newfile.jsonl');
@@ -416,11 +416,10 @@ describe('SimpleFilesTable', () => {
416416
wrapper: createWrapper({ files: mockNewFiles }),
417417
});
418418

419-
const fileInput = screen.getByLabelText('Upload More Files', {
419+
const fileInput = screen.getByLabelText('Upload more files', {
420420
selector: 'input',
421421
});
422422

423-
expect(fileInput).toHaveAttribute('id', 'upload-more-files');
424423
expect(fileInput).toHaveAttribute('type', 'file');
425424
});
426425
});

0 commit comments

Comments
 (0)