fix(a11y): reduce jsx-a11y warnings — common 18→4, studio 45→20#446
Merged
marcusds merged 17 commits intoJun 26, 2026
Conversation
marcusds
force-pushed
the
astd-266-fix-critical-accessibility-issues-identified-by-jsx-a11y-lint/mschwab
branch
from
639c414 to
d34834e
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR lowers accessibility lint warning thresholds in the common and studio web packages, adds scripts to regenerate those thresholds, and updates multiple common and studio components and test mocks to use native buttons, hidden decorative elements, and labeled form controls. ChangesWeb accessibility and UI semantics
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
![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
🤖 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 `@web/packages/common/scripts/update-max-warnings-a11y.ts`:
- Around line 35-40: The update flow in update-max-warnings-a11y.ts assumes
lint:a11y always contains a --max-warnings value, so a changed script format can
silently skip replacement while still writing the package as if it were updated.
In the logic around maxWarningsRegex, currentMax, and the replace on
pkg.scripts['lint:a11y'], add a guard that verifies the regex matches before
attempting to rewrite; if it does not, fail fast with an explicit error instead
of continuing. Keep the existing warningCount comparison and only write pkgPath
after a successful, verified replacement.
In `@web/packages/studio/src/components/FileTag/index.tsx`:
- Around line 67-73: The FileTag button currently renders as an active,
focusable control even when onNoFileClick is missing. Update the button in
FileTag to disable it when no handler is provided by using
disabled={!onNoFileClick}, and add aria-disabled if needed for accessibility.
Keep the existing onClick behavior tied to onNoFileClick so the control only
appears interactive when it actually does something.
🪄 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: Enterprise
Run ID: 7e363d4a-6d24-4525-be44-95706f8b9b49
📒 Files selected for processing (16)
web/packages/common/package.jsonweb/packages/common/scripts/update-max-warnings-a11y.tsweb/packages/common/src/components/DataView/StudioAppliedFilters.test.tsxweb/packages/common/src/components/DataView/StudioDataView.test.tsxweb/packages/common/src/components/DataView/useRowClick.tsxweb/packages/common/src/components/Nebula/index.tsxweb/packages/common/src/components/StatusBadge/index.tsxweb/packages/common/src/components/TableEmptyState/TableEmptyState.test.tsxweb/packages/common/src/components/UploadModal/SimpleFilesTable.tsxweb/packages/common/src/components/form/ControlledSearchableSelect/index.test.tsxweb/packages/studio/package.jsonweb/packages/studio/src/components/FileTag/index.tsxweb/packages/studio/src/components/FilesTable/index.tsxweb/packages/studio/src/components/NewCustomizationForm/index.tsxweb/packages/studio/src/routes/SafeSynthesizerNewRoute/components/AdvancedParameters.tsxweb/packages/studio/src/tests/util/mockStudioDataView.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/packages/studio/src/components/ReportTraceModal/index.tsx (1)
226-262: 🩺 Stability & Availability | 🟠 MajorMove the Copy action out of the row
<button>. The nested<Button>inside the outer<button>is invalid HTML and breaks keyboard/screen-reader semantics. Use a non-button wrapper with explicit keyboard handling, or place Copy outside the clickable row.🤖 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 `@web/packages/studio/src/components/ReportTraceModal/index.tsx` around lines 226 - 262, The trace row in ReportTraceModal is using a nested interactive control: the inner Copy <Button> sits inside the outer row <button>, which creates invalid semantics and accessibility issues. Update the trace item rendering in the trace list so the row click target and the copy action are separate controls; keep handleViewDetails on the row container, and move handleCopyTrace to a standalone action outside that button or replace the row button with a non-button wrapper that has explicit keyboard support. Use the ReportTraceModal trace item markup, handleViewDetails, and handleCopyTrace to locate and adjust this interaction.
🤖 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 `@web/packages/studio/src/components/FilesTable/index.tsx`:
- Around line 79-83: Quick-action clicks are still bubbling up and triggering
the row action, so opening or using the file/directory quick actions can select
or navigate the row. Update the quick-actions handling in FileQuickActions,
DirectoryQuickActions, or QuickActionsMenuRoot so click propagation is stopped
within the actions/menu control itself rather than relying on the row cell
wrapper, and keep the row activation logic in the table row unchanged.
In
`@web/packages/studio/src/components/SafeSynthesizerFilesetPreview/FilesetFilePreviewLink.tsx`:
- Around line 135-141: The preview button in FilesetFilePreviewLink can render
without an accessible label when children is missing, so update the Button
content to fall back to the formatted file path used by the component. Use the
existing FilesetFilePreviewLink rendering logic and its path-formatting
helper/state to ensure the button always shows a label, with children taking
precedence only when provided.
---
Outside diff comments:
In `@web/packages/studio/src/components/ReportTraceModal/index.tsx`:
- Around line 226-262: The trace row in ReportTraceModal is using a nested
interactive control: the inner Copy <Button> sits inside the outer row <button>,
which creates invalid semantics and accessibility issues. Update the trace item
rendering in the trace list so the row click target and the copy action are
separate controls; keep handleViewDetails on the row container, and move
handleCopyTrace to a standalone action outside that button or replace the row
button with a non-button wrapper that has explicit keyboard support. Use the
ReportTraceModal trace item markup, handleViewDetails, and handleCopyTrace to
locate and adjust this interaction.
🪄 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: Enterprise
Run ID: e05bd2d8-925f-4156-93c4-7622bf224f0f
📒 Files selected for processing (9)
web/packages/common/package.jsonweb/packages/common/src/components/DataView/StudioDataView.test.tsxweb/packages/studio/package.jsonweb/packages/studio/src/components/FilesTable/index.tsxweb/packages/studio/src/components/PromptTuningForm/ToolsSection/components/AddToolForm.tsxweb/packages/studio/src/components/ReportTraceModal/index.tsxweb/packages/studio/src/components/SafeSynthesizerFilesetPreview/FilesetFilePreviewLink.tsxweb/packages/studio/src/routes/FilesetDetailRoute/DatasetSchemaEditor/index.test.tsxweb/packages/studio/src/tests/util/mockStudioDataView.tsx
✅ Files skipped from review due to trivial changes (2)
- web/packages/studio/src/routes/FilesetDetailRoute/DatasetSchemaEditor/index.test.tsx
- web/packages/studio/src/components/PromptTuningForm/ToolsSection/components/AddToolForm.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/packages/studio/src/tests/util/mockStudioDataView.tsx
- web/packages/common/package.json
marcusds
changed the title
marcusds
force-pushed
the
astd-266-fix-critical-accessibility-issues-identified-by-jsx-a11y-lint/mschwab
branch
from
2e837af to
822e80f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@web/packages/common/scripts/update-max-warnings-a11y.ts`:
- Around line 11-16: The update-max-warnings-a11y script assumes pkg.scripts
always exists before checking lint:a11y, so a package without a scripts object
will throw early. In update-max-warnings-a11y.ts, adjust the parsed pkg shape to
make scripts optional and guard pkg.scripts before indexing it for lint:a11y,
keeping the existing explicit error handling path in place.
🪄 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: Enterprise
Run ID: 0ab98285-2d38-4522-bbc9-f52dda189f63
📒 Files selected for processing (1)
web/packages/common/scripts/update-max-warnings-a11y.ts
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Created PR with unit tests: #482 |
nakolean
approved these changes
Jun 26, 2026
- 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>
- 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>
- 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>
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>
- 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>
- 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>
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>
- 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>
Signed-off-by: mschwab <mschwab@nvidia.com>
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>
…Badge 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>
- 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>
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>
Signed-off-by: mschwab <mschwab@nvidia.com>
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>
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>
marcusds
force-pushed
the
astd-266-fix-critical-accessibility-issues-identified-by-jsx-a11y-lint/mschwab
branch
from
dc6f271 to
7f1cab8
Compare
…dentified-by-jsx-a11y-lint/mschwab
marcusds
deleted the
astd-266-fix-critical-accessibility-issues-identified-by-jsx-a11y-lint/mschwab
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix accessibility issues flagged by the
jsx-a11yeslint plugin (added in #431).Warning reduction (
lint:a11yceilings):Key changes:
role="button"span → native<button>for theuseRowClicksr-only keyboard target; KUI<Button>primitive (not a raw<button>) for clickable regions in Studio (ReportTraceModal,FileTag) so KUI focus/styling tokens are preserved.role="article"div →<article>(IntakeAnnotationsPanel).role="img"→aria-hiddenon icons (StatusBadge);aria-hidden+tabIndex={-1}on the canvas (Nebula).aria-labelledby(AdvancedParameters).SimpleFilesTable"Upload More Files":<Button onClick>driving auseRefto the hidden file input (witharia-label;tabIndex={-1}so the hidden input isn't a duplicate tab stop).role="form"; gave the form an accessible name viaaria-labelinstead (NewCustomizationForm).autoFocus(CustomFilesetForm); intentional modal autofocus kept.eslint.config.a11y.js): enable only non-deprecatedjsx-a11yrules —accessible-emojiandno-onchangeare deprecated in jsx-a11y 6.10.2 (latest) and produce false positives.update-max-warnings-a11y.ts(fails fast when--max-warningsis absent) to keep the warning baselines in sync.aria-hiddenicons (StudioDataView,StatusBadge,SimpleFilesTable,create-a-customization).Remaining warnings are intentional (modal autofocus) or legitimate ARIA on custom widgets (
listbox/option/dialog/separator), where converting to native tags would regress behavior.Verification
Fixes ASTD-266
Summary by CodeRabbit