Aitema-gmbh commented on code in PR #39242:
URL: https://github.com/apache/superset/pull/39242#discussion_r3232765825
##########
superset-frontend/packages/superset-core/src/theme/GlobalStyles.tsx:
##########
@@ -109,6 +109,14 @@ export const GlobalStyles = () => {
display: flex;
margin-top: ${theme.marginXS}px;
}
+
+ /* WCAG 2.4.7: Focus Visible — ensure all interactive elements have a
visible
+ keyboard focus indicator. Uses :focus-visible to avoid showing on
mouse clicks.
+ Individual components can override with their own focus styles. */
+ *:focus-visible {
+ outline: 2px solid ${theme.colorPrimary};
+ outline-offset: 2px;
Review Comment:
WONTFIX. Thanks for flagging this. The suggestion conflicts with maintainer
feedback on this PR: @rusackas requested that the `!important` flags be removed
from the global `*:focus-visible` rule (and component-level overrides such as
`outline: 2px solid transparent` for the double-ring suppression be allowed to
win). Specifically, several updated components in this PR rely on being able to
override the global outline. Doubling down on `!important` here would break
those component-level overrides and produce both a global outline ring AND the
intended component `box-shadow` ring simultaneously. The underlying point you
raise about `InfoTooltip` (and similar components) suppressing focus via
higher-specificity `outline: none` rules is real, but the correct fix is to
update those component rules to honor `:focus-visible`, not to force
`!important` into the global cascade. Will be addressed in the follow-up
cleanup pass on this PR.
##########
superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx:
##########
@@ -80,10 +80,15 @@ export const FilterItem = styled.button`
padding: 0;
border: none;
background: none;
- outline: none;
+ outline: 2px solid transparent; /* WCAG 2.4.7: transparent outline
prevents double-ring with box-shadow */
width: 100%;
color: inherit;
+ &:focus-visible {
+ box-shadow: 0 0 0 2px ${theme.colorPrimary};
Review Comment:
ADDRESSED. Thanks — the `box-shadow: inset 0 0 0 2px ${theme.colorPrimary};`
suggestion was applied. Current code at
`superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx:88` reads
exactly:
```css
&:focus-visible {
box-shadow: inset 0 0 0 2px ${theme.colorPrimary};
border-radius: ${theme.borderRadius}px;
}
```
The inset ring stays fully visible inside the `FiltersDetailsContainer`
(`overflow-x: hidden`, `max-width: 300px`) and resolves the WCAG 2.4.7 clipping
concern.
##########
superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx:
##########
@@ -50,7 +50,7 @@ interface WithPopoverMenuState {
const WithPopoverMenuStyles = styled.div`
${({ 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 */
Review Comment:
VALID-FOLLOWUP. Good catch — applying `outline: 2px solid transparent`
unconditionally to the `WithPopoverMenu` wrapper means Windows High Contrast /
forced-colors mode renders a visible system-highlight outline around every
dashboard layout wrapper (Row, Column, Header) regardless of focus state. The
fix is to scope the transparent outline to `&.with-popover-menu--focused` (or
equivalent focused-state class) as you suggest, so the HC-mode fallback only
appears when the wrapper is actually focused. Will track this for the follow-up
pass on this PR — not changing it in this triage round, but the finding is
valid.
##########
superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.tsx:
##########
@@ -211,8 +211,9 @@ const ScopeSelector = styled.div`
text-decoration: underline;
}
- &:focus {
- outline: none;
+ &:focus-visible {
+ outline: 2px solid transparent; /* WCAG 2.4.7: transparent outline
prevents double-ring */
+ box-shadow: 0 0 0 2px ${theme.colorPrimary};
Review Comment:
VALID-FOLLOWUP. Confirmed: keyboard focus in `react-checkbox-tree` lands on
the `.rct-option` button, not on the `.rct-icon-expand-all` /
`.rct-icon-collapse-all` `<span>` elements where the `:focus-visible` rule is
currently attached. As a result the new focus indicator never triggers on the
Expand-all / Collapse-all controls. The correct selector is
`.react-checkbox-tree .rct-option:focus-visible .rct-icon.rct-icon-expand-all`
(and the collapse-all variant) — the focus-visible state must live on the
focusable button, with the styled icon as a descendant. Tracking for the
follow-up cleanup pass — not modifying in this triage round, but the finding is
valid.
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -114,7 +114,7 @@ const FocusContainer = styled.div`
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 */
}`}
Review Comment:
VALID-FOLLOWUP. Agreed — the `box-shadow` focus ring on the Range filter
container should hang off `:focus-visible`, not `:focus`, so programmatic /
mouse-driven focus events don't paint a keyboard-style focus ring. The
transparent outline can stay as the forced-colors-mode baseline. Will tighten
this in the follow-up pass — flagged as a valid finding but not changing in
this triage round.
##########
superset-frontend/packages/superset-core/src/theme/GlobalStyles.tsx:
##########
@@ -109,6 +109,14 @@ export const GlobalStyles = () => {
display: flex;
margin-top: ${theme.marginXS}px;
}
+
+ /* WCAG 2.4.7: Focus Visible — ensure all interactive elements have a
visible
+ keyboard focus indicator. Uses :focus-visible to avoid showing on
mouse clicks.
+ Individual components can override with their own focus styles. */
+ *:focus-visible {
+ outline: 2px solid ${theme.colorPrimary} !important;
+ outline-offset: 2px !important;
Review Comment:
VALID-FOLLOWUP. Correct and aligned with maintainer feedback (@rusackas
asked for the `!important` flags to be removed). The `!important` on the global
`*:focus-visible` rule prevents the per-component `outline: 2px solid
transparent` overrides (used in tandem with a component `box-shadow` to avoid a
double focus ring) from winning the cascade — so users currently see both the
global outline AND the component box-shadow. Will be removed in the follow-up
cleanup pass on this PR.
--
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]
