codeant-ai-for-open-source[bot] commented on code in PR #39234:
URL: https://github.com/apache/superset/pull/39234#discussion_r3499189721
##########
superset-frontend/packages/superset-core/src/theme/Theme.tsx:
##########
@@ -124,6 +161,30 @@ export class Theme {
// First phase: Let Ant Design compute the tokens
const tokens = Theme.getFilteredAntdTheme(antdConfig);
+ // WCAG 1.4.3: when the caller supplies no brand color at all, Ant Design's
+ // own default `colorLink = #1677ff` sits at ~4.32:1 against white — below
+ // the 4.5:1 normal-text threshold. Nudge the link tokens into compliance
+ // only in that "neither colorPrimary nor colorLink was supplied" path, so
+ // explicit brand colors keep the exact value the caller picked.
Deployments
+ // that intentionally pass a low-contrast brand colorPrimary keep their
+ // brand fidelity; the WCAG guarantee here applies to the *default*
Superset
+ // theme rather than to operator-customized themes.
+ const callerSetLinkOrPrimary =
+ !!(config as AnyThemeConfig)?.token?.colorLink ||
+ !!(config as AnyThemeConfig)?.token?.colorPrimary;
+ if (!callerSetLinkOrPrimary) {
Review Comment:
**Suggestion:** The contrast sanitization gate is checking whether
`colorLink`/`colorPrimary` exists anywhere in the merged config, which
incorrectly treats inherited base-theme defaults as explicit user intent. In
real bootstrapped themes where base config already provides low-contrast
`colorLink` (for example `#2893B3`), this condition skips `ensureLinkContrast`,
so the WCAG fix is not applied at runtime. Gate this on whether the current
caller explicitly set link/primary (not whether they exist after merge), or
sanitize final link tokens unless there is an explicit per-call opt-out.
[incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard link tokens from base themes stay low contrast.
- ⚠️ Global theme link colors may miss WCAG 1.4.3.
- ⚠️ Accessibility audits fail when using THEME_DEFAULT bootstrap themes.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Backend supplies a system default theme with a low-contrast brand color
(e.g.
`token.colorPrimary = "#2893B3"` and no explicit `colorLink`), as documented
in
`docs/admin_docs_versioned_docs/version-6.1.0/configuration/theming.mdx:232-234`,
which is
exposed to the frontend via bootstrap data and loaded by
`ThemeController.loadBootstrapData()` in
`superset-frontend/src/theme/ThemeController.ts:682-21`, setting
`this.defaultTheme` to
that bootstrap default theme.
2. Open a dashboard whose CRUD theme JSON does not override
`token.colorPrimary` or
`token.colorLink` (a common case when relying on the system’s base theme).
The dashboard
theming flow in
`superset-frontend/src/components/CrudThemeProvider.tsx:52-73` parses
`theme.json_data`, normalizes it, reads `bootstrapTheme` from
`getBootstrapData()`,
computes `baseTheme = isDark ? bootstrapTheme.dark :
bootstrapTheme.default`, and then
calls `Theme.fromConfig(normalizedConfig, baseTheme || undefined)` at lines
67-70.
3. Inside `Theme.fromConfig` in
`superset-frontend/packages/superset-core/src/theme/Theme.tsx:104-129`,
because both
`baseTheme` and `config` are present, the method merges them into
`mergedConfig`, which
now contains `token.colorPrimary` inherited from `baseTheme` (e.g.
`"#2893B3"`) and
potentially a derived `colorLink`. It then constructs a new `Theme` instance
via `return
new Theme({ config: mergedConfig });`, whose private constructor at
`Theme.tsx:90-93`
immediately calls `this.setConfig(config || {});` with that merged
configuration.
4. In `Theme.setConfig` at `Theme.tsx:154-186`, the `callerSetLinkOrPrimary`
flag is
computed from the merged config: `const callerSetLinkOrPrimary = !!(config as
AnyThemeConfig)?.token?.colorLink || !!(config as
AnyThemeConfig)?.token?.colorPrimary;`
(lines 172-174). Because `mergedConfig.token.colorPrimary` came from the
base theme, this
flag is truthy, so the `if (!callerSetLinkOrPrimary) { ... }` block at lines
175-186 is
skipped and `ensureLinkContrast()` is never applied to `tokens.colorLink`,
`tokens.colorLinkHover`, or `tokens.colorLinkActive`. As a result, dashboard
link colors
derived from the inherited base theme brand (e.g. `#2893B3`) retain their
original,
sub-4.5:1 contrast against `tokens.colorBgContainer` instead of being nudged
to the
accessible value the WCAG helper would compute, meaning the PR’s intended
contrast fix
does not actually take effect for these merged base-theme-driven themes.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cd2c40249d2c48e0bbcba3b87b0c8c1c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=cd2c40249d2c48e0bbcba3b87b0c8c1c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-core/src/theme/Theme.tsx
**Line:** 172:175
**Comment:**
*Incorrect Condition Logic: The contrast sanitization gate is checking
whether `colorLink`/`colorPrimary` exists anywhere in the merged config, which
incorrectly treats inherited base-theme defaults as explicit user intent. In
real bootstrapped themes where base config already provides low-contrast
`colorLink` (for example `#2893B3`), this condition skips `ensureLinkContrast`,
so the WCAG fix is not applied at runtime. Gate this on whether the current
caller explicitly set link/primary (not whether they exist after merge), or
sanitize final link tokens unless there is an explicit per-call opt-out.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39234&comment_hash=57d2b379e5063a0a01f38a60ce2946144fd837d588045b9a136e4e1307fb949b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39234&comment_hash=57d2b379e5063a0a01f38a60ce2946144fd837d588045b9a136e4e1307fb949b&reaction=dislike'>👎</a>
--
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]
