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 2 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
1 participant
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.