Copilot commented on code in PR #38342:
URL: https://github.com/apache/superset/pull/38342#discussion_r2885564049
##########
superset-frontend/src/explore/components/controls/NumberControl/index.tsx:
##########
@@ -84,7 +90,19 @@ export default function NumberControl({
onBlur={handleBlur}
disabled={disabled}
aria-label={rest.label}
+ aria-invalid={hasErrors || undefined}
+ aria-describedby={errorId}
+ id={rest.name}
/>
+ {hasErrors && (
+ <span
+ id={errorId}
+ role="alert"
+ style={{ color: 'red', fontSize: 'inherit', display: 'block',
marginTop: '4px' }}
+ >
+ {rest.validationErrors!.join('. ')}
+ </span>
+ )}
Review Comment:
`errorId` is derived from `rest.name` (`${rest.name}-error`), but
`ControlHeader` also renders a hidden alert span with id `${name}-error` when
there are validation errors. This produces duplicate IDs and ambiguous
`aria-describedby` targets. Prefer a single error element per control (e.g.,
have the input reference the `ControlHeader` error span and remove the second
span here).
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -422,6 +431,8 @@ class Chart extends PureComponent<ChartProps, {}> {
data-test="chart-container"
height={height}
width={width}
+ role="img"
+ aria-label={chartAriaLabel}
>
Review Comment:
This wrapper contains complex/interactive chart content, but is now given
`role="img"`. In ARIA, `img` is intended for non-interactive graphics and can
cause assistive tech to ignore the subtree. Prefer using `aria-label` without
changing the role, or use a landmark/region role (e.g.,
`role="region"`/`figure`) so child content remains exposed.
##########
superset-frontend/src/a11y-regression.test.tsx:
##########
@@ -0,0 +1,132 @@
+/**
+ * A11y Regression Test Suite
+ *
+ * These tests verify that critical accessibility patches survive upstream
updates.
+ * Each test reads the actual source files and checks for required a11y
attributes,
+ * ensuring that merges or rebases do not silently remove accessibility fixes.
+ *
+ * Run: npx jest a11y-regression.test
+ */
+
+import fs from 'fs';
+import path from 'path';
+
+const ROOT = path.resolve(__dirname, '..');
+
+function readSource(relativePath: string): string {
+ const fullPath = path.join(ROOT, relativePath);
+ if (!fs.existsSync(fullPath)) {
+ throw new Error(`Source file not found: ${fullPath}`);
+ }
+ return fs.readFileSync(fullPath, 'utf-8');
+}
+
+describe('A11y Regression Tests', () => {
+ describe('Phase 1: Level A Fixes', () => {
+ test('ECharts Echart component uses SVGRenderer instead of
CanvasRenderer', () => {
Review Comment:
Repo guidance recommends using `test()` instead of nesting `describe()`
blocks to avoid deep nesting and improve readability. This new test suite uses
multiple nested `describe()` blocks; consider flattening to top-level `test()`
cases (or minimal grouping) to match the established convention.
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx:
##########
@@ -266,6 +266,9 @@ const VerticalFilterBar: FC<VerticalBarProps> = ({
className={cx({ open: !filtersOpen })}
onClick={openFiltersBar}
role="button"
+ tabIndex={0}
+ aria-expanded={false}
+ aria-label={t('Expand filters')}
offset={offset}
>
Review Comment:
`CollapsedBar` is exposed as a button but (1) `aria-expanded` is hard-coded
to `false` instead of reflecting `filtersOpen`, and (2) there’s no `onKeyDown`
handler for Enter/Space to activate it. Wire `aria-expanded` to state and add
keyboard activation to match `onClick` behavior.
##########
superset-frontend/src/components/Modal/ModalFormField.tsx:
##########
@@ -128,16 +128,36 @@ export function ModalFormField({
validateStatus,
hasFeedback = false,
}: ModalFormFieldProps) {
+ const uniqueId = useId();
+ const errorId = error ? `${uniqueId}-error` : undefined;
+
+ // Clone the first child element to inject aria-invalid and aria-describedby
+ const enhancedChildren = error
+ ? Children.map(children, (child, index) => {
+ if (index === 0 && isValidElement(child)) {
+ return cloneElement(child as React.ReactElement<any>, {
+ 'aria-invalid': true,
+ 'aria-describedby': errorId,
+ });
+ }
Review Comment:
`cloneElement(child as React.ReactElement<any>, ...)` references the `React`
namespace, but this file doesn’t import `React`. This will fail
typechecking/compilation. Use the `ReactElement` type (already available from
`react`) or import `type React` / `type ReactElement` and avoid
`React.ReactElement` here.
##########
superset-frontend/src/views/App.tsx:
##########
@@ -49,6 +49,13 @@ setupAGGridModules();
const bootstrapData = getBootstrapData();
+// WCAG 3.1.2: Set the HTML lang attribute based on the current locale
+// so screen readers announce the correct language for the page content.
+// Converts locale formats like "pt_BR" or "de-DE" to BCP-47 primary tag
("pt", "de").
+const locale =
+ bootstrapData.common?.locale || window.navigator.language || 'en';
+document.documentElement.lang = String(locale).split(/[_-]/)[0];
Review Comment:
Setting `document.documentElement.lang` to only the primary subtag (`pt_BR`
→ `pt`) drops region/script information and can reduce pronunciation accuracy
for some locales. Consider normalizing to a full BCP-47 tag (e.g., `pt-BR`)
instead of truncating, and/or documenting why truncation is required.
##########
superset-frontend/src/explore/components/controls/TextControl/index.tsx:
##########
@@ -116,7 +119,19 @@ export default class TextControl<
value={value}
disabled={this.props.disabled}
aria-label={this.props.label}
+ aria-invalid={hasErrors || undefined}
+ aria-describedby={errorId}
+ id={this.props.controlId || this.props.name}
/>
Review Comment:
The input `id` is set to `controlId || name`, but `ControlHeader`’s
`<FormLabel htmlFor={name}>` still points at `name`. When `controlId` is
provided and differs from `name`, this breaks label-to-input association.
Consider keeping `id` aligned with `name` (or update `ControlHeader` to use the
same id source).
##########
superset-frontend/src/explore/components/controls/TextControl/index.tsx:
##########
@@ -116,7 +119,19 @@ export default class TextControl<
value={value}
disabled={this.props.disabled}
aria-label={this.props.label}
+ aria-invalid={hasErrors || undefined}
+ aria-describedby={errorId}
+ id={this.props.controlId || this.props.name}
/>
+ {hasErrors && (
+ <span
+ id={errorId}
+ role="alert"
+ style={{ color: 'red', fontSize: 'inherit', display: 'block',
marginTop: '4px' }}
+ >
+ {this.props.validationErrors!.join('. ')}
+ </span>
+ )}
Review Comment:
`errorId` is generated as `${controlId || name}-error`, but `ControlHeader`
now also renders a hidden alert span with id `${name}-error` when there are
validation errors. When `controlId` is unset (common), this creates duplicate
IDs in the DOM. Either reuse the `ControlHeader` error element for
`aria-describedby` (and avoid rendering a second one), or ensure the IDs are
distinct.
##########
superset-frontend/src/explore/components/ControlHeader.tsx:
##########
@@ -176,8 +176,28 @@ const ControlHeader: FC<ControlHeaderProps> = ({
placement="top"
title={validationErrors?.join(' ')}
>
- <Icons.ExclamationCircleOutlined iconColor={theme.colorError}
/>
- </Tooltip>{' '}
+ <Icons.ExclamationCircleOutlined
+ iconColor={theme.colorError}
+ aria-hidden="true"
+ />
+ </Tooltip>
+ <span
+ id={`${name || 'control'}-error`}
+ role="alert"
+ css={css`
+ position: absolute;
+ width: 1px;
+ height: 1px;
+ padding: 0;
+ margin: -1px;
+ overflow: hidden;
+ clip: rect(0, 0, 0, 0);
+ white-space: nowrap;
+ border: 0;
+ `}
+ >
+ {validationErrors?.join('. ')}
+ </span>{' '}
Review Comment:
`ControlHeader` now renders a hidden `<span role="alert">` with id
`${name}-error` for validation errors. Several controls (e.g.,
TextControl/NumberControl) also render their own error elements with the same
id pattern, which can lead to duplicate IDs. It would be better to have a
single source of truth for the error element/id and have inputs reference that
via `aria-describedby`.
--
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]
