fix(a11y): WCAG 2.4.7 — ensure visible keyboard focus on all interactive elements#39242
Open
Aitema-gmbh wants to merge 2 commits intoapache:masterfrom
Open
fix(a11y): WCAG 2.4.7 — ensure visible keyboard focus on all interactive elements#39242Aitema-gmbh wants to merge 2 commits intoapache:masterfrom
Aitema-gmbh wants to merge 2 commits intoapache:masterfrom
Conversation
…ive elements - Add global *:focus-visible baseline rule in GlobalStyles.tsx (2px solid colorPrimary) - Replace outline:none with outline:2px solid transparent in dashboard styles.ts - Fix focus visibility in RangeFilterPlugin, TimeFilterPlugin, ConditionalFormattingControl, VizTypeGallery - Fix focus visibility in FilterScopeSelector, WithPopoverMenu, FiltersBadge/Styles - Add :focus-visible box-shadow indicators where no alternative focus style existed Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Code Review Agent Run #8d889eActionable 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 |
dosubot
Bot
added
change:frontend
6 tasks
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+117
to
+118
| outline: 2px solid ${theme.colorPrimary}; | ||
| outline-offset: 2px; |
Contributor
There was a problem hiding this comment.
Suggestion: The global focus rule can be silently overridden by existing component styles that set outline: none on :focus with higher specificity (for example in InfoTooltip), so keyboard users will still see no visible focus in those places. Make the global rule win by default so it actually enforces a visible indicator across the app. [logic error]
Severity Level: Critical 🚨
- ❌ InfoTooltip icons lack visible focus across many screens.
- ❌ Database extra options modal fails WCAG focus-visible requirement.
- ❌ Global focus ring bypassed by outline-none component styles.
- ⚠️ Keyboard navigation harder in configuration and explore dialogs.
Suggested change
| outline: 2px solid ${theme.colorPrimary}; | |
| outline-offset: 2px; | |
| outline: 2px solid ${theme.colorPrimary} !important; | |
| outline-offset: 2px !important; |
Steps of Reproduction ✅
1. Start the Superset frontend with this PR's code, which defines the global focus rule in
`superset-frontend/packages/superset-core/src/theme/GlobalStyles.tsx:113-119`
(`*:focus-visible { outline: 2px solid ${theme.colorPrimary}; outline-offset: 2px; }`).
2. Open the "Database" modal and navigate to the Extra options step implemented in
`superset-frontend/src/features/databases/DatabaseModal/ExtraOptions.tsx:135-193`, where
several `<InfoTooltip />` icons are rendered next to checkboxes (e.g., lines 167-169 and
188-192).
3. Using only the keyboard, Tab forward until one of the InfoTooltip icons receives focus;
the underlying button is rendered by `InfoTooltip` in
`superset-frontend/packages/superset-ui-core/src/components/InfoTooltip/index.tsx:2-33`,
where the CSS block `&&&:hover, &&&:focus, &&&:active { … outline: none; … }` is defined
on lines 16-22.
4. While the button now matches both `:focus` and `:focus-visible`, the component rule
`&&&:focus { outline: none; }` (specificity with multiple `&` classes) overrides the
global `*:focus-visible { outline: 2px solid … }` rule (lower specificity), so the outline
remains `none` and no visible focus ring appears for keyboard users, confirming that the
global rule is too weak without `!important`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-core/src/theme/GlobalStyles.tsx
**Line:** 117:118
**Comment:**
*Logic Error: The global focus rule can be silently overridden by existing component styles that set `outline: none` on `:focus` with higher specificity (for example in `InfoTooltip`), so keyboard users will still see no visible focus in those places. Make the global rule win by default so it actually enforces a visible indicator across the app.
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.
| color: inherit; | ||
|
|
||
| &:focus-visible { | ||
| box-shadow: 0 0 0 2px ${theme.colorPrimary}; |
Contributor
There was a problem hiding this comment.
Suggestion: The focus ring is rendered with an outer box-shadow, but these buttons live inside a container with overflow-x: hidden, so the ring can be clipped at the left/right edges and become partially invisible. Use an inset focus ring so the indicator stays fully visible inside the element bounds. [possible bug]
Severity Level: Major ⚠️
- ⚠️ FiltersBadge popover focus ring clipped at horizontal edges.
- ⚠️ Cross-filter indicator keyboard focus less visually apparent.
Suggested change
| box-shadow: 0 0 0 2px ${theme.colorPrimary}; | |
| box-shadow: inset 0 0 0 2px ${theme.colorPrimary}; |
Steps of Reproduction ✅
1. Open any dashboard in the UI that uses the chart header `FiltersBadge` component (entry
point `superset-frontend/src/dashboard/components/SliceHeader/index.tsx:327` rendering
`<FiltersBadge chartId={slice.slice_id} />`).
2. Apply at least one cross-filter so that `appliedCrossFilterIndicators` is non-empty and
the filters popover content renders (`DetailsPanelPopover` in
`superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx:124–179`).
3. From the chart header, move keyboard focus to the filter count badge and press Enter to
open the popover (`StyledFilterCount` key handler in
`superset-frontend/src/dashboard/components/FiltersBadge/index.tsx:147–151` sets
`popoverVisible` which shows the `Popover` content containing `FiltersDetailsContainer`).
4. While focus is on the popover container (`FiltersDetailsContainer` in
`Styles.tsx:119–123` with `max-width: 300px` and `overflow-x: hidden`), press ArrowDown so
`DetailsPanelPopover`'s `handleKeyDown` (`DetailsPanel/index.tsx:62–91`) moves focus to a
`FilterIndicator` button (`FilterIndicator/index.tsx:40–46` renders `FilterItem`). The
`FilterItem` focus style (`Styles.tsx:87–90` `box-shadow: 0 0 0 2px
${theme.colorPrimary};`) draws a 2px outer ring that extends beyond the button's
horizontal bounds, and because an ancestor (`FiltersDetailsContainer`,
`Styles.tsx:121–123`) has `overflow-x: hidden`, the left/right portions of the focus ring
are clipped, making the keyboard focus indicator only partially visible at the edges.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx
**Line:** 88:88
**Comment:**
*Possible Bug: The focus ring is rendered with an outer `box-shadow`, but these buttons live inside a container with `overflow-x: hidden`, so the ring can be clipped at the left/right edges and become partially invisible. Use an inset focus ring so the indicator stays fully visible inside the element bounds.
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.
| ${({ theme }) => css` | ||
| position: relative; | ||
| outline: none; | ||
| outline: 2px solid transparent; /* WCAG 2.4.7: transparent outline prevents double-ring; :after pseudo-element provides visible focus border */ |
Contributor
There was a problem hiding this comment.
Suggestion: Applying a transparent outline unconditionally to the wrapper can make every dashboard item show a forced-color outline in Windows High Contrast Mode, even when not focused/selected. Scope the transparent outline to the focused state class so HC fallback appears only when this item is actually focused. [logic error]
Severity Level: Major ⚠️
- ⚠️ Dashboards show false focus in Windows High Contrast.
- ⚠️ Dashboard rows/columns look focused when they are not.
- ⚠️ Keyboard users may misidentify active dashboard element.
- ⚠️ Visual noise around all dashboard layout wrappers in HC.
Suggested change
| outline: 2px solid transparent; /* WCAG 2.4.7: transparent outline prevents double-ring; :after pseudo-element provides visible focus border */ | |
| &.with-popover-menu--focused { | |
| outline: 2px solid transparent; /* WCAG 2.4.7: transparent outline for forced-colors fallback only when focused */ | |
| } |
Steps of Reproduction ✅
1. Open any dashboard in edit mode so that grid layout components render (e.g., Row,
Column, Header). These components use `WithPopoverMenu` as a wrapper: `Row` at
`superset-frontend/src/dashboard/components/gridComponents/Row/Row.tsx:93-106`, `Column`
at `.../Column/Column.tsx:20-36`, and `Header` at `.../Header/Header.tsx:35-51`, each
rendering a `<WithPopoverMenu>` around the visible dashboard content.
2. In the rendered DOM, inspect one of these wrappers: it is the styled `<div>` created in
`WithPopoverMenuStyles` at
`superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx:11-34`, rendered with
class `"with-popover-menu"` in `render()` at lines `232-239`. Note that the styled
definition unconditionally applies `outline: 2px solid transparent;` (line 53 in the PR
hunk) regardless of the `isFocused` state.
3. Enable Windows High Contrast / forced-colors mode in the OS and view the dashboard in a
browser that honors forced colors (e.g., Edge). In this mode, the user agent replaces
transparent outline colors with a visible system highlight color, so any element with
`outline: 2px solid transparent` gets a visible outline even when it is not actually
focused.
4. Observe that all `WithPopoverMenu` wrappers (around rows, columns, and headers) now
display a visible outline in High Contrast mode even when `isFocused` is `false` and the
`"with-popover-menu--focused"` class is not present (see class toggle at
`WithPopoverMenu.tsx:36-39`). This creates false focus indicators on unfocused dashboard
components. If the outline were scoped as in the suggestion (`&.with-popover-menu--focused
{ outline: 2px solid transparent; }`), the forced-colors fallback outline would only
appear when the item is actually focused.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx
**Line:** 53:53
**Comment:**
*Logic Error: Applying a transparent outline unconditionally to the wrapper can make every dashboard item show a forced-color outline in Windows High Contrast Mode, even when not focused/selected. Scope the transparent outline to the focused state class so HC fallback appears only when this item is actually focused.
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.
Comment on lines
+214
to
+216
| &:focus-visible { | ||
| outline: 2px solid transparent; /* WCAG 2.4.7: transparent outline prevents double-ring */ | ||
| box-shadow: 0 0 0 2px ${theme.colorPrimary}; |
Contributor
There was a problem hiding this comment.
Suggestion: The focus style is attached to .rct-icon-* spans, but keyboard focus lands on the expand/collapse option button, not the inner icon span. As a result, the new :focus-visible rule never applies and keyboard users still won't see a focus indicator on these controls. Move the focus-visible selector to the focusable .rct-option and style the nested icon from there. [logic error]
Severity Level: Major ⚠️
- ❌ Keyboard focus indicator missing on Expand/Collapse-all scope controls.
- ⚠️ WCAG 2.4.7 partially unfulfilled for filter scope configuration.
- ⚠️ Keyboard dashboard users struggle configuring filter scopes accurately.
Suggested change
| &:focus-visible { | |
| outline: 2px solid transparent; /* WCAG 2.4.7: transparent outline prevents double-ring */ | |
| box-shadow: 0 0 0 2px ${theme.colorPrimary}; | |
| } | |
| .react-checkbox-tree .rct-option:focus-visible .rct-icon.rct-icon-expand-all, | |
| .react-checkbox-tree .rct-option:focus-visible .rct-icon.rct-icon-collapse-all { | |
| outline: 2px solid transparent; /* WCAG 2.4.7: transparent outline prevents double-ring */ | |
| box-shadow: 0 0 0 2px ${theme.colorPrimary}; |
Steps of Reproduction ✅
1. Open the dashboard filter scope configuration UI, which renders the `FilterScope`
container at `superset-frontend/src/dashboard/containers/FilterScope.tsx:24` and in turn
the `FilterScopeSelector` component defined in
`superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.tsx`.
2. In `FilterScopeSelector`, the left-hand filter tree uses `FilterFieldTree`
(`superset-frontend/src/dashboard/components/filterscope/FilterFieldTree.tsx:35-56`),
which renders `CheckboxTree` from `react-checkbox-tree` with `showExpandAll` and
`icons={treeIcons}`. This causes "Expand all" and "Collapse all" controls to be rendered
above the tree inside a `.react-checkbox-tree` container.
3. The icons for those controls are defined in
`superset-frontend/src/dashboard/components/filterscope/treeIcons.tsx:34-39` as `<span
className="rct-icon rct-icon-expand-all">` and `<span className="rct-icon
rct-icon-collapse-all">` with no `tabIndex` or `role`, meaning these spans themselves are
not focusable; focus is handled by react-checkbox-tree on its own interactive wrapper
element (e.g. `.rct-option`).
4. The focus styles added in this PR target `.react-checkbox-tree
.rct-icon.rct-icon-expand-all, .react-checkbox-tree .rct-icon.rct-icon-collapse-all { …
&:focus-visible { outline: …; box-shadow: … }}` at `FilterScopeSelector.tsx:200-201`.
Because `:focus-visible` only matches the actually focused element and the focusable
control is the outer react-checkbox-tree option (not the inner `.rct-icon` span), these
rules never apply in practice; keyboard-tabbing to "Expand all"/"Collapse all" does not
trigger the new box-shadow focus indicator on those controls, so the intended WCAG 2.4.7
focus styling for them is effectively dead code.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.tsx
**Line:** 214:216
**Comment:**
*Logic Error: The focus style is attached to `.rct-icon-*` spans, but keyboard focus lands on the expand/collapse option button, not the inner icon span. As a result, the new `:focus-visible` rule never applies and keyboard users still won't see a focus indicator on these controls. Move the focus-visible selector to the focusable `.rct-option` and style the nested icon from there.
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.…pe HC outline, fix focus target
Contributor
Code Review Agent Run #40ef65Actionable Suggestions - 0Additional Suggestions - 1
Review 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 |
Member
|
I'd suggest considering the following:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements WCAG 2.4.7 (Focus Visible) across Superset by introducing a global :focus-visible baseline and updating several components to avoid outline: none while providing consistent, visible keyboard focus indicators.
Changes:
- Add a global
*:focus-visibleoutline rule in core theme global styles. - Update multiple UI components to use transparent outlines and component-specific box-shadow focus rings instead of disabling outlines.
- Add/adjust focus-visible styling in filter plugins and dashboard/explore controls.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| superset-frontend/packages/superset-core/src/theme/GlobalStyles.tsx | Adds global *:focus-visible focus outline baseline. |
| superset-frontend/src/filters/components/Time/TimeFilterPlugin.tsx | Updates focus styling for the time filter container. |
| superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx | Updates focus styling for the range filter focus container. |
| superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx | Adds :focus-visible focus ring styling for viz type selector elements. |
| superset-frontend/src/explore/components/controls/ConditionalFormattingControl/ConditionalFormattingControl.tsx | Adds :focus-visible focus ring styling to the close button. |
| superset-frontend/src/dashboard/styles.ts | Updates shared dashboard focus styles to keep a transparent outline baseline. |
| superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx | Removes blanket outline suppression and adds :focus-visible outline baseline. |
| superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.tsx | Adds :focus-visible focus ring styling for react-checkbox-tree buttons and restores outline baseline for search input. |
| superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx | Adds :focus-visible focus ring styling for filter badge buttons. |
Comment on lines
113
to
118
| &:focus { | ||
| box-shadow: 0 0 0 2px ${theme.colorPrimary}; | ||
| } | ||
| &:focus-visible { | ||
| outline: none; | ||
| outline: 2px solid transparent; /* WCAG 2.4.7: transparent outline prevents double-ring; box-shadow on :focus provides visible indicator */ | ||
| }`} |
There was a problem hiding this comment.
box-shadow is currently applied on :focus, while :focus-visible only sets a transparent outline. If the container gets focused via mouse click (e.g., via a programmatic .focus() in response to clicking inside), the box-shadow focus ring may still appear on mouse interactions, which conflicts with the PR’s intent to show focus indication only for keyboard focus. Consider applying the visible focus styling (box-shadow) on :focus-visible instead of :focus (and keep the transparent outline as the HC-mode baseline).
Comment on lines
+117
to
+118
| outline: 2px solid ${theme.colorPrimary} !important; | ||
| outline-offset: 2px !important; |
There was a problem hiding this comment.
The global *:focus-visible rule uses !important for outline/outline-offset, but several components updated in this PR rely on setting outline: 2px solid transparent on :focus-visible to avoid a double focus ring while using box-shadow as the visible indicator. With !important here, those component-level outline declarations cannot override the global outline, so users will likely see both the global outline and the component box-shadow. Consider removing !important (or, if you truly need it, ensure component overrides also use !important where they intentionally make the outline transparent).
Suggested change
| outline: 2px solid ${theme.colorPrimary} !important; | |
| outline-offset: 2px !important; | |
| outline: 2px solid ${theme.colorPrimary}; | |
| outline-offset: 2px; |
| @@ -62,8 +62,8 @@ const ControlContainer = styled.div<{ | |||
|
|
|||
| &:focus > div { | |||
There was a problem hiding this comment.
This focus styling is attached to :focus rather than :focus-visible, which means the custom focus ring can still appear for mouse interactions (depending on how/when the container is programmatically focused). To align with the PR goal and testing instructions (“click elements → no focus ring”), consider moving the visual focus indicator (border/box-shadow) to :focus-visible (and/or :focus-within if the intent is to reflect focus on the inner date input).
Suggested change
| &:focus > div { | |
| &:focus-visible > div { |
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
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.
SUMMARY
Implements WCAG 2.1 criterion 2.4.7 (Focus Visible, Level AA).
*:focus-visiblerule with2px solidoutline using theme primary coloroutline: nonedeclarations withoutline: 2px solid transparent(preserves Windows High Contrast Mode)box-shadowas visible focus ring with transparent outline as baselineTESTING INSTRUCTIONS
:focus-visibleonly triggers on keyboard)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.