fix(a11y): WCAG 2.4.1 — add SkipLink component for keyboard bypass blocks#39240
fix(a11y): WCAG 2.4.1 — add SkipLink component for keyboard bypass blocks#39240Aitema-gmbh wants to merge 4 commits intoapache:masterfrom
Conversation
dosubot
Bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| } | ||
|
|
||
| const SkipLink: FC<SkipLinkProps> = ({ | ||
| targetId = 'main-content', |
There was a problem hiding this comment.
Suggestion: The default anchor target is main-content, but that ID does not exist in the current frontend/templates, so using this component without a custom targetId will not move focus to real main content. Point the default to an existing stable container ID so the skip link works out of the box. [logic error]
Severity Level: Major ⚠️
- ⚠️ Global "Skip to main content" link focus stays on link.
- ⚠️ WCAG 2.4.1 bypass blocks unmet in SPA layout.
- ⚠️ Keyboard users cannot reliably jump past navigation.| targetId = 'main-content', | |
| targetId = 'app', |
Steps of Reproduction ✅
1. Note that the SPA root element is defined in the server template at
`superset/templates/superset/spa.html:10` as `<div id="app" data-bootstrap="{{
bootstrap_data }}">`, and a repo-wide search (`Grep id=["']main-content["']`) shows no
element with `id="main-content"` anywhere in the codebase.
2. The React application mounts into this `#app` container from
`superset-frontend/src/views/index.tsx:6-14`, where `const appMountPoint =
document.getElementById('app');` and `ReactDOM.render(<App />, appMountPoint);` render
`App` into that root.
3. The global layout for all frontend routes is defined in
`superset-frontend/src/views/App.tsx:73-110`, which renders `<Menu />`, `<Layout>`, and
`<Layout.Content>` but does not assign `id="main-content"` (or any other matching ID) to
the main content container; there is currently no matching DOM node for the default
`targetId`.
4. When a developer uses the new `SkipLink` component with its default props anywhere
inside the SPA (for example, adding `<SkipLink />` at the top of the layout in `App.tsx`),
the click handler at `SkipLink.tsx:65-85` executes `document.getElementById(targetId)`
with `targetId = 'main-content'`, receives `null` (because no such element exists), and
falls into the `else` branch which only sets `window.location.hash = '#main-content'`. As
a result, pressing Enter on the "Skip to main content" link does not move focus to any
real main content container, so the skip link silently fails to fulfill its intended
bypass behavior in the current SPA structure unless callers always override `targetId` or
introduce a matching `id="main-content"` element.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Accessibility/SkipLink.tsx
**Line:** 62:62
**Comment:**
*Logic Error: The default anchor target is `main-content`, but that ID does not exist in the current frontend/templates, so using this component without a custom `targetId` will not move focus to real main content. Point the default to an existing stable container ID so the skip link works out of the box.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| export { default as SkipLink } from './SkipLink'; |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
SkipLink is exported but never rendered in any application shell or layout, so the "Skip to main content" link never appears on real pages and keyboard users cannot bypass navigation.
Suggestion: Render SkipLink at the top of the global layout(s) or app shell, before navigation, so it is the first tabbable element across SPA and menu-only entrypoints.
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #a778cb
Actionable Suggestions - 1
-
superset-frontend/src/components/Accessibility/SkipLink.tsx - 1
- Missing translation for default text · Line 63-63
Review Details
-
Files reviewed - 3 · Commit Range:
16d6282..806c8b2- superset-frontend/src/components/Accessibility/SkipLink.tsx
- superset-frontend/src/components/Accessibility/index.tsx
- superset-frontend/src/views/App.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
|
|
||
| const SkipLink: FC<SkipLinkProps> = ({ | ||
| targetId = 'main-content', | ||
| children = 'Skip to main content', |
There was a problem hiding this comment.
Import the translation function t from @superset-ui/core and change the default children value to t('Skip to main content') to enable internationalization.
Code Review Run #a778cb
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Address Bito code review finding: the hardcoded English default 'Skip to main content' bypassed i18n so non-English users saw untranslated link text. Pipe the default through t() from @superset-ui/core so it picks up the user's locale catalog. Notes on the other bot findings for this PR: - 'SkipLink exported but never rendered' (CodeAnt Architect) was already addressed in the follow-up commit that mounted <SkipLink /> in the App shell and added id='main-content' to Layout.Content. - Default targetId remains 'main-content' rather than 'app' because WCAG 2.4.1 Bypass Blocks specifically asks users to skip *past* the nav, which is what Layout.Content represents; #app is the SPA root above the nav.
|
Thanks @bito-code-review @codeant-ai-for-open-source — addressed in On the two other findings flagged by CodeAnt:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #25a350Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
This one's looking generally good to me, although you might want to review the bot feedback for any changes you would consider taking. Otherwise, I think I'm ready to improve, but there are a few little things that we might want to consider if there's a good way to improve things for sighted users as well:
|
There was a problem hiding this comment.
Pull request overview
Implements WCAG 2.4.1 (Bypass Blocks) in Superset’s frontend by introducing a global “Skip to main content” link and wiring it to the app’s main content container.
Changes:
- Add a new
SkipLinkcomponent undersrc/components/Accessibility/with focus management for the main content target. - Render
SkipLinknear the top of the app layout and addid="main-content"to the main content container. - Add an accessibility barrel export for
SkipLink.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| superset-frontend/src/views/App.tsx | Renders SkipLink and adds id="main-content" to the main content container. |
| superset-frontend/src/components/Accessibility/index.tsx | Exports SkipLink from the new Accessibility component folder. |
| superset-frontend/src/components/Accessibility/SkipLink.tsx | Implements the skip link UI + click/focus behavior and temporary tabindex management. |
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| export { default as SkipLink } from './SkipLink'; |
There was a problem hiding this comment.
src/components/Accessibility/index.tsx only re-exports a symbol and doesn’t contain JSX. In this codebase, barrel files without JSX are typically .ts (e.g., src/components/ListView/index.ts), so renaming this to index.ts would better match existing conventions and avoid implying JSX content.
| const el = document.getElementById(targetId); | ||
| if (el) { | ||
| // Temporarily set tabindex to allow programmatic focus, | ||
| // then remove it on blur so the element stays in the natural tab order |
There was a problem hiding this comment.
The inline comment says removing tabindex is needed to keep the element in the “natural tab order”, but tabindex="-1" is already excluded from sequential tab navigation. Consider rewording this comment to reflect the actual reason for removing the attribute (e.g., avoiding permanently changing the element’s focusability/markup) or keep the attribute permanently if that’s acceptable.
| // then remove it on blur so the element stays in the natural tab order | |
| // then remove it on blur to restore the element's original markup |
| const StyledSkipLink = styled.a` | ||
| position: absolute; | ||
| top: -100px; | ||
| left: 0; | ||
| background: ${({ theme }) => theme.colorPrimary}; | ||
| color: ${({ theme }) => theme.colorWhite}; |
There was a problem hiding this comment.
The skip link is hidden by positioning it at top: -100px, which can still leave it partially visible at high zoom / large font sizes (contradicts the requirement that it be invisible until focused). Consider hiding it with a transform-based approach (e.g., translateY(-100%)) or a standard visually-hidden pattern that guarantees it’s fully off-screen regardless of its computed height, then reveal on focus.
| const SkipLink: FC<SkipLinkProps> = ({ | ||
| targetId = 'main-content', | ||
| children = t('Skip to main content'), | ||
| }) => { | ||
| const handleClick = (e: MouseEvent<HTMLAnchorElement>) => { | ||
| e.preventDefault(); | ||
| const el = document.getElementById(targetId); | ||
| if (el) { | ||
| // Temporarily set tabindex to allow programmatic focus, | ||
| // then remove it on blur so the element stays in the natural tab order | ||
| const hadTabindex = el.hasAttribute('tabindex'); | ||
| if (!hadTabindex) { | ||
| el.setAttribute('tabindex', '-1'); | ||
| } | ||
| el.focus(); | ||
| if (!hadTabindex) { | ||
| el.addEventListener( | ||
| 'blur', | ||
| () => el.removeAttribute('tabindex'), | ||
| { once: true }, | ||
| ); | ||
| } | ||
| } else { | ||
| window.location.hash = `#${targetId}`; | ||
| } | ||
| }; |
There was a problem hiding this comment.
This new SkipLink component introduces focus management and hash navigation behavior, but there are no accompanying frontend unit tests. Adding tests (e.g., renders hidden until focus; clicking/Enter moves focus to the target and temporarily manages tabindex; falls back to hash when target is missing) will help prevent regressions.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39240 +/- ##
=======================================
Coverage 64.37% 64.38%
=======================================
Files 2550 2552 +2
Lines 132180 132213 +33
Branches 30661 30668 +7
=======================================
+ Hits 85096 85127 +31
- Misses 45599 45601 +2
Partials 1485 1485
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…and SkipLink tests Addresses three review polish points on PR apache#39240: - Add `<main>` landmark via role="main" on Layout.Content so the skip target has semantic meaning beyond just an id anchor. - Scope `outline: none` to `[tabindex='-1']:focus` on the same element so the programmatic-focus moment doesn't render a stray browser outline. The rule is intentionally tight: only fires when SkipLink has dynamically attached tabindex='-1', does not affect any other element. - Add `VisuallyHidden` reusable component (with `as` prop for h1, h2, etc.) in the Accessibility folder. This is the shared sr-only primitive that WCAG 2.4.6 (PR apache#39241) needs to deduplicate across pages. Tests added for both SkipLink and VisuallyHidden covering default behavior, custom props, focus management, and tabindex cleanup on blur.
…isuallyHidden Addresses six review concerns on PR apache#39241: - Bug: vertical filter bar rendered "Filters" h2 twice (once outside the ResizableSidebar wrapper, once inside renderChild before StickyPanel). Screen readers announced the heading twice. Removed the outer one and kept the inner one — that's the semantically correct location since it scopes to the FiltersPanel container. - DRY: replaced the SrOnlyH1 / SrOnlyH2 styled blocks copy-pasted into five files with the shared VisuallyHidden component (with `as` prop). Component lives in src/components/Accessibility/ alongside SkipLink. - ChartCreation no longer renders the title twice (sr-only h1 plus visible aria-hidden h2). Now uses a single visible h1 styled to match the previous h2 size, with the StyledContainer CSS rule updated from h2 to h1. - StyledContainer CSS audit: no other selectors target ChartCreation h3 anywhere in src — the previous h3 → h2 → h1 chain is contained. - Indentation in DashboardWrapper return cleaned up (no fragment wrapper needed after removing the duplicate h2). - Tests added for VisuallyHidden covering default rendering, custom `as`, prop forwarding, and the sr-only style invariant. This PR depends on apache#39240 introducing the same Accessibility folder; once that merges first, the duplicate component definition will rebase away.
There was a problem hiding this comment.
Code Review Agent Run #a27561
Actionable Suggestions - 1
-
superset-frontend/src/components/Accessibility/VisuallyHidden.test.tsx - 1
- Test assertion mismatch · Line 53-53
Review Details
-
Files reviewed - 5 · Commit Range:
73d91f5..70c7be1- superset-frontend/src/components/Accessibility/SkipLink.test.tsx
- superset-frontend/src/components/Accessibility/VisuallyHidden.test.tsx
- superset-frontend/src/components/Accessibility/VisuallyHidden.tsx
- superset-frontend/src/components/Accessibility/index.tsx
- superset-frontend/src/views/App.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| test('applies clip-rect style for screen-reader-only behavior', () => { | ||
| render(<VisuallyHidden>Hidden</VisuallyHidden>); | ||
| const el = screen.getByText('Hidden'); | ||
| expect(el).toHaveStyle({ position: 'absolute' }); |
There was a problem hiding this comment.
The test 'applies clip-rect style for screen-reader-only behavior' claims to verify clip-rect application but only asserts position: 'absolute'. The VisuallyHidden component uses clip: rect(0, 0, 0, 0) as the primary mechanism for visual hiding while maintaining accessibility. Update the assertion to check clip to match the test's intent and prevent potential regressions.
Code suggestion
Check the AI-generated fix before applying
| expect(el).toHaveStyle({ position: 'absolute' }); | |
| expect(el).toHaveStyle({ clip: 'rect(0, 0, 0, 0)' }); |
Code Review Run #a27561
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
SUMMARY
Implements WCAG 2.1 criterion 2.4.1 (Bypass Blocks, Level A).
SkipLinkcomponent insrc/components/Accessibility/tabindexon target for reliable focus managementTESTING INSTRUCTIONS
ADDITIONAL INFORMATION
Part of a series of 16 individual WCAG 2.1 accessibility PRs. See also fix(a11y): Accessibility improvements for WCAG 2.1 Level A compliance #38342.