codeant-ai-for-open-source[bot] commented on code in PR #37211: URL: https://github.com/apache/superset/pull/37211#discussion_r2701767972
########## superset-frontend/src/components/Accessibility/SkipLink.tsx: ########## @@ -0,0 +1,91 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import React from 'react'; +import { styled } from '@superset-ui/core'; + +/** + * SkipLink - WCAG 2.4.1 Bypass Blocks + * Allows keyboard users to skip navigation and jump directly to main content. + * The link is visually hidden but becomes visible when focused. + */ + +const StyledSkipLink = styled.a` + position: absolute; + top: -100px; + left: 0; + background: ${({ theme }) => theme.colors.primary.dark1}; + color: ${({ theme }) => theme.colors.grayscale.light5}; + padding: ${({ theme }) => theme.gridUnit * 3}px ${({ theme }) => theme.gridUnit * 4}px; + z-index: 10000; + text-decoration: none; + font-weight: ${({ theme }) => theme.typography.weights.bold}; + font-size: ${({ theme }) => theme.typography.sizes.m}px; + border-radius: 0 0 ${({ theme }) => theme.borderRadius}px ${({ theme }) => theme.borderRadius}px; + transition: top 0.2s ease-in-out; + + &:focus, + &:focus-visible { + top: 0 !important; + outline: 3px solid ${({ theme }) => theme.colors.primary.light1}; + outline-offset: 2px; + } + + &:hover { + background: ${({ theme }) => theme.colors.primary.base}; + } +`; + +interface SkipLinkProps { + targetId?: string; + children?: React.ReactNode; +} + +const SkipLink: React.FC<SkipLinkProps> = ({ + targetId = 'main-content', + children = 'Skip to main content', +}) => { + const handleClick = (e: React.MouseEvent<HTMLAnchorElement>) => { + e.preventDefault(); + const el = document.getElementById(targetId); + if (el) { + // Make sure the element is focusable and focus it + // Note: We intentionally keep the tabindex to ensure the element remains focusable + // for subsequent keyboard navigation (fixes skip link accessibility) + if (!el.hasAttribute('tabindex')) { + el.setAttribute('tabindex', '-1'); + } + el.focus(); Review Comment: **Suggestion:** Logic bug: the code sets a permanent `tabindex="-1"` (when absent) and leaves it on the target element — `tabindex="-1"` removes the element from the keyboard tab order, preventing users from reaching main content via Tab; instead temporarily add `tabindex="-1"` only to programmatically focus and then remove it (after focus) so normal keyboard navigation is preserved. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Skip link keyboard navigation fails reaching main content. - ⚠️ Keyboard users cannot access targeted content via Tab. - ⚠️ Breaks WCAG 2.4.1 bypass block compliance. ``` </details> ```suggestion // Temporarily add tabindex so we can programmatically focus, then remove it to preserve tab order const _addedTabIndex = !el.hasAttribute('tabindex'); if (_addedTabIndex) { el.setAttribute('tabindex', '-1'); } el.focus(); if (_addedTabIndex) { // Remove the temporary tabindex on the next macrotask to ensure focus is retained window.setTimeout(() => { if (document.body.contains(el)) { el.removeAttribute('tabindex'); } }, 0); } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Ensure the SkipLink component is present on a page (component defined at superset-frontend/src/components/Accessibility/SkipLink.tsx). The click handler starts at the code around lines 64-76; the relevant block is lines 67-73. 2. Add a target element without an existing tabindex, e.g. <main id="main-content">...</main>, and confirm no tabindex attribute is present. 3. Focus the SkipLink (Tab until it appears) and activate it (Enter or click). This calls handleClick() in SkipLink (see lines 64-76), which executes the existing lines 67-73. 4. After activation, the code sets tabindex="-1" on the target and leaves it. Observe that subsequent Tab presses do NOT reach the main content because tabindex="-1" removes it from tab order — reproducing the accessibility regression. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/components/Accessibility/SkipLink.tsx **Line:** 68:73 **Comment:** *Logic Error: Logic bug: the code sets a permanent `tabindex="-1"` (when absent) and leaves it on the target element — `tabindex="-1"` removes the element from the keyboard tab order, preventing users from reaching main content via Tab; instead temporarily add `tabindex="-1"` only to programmatically focus and then remove it (after focus) so normal keyboard navigation is preserved. 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. ``` </details> ########## superset-frontend/src/components/Accessibility/SkipLink.stories.tsx: ########## @@ -0,0 +1,240 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import SkipLink from './SkipLink'; + +export default { + title: 'Components/Accessibility/SkipLink', + component: SkipLink, + parameters: { + docs: { + description: { + component: + 'WCAG 2.4.1 Bypass Blocks - Skip link for keyboard navigation. ' + + 'Allows keyboard users to skip repetitive navigation and jump directly to main content. ' + + 'The link is visually hidden but becomes visible when focused via Tab key.', + }, + }, + }, +}; + +interface SkipLinkArgs { + targetId: string; + children: string; +} + +// Interactive story with controls +export const InteractiveSkipLink = (args: SkipLinkArgs) => ( + <div> + <SkipLink {...args} /> + <p style={{ marginTop: 20, color: '#666' }}> + Press Tab to see the skip link appear at the top of the viewport. + </p> + <div id={args.targetId} style={{ marginTop: 40, padding: 20, background: '#f0f0f0' }}> + <h2>Target Content</h2> + <p>This is the target element that will receive focus when the skip link is activated.</p> + </div> + </div> +); + +InteractiveSkipLink.args = { + targetId: 'main-content', + children: 'Skip to main content', +}; + +InteractiveSkipLink.argTypes = { + targetId: { + control: 'text', + description: 'ID of the target element to focus when activated', + table: { + defaultValue: { summary: 'main-content' }, + }, + }, + children: { + control: 'text', + description: 'The text displayed in the skip link', + table: { + defaultValue: { summary: 'Skip to main content' }, + }, + }, +}; + +// Full page demo showing realistic usage +export const FullPageDemo = () => ( + <div style={{ minHeight: '100vh' }}> + <SkipLink targetId="main-content" /> + + <header style={{ padding: 20, background: '#f8f8f8', borderBottom: '1px solid #ddd' }}> + <nav> + <a href="#" style={{ marginRight: 16 }}> + Home + </a> + <a href="#" style={{ marginRight: 16 }}> + Dashboards + </a> + <a href="#" style={{ marginRight: 16 }}> + Charts + </a> + <a href="#" style={{ marginRight: 16 }}> + SQL Lab + </a> + <a href="#">Settings</a> + </nav> + </header> + + <main + id="main-content" + tabIndex={-1} + style={{ padding: 40, outline: 'none' }} Review Comment: **Suggestion:** The `<main>` element explicitly sets `outline: 'none'`, which removes the browser focus indicator and contradicts the accessibility checklist (focus indicator should be visible). Removing the `outline: 'none'` (or replacing it with a visible focus style) ensures keyboard users can see focus when the skip link targets the main area. [possible bug] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Keyboard users cannot see focus on main content. - ⚠️ Accessibility test story fails visible focus check. - ⚠️ WCAG keyboard navigation validation impacted. ``` </details> ```suggestion style={{ padding: 40 }} ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the FullPageDemo story in superset-frontend/src/components/Accessibility/SkipLink.stories.tsx (FullPageDemo defined lines 79-117). The main target element is declared at lines 101-105 with tabIndex={-1} and style outline: 'none'. 2. Start Storybook and navigate to the FullPageDemo story. Press Tab until the skip link appears (as described in the story) and activate it (click or press Enter). 3. The SkipLink component should programmatically move focus to the target element with id="main-content" (the story documents this expectation). Because the main element has style outline: 'none' (lines 101-105), the browser's visible focus indicator will be suppressed. 4. Observe that after activation the target receives focus but there is no visible focus ring, failing the story's own checklist (visible focus indicator) and making keyboard users unable to perceive focus location. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/components/Accessibility/SkipLink.stories.tsx **Line:** 104:104 **Comment:** *Possible Bug: The `<main>` element explicitly sets `outline: 'none'`, which removes the browser focus indicator and contradicts the accessibility checklist (focus indicator should be visible). Removing the `outline: 'none'` (or replacing it with a visible focus style) ensures keyboard users can see focus when the skip link targets the main area. 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. ``` </details> ########## superset-frontend/src/components/Accessibility/SkipLink.test.tsx: ########## @@ -0,0 +1,345 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import userEvent from '@testing-library/user-event'; +import { render, screen, waitFor } from 'spec/helpers/testing-library'; +import SkipLink from './SkipLink'; + +describe('SkipLink', () => { + beforeEach(() => { + // Clean up any target elements from previous tests + const existingTarget = document.getElementById('main-content'); + if (existingTarget) { + existingTarget.remove(); + } + }); + + describe('Rendering', () => { + test('renders with default "Skip to main content" text', () => { + render(<SkipLink />); + expect(screen.getByText('Skip to main content')).toBeInTheDocument(); + }); + + test('renders with custom children text', () => { + render(<SkipLink>Skip to navigation</SkipLink>); + expect(screen.getByText('Skip to navigation')).toBeInTheDocument(); + }); + + test('renders as anchor element with correct href', () => { + render(<SkipLink targetId="custom-target" />); + const link = screen.getByRole('link'); + expect(link).toHaveAttribute('href', '#custom-target'); + }); + + test('applies styled-component class', () => { + render(<SkipLink />); + const link = screen.getByRole('link'); + expect(link).toHaveClass('a11y-skip-link'); + }); + }); + + describe('Visual States', () => { + test('is positioned off-screen by default (top: -100px)', () => { + render(<SkipLink />); + const link = screen.getByRole('link'); + expect(link).toHaveStyleRule('top', '-100px'); + }); + + test('has correct z-index for overlay (10000)', () => { + render(<SkipLink />); + const link = screen.getByRole('link'); + expect(link).toHaveStyleRule('z-index', '10000'); + }); + + test('has absolute positioning', () => { + render(<SkipLink />); + const link = screen.getByRole('link'); + expect(link).toHaveStyleRule('position', 'absolute'); + }); + }); + + describe('Focus Behavior', () => { + test('becomes visible when focused (top: 0)', async () => { + render(<SkipLink />); + const link = screen.getByRole('link'); + link.focus(); + expect(link).toHaveStyleRule('top', '0 !important', { + modifier: ':focus', + }); + }); + + test('receives focus on Tab key press', async () => { + const user = userEvent.setup(); + render(<SkipLink />); + await user.tab(); + const link = screen.getByRole('link'); + expect(link).toHaveFocus(); + }); + + test('shows focus outline styling', () => { + render(<SkipLink />); + const link = screen.getByRole('link'); + expect(link).toHaveStyleRule('outline', expect.stringContaining('3px solid'), { + modifier: ':focus', + }); + }); + + test('shows focus-visible outline styling', () => { + render(<SkipLink />); + const link = screen.getByRole('link'); + expect(link).toHaveStyleRule('top', '0 !important', { + modifier: ':focus-visible', + }); + }); + }); + + describe('Keyboard Navigation', () => { + test('activates on Enter key press', async () => { + const user = userEvent.setup(); + const targetElement = document.createElement('main'); + targetElement.id = 'main-content'; + document.body.appendChild(targetElement); + + render(<SkipLink />); + await user.tab(); + await user.keyboard('{Enter}'); + + await waitFor(() => { + expect(targetElement).toHaveFocus(); + }); + + targetElement.remove(); + }); + + test('activates on click', async () => { + const user = userEvent.setup(); + const targetElement = document.createElement('main'); + targetElement.id = 'main-content'; + document.body.appendChild(targetElement); + + render(<SkipLink />); + await user.click(screen.getByRole('link')); + + await waitFor(() => { + expect(targetElement).toHaveFocus(); + }); + + targetElement.remove(); + }); + }); + + describe('Click Handler', () => { + test('prevents default anchor behavior', async () => { + const user = userEvent.setup(); + const targetElement = document.createElement('main'); + targetElement.id = 'main-content'; + document.body.appendChild(targetElement); + + render(<SkipLink />); + const link = screen.getByRole('link'); + + const clickEvent = new MouseEvent('click', { bubbles: true }); + const preventDefaultSpy = jest.spyOn(clickEvent, 'preventDefault'); + + link.dispatchEvent(clickEvent); + expect(preventDefaultSpy).toHaveBeenCalled(); Review Comment: **Suggestion:** Spying on `preventDefault` on a native MouseEvent instance can fail in jsdom because the instance method may not be spyable; replace spying on the instance with spying on `Event.prototype.preventDefault` and restore the spy after the assertion to avoid test errors and global side effects. [possible bug] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ SkipLink test can fail during CI runs. - ⚠️ Local developer test flakiness while running tests. ``` </details> ```suggestion const preventDefaultSpy = jest.spyOn(Event.prototype, 'preventDefault'); link.dispatchEvent(clickEvent); expect(preventDefaultSpy).toHaveBeenCalled(); preventDefaultSpy.mockRestore(); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the SkipLink test file with the specific test: yarn test superset-frontend/src/components/Accessibility/SkipLink.test.tsx -t "prevents default anchor behavior". 2. Test executes the block in this file at superset-frontend/src/components/Accessibility/SkipLink.test.tsx lines 157-161 (inside test 'prevents default anchor behavior' which creates a MouseEvent and calls jest.spyOn on the instance). 3. In a jsdom environment (used by Jest), the instance method may not be spyable; jest.spyOn(clickEvent, 'preventDefault') can throw or not attach correctly, causing the test to error or give a false negative at the call on line 158. 4. Observed outcome: test fails with a spy-related error (cannot spy property / not a function) or flaky assertion. The proposed change (spy on Event.prototype and restore) avoids spying on a possibly non-configurable instance method. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/components/Accessibility/SkipLink.test.tsx **Line:** 158:161 **Comment:** *Possible Bug: Spying on `preventDefault` on a native MouseEvent instance can fail in jsdom because the instance method may not be spyable; replace spying on the instance with spying on `Event.prototype.preventDefault` and restore the spy after the assertion to avoid test errors and global side effects. 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. ``` </details> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
