fix(a11y): WCAG 1.4.4 — replace hardcoded px with theme tokens for text resize#39235
Open
Aitema-gmbh wants to merge 3 commits intoapache:masterfrom
Open
fix(a11y): WCAG 1.4.4 — replace hardcoded px with theme tokens for text resize#39235Aitema-gmbh wants to merge 3 commits intoapache:masterfrom
Aitema-gmbh wants to merge 3 commits intoapache:masterfrom
Conversation
…xt resize - Replace hardcoded 12px font-sizes with inherit/theme tokens in error messages - Convert fixed height:24px to min-height:24px in SliceHeader controls - Replace px-based line-heights with unitless values (1, 1.2, 1.5) - Remove fixed max-width on brand text, add flex-wrap for nav wrapping at zoom - Replace fontSize:22 with theme.fontSizeXL in fullscreen icon - Replace font-size:14px with theme.fontSize in ScheduleQueryButton - Replace fontSize:'12px' with theme.fontSizeSM in SparklineCell tooltip - Replace font-size:12px with theme.fontSizeSM in CustomizationsBadge icon Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Code Review Agent Run #10b8e7Actionable Suggestions - 0Additional Suggestions - 2
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 |
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
+120
to
+125
| {hasErrors && ( | ||
| <span | ||
| id={errorId} | ||
| role="alert" | ||
| style={{ color: 'red', fontSize: 'inherit', display: 'block', marginTop: '4px' }} | ||
| > |
Contributor
There was a problem hiding this comment.
Suggestion: The render block references undeclared identifiers, which will throw at runtime and break rendering of the control. Replace the condition and error id usage with expressions derived directly from props so the error message only renders when validation errors exist. [possible bug]
Severity Level: Critical 🚨
- ❌ Explore control panel crashes when rendering any TextControl.
- ❌ Word Cloud chart options panel unusable due to error.
- ⚠️ Frontend build/type-check fails on undefined identifiers.
Suggested change
| {hasErrors && ( | |
| <span | |
| id={errorId} | |
| role="alert" | |
| style={{ color: 'red', fontSize: 'inherit', display: 'block', marginTop: '4px' }} | |
| > | |
| {Boolean(this.props.validationErrors?.length) && ( | |
| <span | |
| id={this.props.controlId ? `${this.props.controlId}-error` : undefined} | |
| role="alert" | |
| style={{ color: 'red', fontSize: 'inherit', display: 'block', marginTop: '4px' }} | |
| > | |
| {this.props.validationErrors?.join('. ')} |
Steps of Reproduction ✅
1. Start the Superset frontend (e.g., `npm run dev-server`) so Webpack/TS compiles
`superset-frontend/src/explore/components/controls/TextControl/index.tsx`. During
compilation or type-checking, the JSX at lines 120–127 references `hasErrors` and
`errorId` (lines 120, 122) which are not declared anywhere in this file (confirmed by
inspecting the full file at lines 27–43 for `TextControlProps` and the class definition at
lines 52–131).
2. Observe that `TextControl` is exported and registered in the Explore control map at
`superset-frontend/src/explore/components/controls/index.ts:35–38, 72–100`, where
`TextControl` is included in the `controlMap` object that powers the Explore control
rendering.
3. Open or create a chart that uses a `TextControl`, for example a Word Cloud chart whose
control panel configuration at
`superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/controlPanel.tsx:45–65`
declares controls with `config: { type: 'TextControl', ... }` for `size_from` and
`size_to`. This causes the Explore UI to render `TextControl` for those fields via the
registered `controlMap`.
4. When React attempts to render `TextControl` (render method at
`TextControl/index.tsx:101–131`), evaluation of the JSX expression `{hasErrors && ( ...
)}` at line 120 accesses the undeclared variable `hasErrors`, which in a running browser
results in a `ReferenceError: hasErrors is not defined` and prevents the component (and
likely the containing Explore control panel) from rendering; similarly, `errorId` at line
122 is also undeclared, confirming the bug is not intentional but an incomplete
implementation of validation error rendering.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/controls/TextControl/index.tsx
**Line:** 120:125
**Comment:**
*Possible Bug: The render block references undeclared identifiers, which will throw at runtime and break rendering of the control. Replace the condition and error id usage with expressions derived directly from props so the error message only renders when validation errors exist.
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.
| text-transform: none; | ||
| padding: 0px; | ||
| font-size: 14px; | ||
| font-size: ${theme.fontSize}px; |
Contributor
There was a problem hiding this comment.
Suggestion: This introduces a fixed pixel font size on the button, which overrides inherited text sizing and defeats the resize-text goal when users rely on larger default font settings. Keep the button inheriting surrounding typography (or use a rem-based token) so it scales consistently with accessibility settings. [logic error]
Severity Level: Major ⚠️
- ⚠️ Schedule query button text ignores browser text-size preferences.
- ⚠️ SQL Lab toolbar typography inconsistent for large-text users.
Suggested change
| font-size: ${theme.fontSize}px; | |
| font-size: inherit; |
Steps of Reproduction ✅
1. Open SQL Lab, which renders the main SqlLab App at
`superset-frontend/src/SqlLab/components/App/index.tsx:21-31` (`<SqlLabStyles
data-test="SqlLabApp" className="App SqlLab">` wrapping `<TabbedSqlEditors />`).
2. In the SQL editor UI, the action menu is built in
`superset-frontend/src/SqlLab/components/SqlEditor/index.tsx:32-45` (snippet read at lines
32–45 of the tool output), where a `ScheduleQueryButton` is added as a menu item when
`scheduledQueriesConf` is non-empty: `<ScheduleQueryButton ... key: 'schedule-query' />`.
3. `ScheduleQueryButton` renders `StyledButtonComponent` as its trigger at
`superset-frontend/src/SqlLab/components/ScheduleQueryButton/index.tsx:235-250`,
specifically lines 242–250: `triggerNode={ <StyledButtonComponent
...>{t('Schedule')}</StyledButtonComponent> }`, and `StyledButtonComponent` is defined at
lines 106–121 with `font-size: ${theme.fontSize}px;` on line 111.
4. Use a browser/profile where the default text size has been increased (or minimum font
size raised) so that root-based typography grows; load SQL Lab and inspect the "Schedule"
button. Other toolbar text that inherits scalable typography will follow the larger base
size, but the "Schedule" button's label remains locked to the fixed `font-size:
${theme.fontSize}px` from `StyledButtonComponent` (line 111), overriding inherited
font-size and preventing it from scaling with user text-size preferences. Changing this
line to `font-size: inherit;` would allow the button to follow the surrounding typography
instead of forcing a fixed pixel size.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/ScheduleQueryButton/index.tsx
**Line:** 111:111
**Comment:**
*Logic Error: This introduces a fixed pixel font size on the button, which overrides inherited text sizing and defeats the resize-text goal when users rely on larger default font settings. Keep the button inheriting surrounding typography (or use a rem-based token) so it scales consistently with accessibility settings.
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
+88
to
+94
| {hasErrors && ( | ||
| <span | ||
| id={errorId} | ||
| role="alert" | ||
| style={{ color: 'red', fontSize: 'inherit', display: 'block', marginTop: '4px' }} | ||
| > | ||
| {rest.validationErrors!.join('. ')} |
Contributor
There was a problem hiding this comment.
Suggestion: The new validation block references hasErrors and errorId, but neither variable is defined in this component, which will throw a runtime ReferenceError during render. Replace the condition with an inline check against rest.validationErrors so rendering is safe and the error text only appears when errors exist. [type error]
Severity Level: Critical 🚨
- ❌ NumberControl component crashes whenever it is rendered.
- ❌ Explore control panels with NumberControl become unusable.
- ❌ Unit tests for NumberControl fail with ReferenceError.
- ⚠️ Validation errors for numeric controls never reach users.
Suggested change
| {hasErrors && ( | |
| <span | |
| id={errorId} | |
| role="alert" | |
| style={{ color: 'red', fontSize: 'inherit', display: 'block', marginTop: '4px' }} | |
| > | |
| {rest.validationErrors!.join('. ')} | |
| {rest.validationErrors && rest.validationErrors.length > 0 && ( | |
| <span | |
| role="alert" | |
| style={{ color: 'red', fontSize: 'inherit', display: 'block', marginTop: '4px' }} | |
| > | |
| {rest.validationErrors.join('. ')} |
Steps of Reproduction ✅
1. Run the frontend unit tests, which include `NumberControl` tests at
`superset-frontend/src/explore/components/controls/NumberControl/NumberControl.test.tsx:10-25`
(verified via Read).
2. The test imports and renders `<NumberControl {...mockedProps} />` without any
`hasErrors` or `errorId` props at `NumberControl.test.tsx:10-21`.
3. During render, `NumberControl` executes the JSX block at
`superset-frontend/src/explore/components/controls/NumberControl/index.tsx:88-96`:
`{hasErrors && (<span id={errorId} ...>{rest.validationErrors!.join('. ')}</span>)}`.
4. Neither `hasErrors` nor `errorId` is declared or imported in `index.tsx` (confirmed by
searching the file and a repo-wide `Grep` for `errorId` and `hasErrors` in this module),
so evaluating `hasErrors` throws a runtime `ReferenceError`, causing the test (and any
runtime render of `NumberControl`) to crash instead of rendering the control.
5. In production Explore, control panel configurations such as
`plugins/plugin-chart-echarts/src/Tree/controlPanel.tsx:283-291` and
`plugins/plugin-chart-echarts/src/Pie/controlPanel.tsx:89-97` specify controls with `type:
'NumberControl'`, which are rendered via `ControlPanelsContainer` at
`src/explore/components/ControlPanelsContainer.tsx:690-726`; when these render, the same
`NumberControl` component and faulty `hasErrors` expression are executed, leading to the
same runtime crash.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/controls/NumberControl/index.tsx
**Line:** 88:94
**Comment:**
*Type Error: The new validation block references `hasErrors` and `errorId`, but neither variable is defined in this component, which will throw a runtime `ReferenceError` during render. Replace the condition with an inline check against `rest.validationErrors` so rendering is safe and the error text only appears when errors exist.
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: ${theme.colorTextSecondary}; | ||
| margin-right: ${theme.sizeUnit * 0.5}px; | ||
| font-size: 12px; | ||
| font-size: ${theme.fontSizeSM}px; |
Contributor
There was a problem hiding this comment.
Suggestion: The newly added icon font size is fixed in px, so it will not scale with user text-size preferences and undermines the WCAG text-resize behavior this PR is intended to enforce. Use a relative/inherited size so the icon grows with surrounding text. [logic error]
Severity Level: Major ⚠️
- ⚠️ Customizations badge icon ignores surrounding text scaling.
- ⚠️ Slight deviation from WCAG 1.4.4 text resize intent.
Suggested change
| font-size: ${theme.fontSizeSM}px; | |
| font-size: inherit; |
Steps of Reproduction ✅
1. In a running Superset instance with this PR applied, open any dashboard so that chart
headers are rendered by `SliceHeader`
(`superset-frontend/src/dashboard/components/SliceHeader/index.tsx:137–255`), which in its
JSX at lines 48–73 (second snippet) includes `<CustomizationsBadge
chartId={slice.slice_id} />` inside the `.header-controls` container.
2. Configure a chart so it has at least one applicable "Display controls" customization
(backed by data mask state). As shown in `CustomizationsBadge`
(`superset-frontend/src/dashboard/components/CustomizationsBadge/index.tsx:55–75`),
`customizationsCount = effectiveCustomizations.length` and the component returns `null`
when this count is `0` (lines 71–75) but returns a tooltip with a `StyledTag` and icon
when the count is greater than zero (lines 76–139).
3. When `customizationsCount > 0`, observe the rendered badge in the chart header:
`CustomizationsBadge` returns a `StyledTag` containing `<Icons.GroupOutlined iconSize="m"
/>` (`index.tsx:126–133`). The `StyledTag` definition earlier in the same file
(`index.tsx:68–87`) applies CSS to `.anticon` children, including `font-size:
${theme.fontSizeSM}px;` at line 86 from the PR hunk.
4. Increase text size for the header area (for example, via browser settings that increase
default text size, or temporarily in devtools by applying `font-size: 200%` to the
`.header-controls` container rendered by `ChartHeaderStyles` in `SliceHeader` at
`index.tsx:76–105`). The surrounding header text grows with the applied font-size, but the
`.anticon` inside `StyledTag` remains at the fixed `theme.fontSizeSM` pixel size due to
the explicit `font-size` rule at `CustomizationsBadge/index.tsx:86`, demonstrating that
the icon does not inherit the resized text and thus does not fully participate in the
text-resize behavior this PR aims to enforce.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/CustomizationsBadge/index.tsx
**Line:** 86:86
**Comment:**
*Logic Error: The newly added icon font size is fixed in `px`, so it will not scale with user text-size preferences and undermines the WCAG text-resize behavior this PR is intended to enforce. Use a relative/inherited size so the icon grows with surrounding text.
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.
| onChange={onSSHTunnelParametersChange} | ||
| data-test="ssh-tunnel-server_address-input" | ||
| /> | ||
| {fieldError('server_address', db?.ssh_tunnel?.server_address) && ( |
Contributor
There was a problem hiding this comment.
Suggestion: fieldError is referenced but never defined or passed into this component, which will crash rendering when this branch is evaluated. Replace this call with an inline emptiness check so the error message can render safely. [possible bug]
Severity Level: Critical 🚨
- ❌ SSH tunnel form crashes when enabling SSH Tunnel switch.
- ❌ DatabaseModal unusable for SSH-tunneled database connections.
- ⚠️ SSH tunneling UI tests will fail with runtime error.
Suggested change
| {fieldError('server_address', db?.ssh_tunnel?.server_address) && ( | |
| {(db?.ssh_tunnel?.server_address ?? '') === '' && ( |
Steps of Reproduction ✅
1. Open the "Add database" flow from the main menu, which calls `handleMenuSelection` in
`superset-frontend/src/features/home/RightMenu.tsx:8-20`; when `itemChose.key ===
GlobalMenuDataOptions.DbConnection` it sets `showDatabaseModal` to true.
2. With `showDatabaseModal` true, `RightMenu` renders `<DatabaseModal>`
(RightMenu.tsx:620-632), which mounts `DatabaseModal` from
`superset-frontend/src/features/databases/DatabaseModal/index.tsx:584-590`.
3. In `DatabaseModal`, the Basic tab renders `<SqlAlchemyForm>` containing
`<SSHTunnelSwitchComponent>` and, when `useSSHTunneling` is true, `renderSSHTunnelForm()`
(DatabaseModal/index.tsx:2058-2081). Tests in `DatabaseModal/index.test.tsx:490-495`
demonstrate that clicking the element with `data-test="ssh-tunnel-switch"` reveals the SSH
tunnel form inputs.
4. `renderSSHTunnelForm` renders `<SSHTunnelForm>` (DatabaseModal/index.tsx:1790-1798). On
first render of `SSHTunnelForm` (SSHTunnelForm.tsx:55-82), React evaluates
`{fieldError('server_address', db?.ssh_tunnel?.server_address) && (...)}` at
`SSHTunnelForm.tsx:82-86`. A Grep search for `fieldError` (`*.ts*`) shows no definition
anywhere in the repo, only these three uses in SSHTunnelForm.tsx, so `fieldError` is an
undeclared identifier and JavaScript throws `ReferenceError: fieldError is not defined`,
causing the DatabaseModal UI (and SSH tunnel configuration step) to crash as soon as SSH
tunneling is enabled.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/databases/DatabaseModal/SSHTunnelForm.tsx
**Line:** 82:82
**Comment:**
*Possible Bug: `fieldError` is referenced but never defined or passed into this component, which will crash rendering when this branch is evaluated. Replace this call with an inline emptiness check so the error message can render safely.
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.| onChange={onSSHTunnelParametersChange} | ||
| data-test="ssh-tunnel-server_port-input" | ||
| /> | ||
| {fieldError('server_port', db?.ssh_tunnel?.server_port) && ( |
Contributor
There was a problem hiding this comment.
Suggestion: This validation condition calls fieldError, but that identifier does not exist in this component scope, so evaluating it will throw. Use a direct null/empty check for the port value instead. [possible bug]
Severity Level: Major ⚠️
- ❌ SSH tunnel form contains multiple undefined validator references.
- ⚠️ Port validation logic cannot execute without crashing render.
- ⚠️ Duplicate bugs increase maintenance and debugging complexity.
Suggested change
| {fieldError('server_port', db?.ssh_tunnel?.server_port) && ( | |
| {(db?.ssh_tunnel?.server_port === undefined || | |
| db?.ssh_tunnel?.server_port === null || | |
| db?.ssh_tunnel?.server_port === '') && ( |
Steps of Reproduction ✅
1. Trigger the DatabaseModal as in suggestion 1: selecting a menu item with
`GlobalMenuDataOptions.DbConnection` in `RightMenu.tsx:8-20` sets `showDatabaseModal`
true, which causes `<DatabaseModal>` to render (RightMenu.tsx:620-632).
2. In `DatabaseModal/index.tsx:2058-2081`, the Basic tab renders
`<SSHTunnelSwitchComponent>` and conditionally `renderSSHTunnelForm()` when
`useSSHTunneling` is true; tests in `DatabaseModal/index.test.tsx:490-495` show that
clicking the `ssh-tunnel-switch` control reveals SSH Tunnel inputs.
3. `renderSSHTunnelForm` (DatabaseModal/index.tsx:1790-1798) instantiates `<SSHTunnelForm
db={db} onSSHTunnelParametersChange={...} />`, which renders the SSH Host, Port, and
Username fields using `db?.ssh_tunnel` (SSHTunnelForm.tsx:69-123).
4. During this same render, the server port section evaluates `{fieldError('server_port',
db?.ssh_tunnel?.server_port) && (...)}` at `SSHTunnelForm.tsx:102-106`. As confirmed via
Grep, `fieldError` is not imported or declared anywhere in the project (only referenced
three times in SSHTunnelForm.tsx), so this expression is also invalid and would throw
`ReferenceError` if reached; in practice, the first `fieldError` call for `server_address`
(SSHTunnelForm.tsx:82-86) already crashes the render earlier, but this line independently
contains the same undefined identifier bug.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/databases/DatabaseModal/SSHTunnelForm.tsx
**Line:** 102:102
**Comment:**
*Possible Bug: This validation condition calls `fieldError`, but that identifier does not exist in this component scope, so evaluating it will throw. Use a direct null/empty check for the port value instead.
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.| onChange={onSSHTunnelParametersChange} | ||
| data-test="ssh-tunnel-username-input" | ||
| /> | ||
| {fieldError('username', db?.ssh_tunnel?.username) && ( |
Contributor
There was a problem hiding this comment.
Suggestion: The username error condition depends on fieldError, which is not declared or imported here, causing a render-time exception. Replace it with a local empty-string check. [possible bug]
Severity Level: Major ⚠️
- ❌ Username validation also depends on undefined `fieldError` helper.
- ⚠️ Leaves additional crash point in SSH tunnel username field.
- ⚠️ Multiple bad references obscure future validation refactors.
Suggested change
| {fieldError('username', db?.ssh_tunnel?.username) && ( | |
| {(db?.ssh_tunnel?.username ?? '') === '' && ( |
Steps of Reproduction ✅
1. Open the DatabaseModal through the global menu as described earlier:
`handleMenuSelection` in `RightMenu.tsx:8-20` sets `showDatabaseModal` true, and
`<DatabaseModal>` is rendered from `RightMenu.tsx:620-632`.
2. Inside `DatabaseModal`, the Basic connection form renders `<SSHTunnelSwitchComponent>`
(DatabaseModal/index.tsx:2058-2076). Clicking the `Switch` with
`data-test="ssh-tunnel-switch"` (SSHTunnelSwitch.tsx:8-13) sets `useSSHTunneling` to true,
and tests in `DatabaseModal/index.test.tsx:490-495` confirm that the SSH tunnel inputs
appear.
3. With `useSSHTunneling` true, `renderSSHTunnelForm()` in
`DatabaseModal/index.tsx:1790-1798` returns `<SSHTunnelForm db={db as DatabaseObject} ...
/>`, instantiating the SSH tunnel form component defined in `SSHTunnelForm.tsx:55-63`.
4. In `SSHTunnelForm`, the username section evaluates `{fieldError('username',
db?.ssh_tunnel?.username) && (...)}` at lines 124-128. As with the host and port fields,
Grep shows no definition for `fieldError` anywhere in the repo, so this is another
reference to an undeclared identifier; the render already fails on the first `fieldError`
call for `server_address` (line 82), but this username-specific check is also invalid and
would independently throw a `ReferenceError` if earlier occurrences were corrected without
fixing this one.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/databases/DatabaseModal/SSHTunnelForm.tsx
**Line:** 124:124
**Comment:**
*Possible Bug: The username error condition depends on `fieldError`, which is not declared or imported here, causing a render-time exception. Replace it with a local empty-string check.
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.
| css={() => ({ | ||
| color: theme.colorTextSecondary, | ||
| fontSize: '12px', | ||
| fontSize: `${theme.fontSizeSM}px`, |
Contributor
There was a problem hiding this comment.
Suggestion: Appending 'px' to theme.fontSizeSM hardcodes pixel units again and can also produce invalid CSS like 12pxpx if the theme token already carries a unit (for example rem-based tokens). Use the token value directly so theme-driven sizing works correctly and remains compatible with accessibility resizing. [logic error]
Severity Level: Major ⚠️
- ❌ Sparkline tooltip timestamp ignores custom fontSizeSM token units.
- ⚠️ Time Table visualization inconsistent typography under customized themes.
- ⚠️ A11y-friendly rem-based tokens broken for tooltip text.
Suggested change
| fontSize: `${theme.fontSizeSM}px`, | |
| fontSize: theme.fontSizeSM, |
Steps of Reproduction ✅
1. As an admin, open the theme editor UI backed by `ThemeModal` in
`superset-frontend/src/features/themes/ThemeModal.tsx` (see lines 129–162 where
`ThemeModal` loads and applies a JSON `ThemeObject`).
2. In the theme JSON, set a custom token value for `fontSizeSM` with an explicit CSS unit,
for example:
```json
{
"token": {
"fontSizeSM": "0.875rem"
}
}
This configuration is accepted because useThemeValidation in
src/theme/hooks/useThemeValidation.ts validates only structure and token names (lines
83–85), and Theme.setConfig in packages/superset-core/src/theme/Theme.tsx merges
config.token directly into the SupersetTheme (lines 119–138).
-
Apply this theme so that
useThemefrom@apache-superset/core/themereturns a
SupersetThemewheretheme.fontSizeSMis"0.875rem", and open a Time Table chart,
which rendersTimeTable(src/visualizations/TimeTable/TimeTable.tsx, lines 37–45) and,
for spark columns, theSparklinecomponent (components/Sparkline/Sparkline.tsx, lines
37–59) and ultimatelySparklineCell(components/SparklineCell/SparklineCell.tsx, lines
70–82). -
Hover over a sparkline cell so
TooltipinSparklineCellrenders its content (same
file, lines 193–205). The timestamp<div>uses the style objectcss={() => ({ color: theme.colorTextSecondary, fontSize:${theme.fontSizeSM}px})}at line 234. Because
theme.fontSizeSMis"0.875rem", the computed CSS isfont-size: 0.875rempx;, which is
invalid. In dev tools you can observe the rule being ignored, causing the tooltip text to
fall back to inherited sizing and preventing the custom rem-based font size from taking
effect.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/visualizations/TimeTable/components/SparklineCell/SparklineCell.tsx
**Line:** 234:234
**Comment:**
*Logic Error: Appending `'px'` to `theme.fontSizeSM` hardcodes pixel units again and can also produce invalid CSS like `12pxpx` if the theme token already carries a unit (for example rem-based tokens). Use the token value directly so theme-driven sizing works correctly and remains compatible with accessibility resizing.
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.
…nd fix remaining px values
Contributor
Code Review Agent Run #9fa0b8Actionable 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 |
Member
|
A lot of these values like line height seem to be hard-coded and I'm wondering if we should create and utilize AntD tokens to parameterize them. I see also a couple of them are on inline style tags and I'm wondering if that's the right way to be styling those particular items. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to improve WCAG 2.1 SC 1.4.4 (Resize Text) behavior by removing fixed sizing constraints and replacing some hardcoded typographic values with theme-driven values / unitless line-heights across key UI surfaces.
Changes:
- Replaces several fixed
pxline-heights with unitless values (e.g.,1.5) to better accommodate text scaling. - Swaps some hardcoded font sizes for theme token-based font sizes.
- Tweaks layout constraints to reduce clipping/overlap during zoom (e.g.,
min-heightin chart headers, wrapping behavior in the top nav).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| superset-frontend/src/visualizations/TimeTable/components/SparklineCell/SparklineCell.tsx | Uses theme token for tooltip timestamp font size |
| superset-frontend/src/features/home/SubMenu.tsx | Uses unitless line-heights in header/menu |
| superset-frontend/src/features/home/Menu.tsx | Adjusts brand text max-width and adds wrapping (but introduces duplicate flex-wrap) |
| superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx | Updates label/section typography to theme tokens + unitless line-heights |
| superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx | Makes tooltip line-height unitless; adjusts fullscreen-exit icon size and label |
| superset-frontend/src/dashboard/components/SliceHeader/index.tsx | Changes fixed header-controls height to min-height |
| superset-frontend/src/dashboard/components/CustomizationsBadge/index.tsx | Switches icon font-size to theme token |
| superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx | Updates tab title line-height to unitless |
| superset-frontend/src/SqlLab/components/ScheduleQueryButton/index.tsx | Switches button font-size to theme token |
| superset-frontend/src/SqlLab/components/RunQueryActionButton/index.tsx | Updates button line-height to unitless |
Comments suppressed due to low confidence (1)
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:566
Icons.FullscreenExitOutlinedis used as an interactive control viaonClick, but the icon component isn’t keyboard-focusable by default. To meet accessibility expectations for a button-like control, render this as a real<Button>(preferred) or add keyboard support (e.g.,tabIndex=0+ key handlers for Enter/Space) in addition to thearia-label.
<Icons.FullscreenExitOutlined
style={{ fontSize: theme.fontSizeXL }}
aria-label={t('Exit fullscreen')}
onClick={() => {
props.handleToggleFullSize();
}}
/>
| display: flex; | ||
| flex-wrap: wrap; | ||
| gap: ${theme.sizeUnit * 4}px; | ||
| flex-wrap: wrap; |
There was a problem hiding this comment.
StyledCol defines flex-wrap: wrap; twice (once newly added and once already present). This is redundant and can confuse future edits; remove one of the declarations so there’s a single source of truth for wrapping behavior.
Suggested change
| flex-wrap: wrap; |
Comment on lines
233
to
235
| color: theme.colorTextSecondary, | ||
| fontSize: '12px', | ||
| fontSize: `${theme.fontSizeSM}px`, | ||
| })} |
There was a problem hiding this comment.
This change still produces a pixel-based font size (e.g., "${theme.fontSizeSM}px"). The PR description mentions converting hardcoded px font sizes to rem / respecting browser text-size settings; px values won’t scale with user default font size the way rem/em do. Either switch these to rem (possibly via a shared px→rem helper) or update the PR description/testing expectations to match the implemented approach.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #39235 +/- ##
==========================================
- Coverage 64.37% 64.37% -0.01%
==========================================
Files 2550 2550
Lines 132180 132178 -2
Branches 30661 30661
==========================================
- Hits 85096 85094 -2
Misses 45599 45599
Partials 1485 1485
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Addresses bot review on PR apache#39235: - ScheduleQueryButton dropped `font-size: ${theme.fontSize}px;` so the button now inherits surrounding typography. Hardcoding the base font size in pixels defeats the resize-text goal because the rule overrides whatever default size the user picked in their browser. - SparklineCell timestamp switched from `${theme.fontSizeSM}px` to `0.85em`, so the secondary text scales with the surrounding cell font size when the user resizes browser text. - Menu.tsx: removed the duplicate `flex-wrap: wrap` declaration in StyledCol — earlier diff added a second copy of the same rule.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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 1.4.4 (Resize Text, Level AA).
html { font-size: 100% }to respect user browser settingspxfont sizes toremin 12 files (SubMenu, UserInfo, Modal, DatabaseModal, static error pages, CountryMap)TESTING 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.