fix(a11y): WCAG 3.3.1 — add error identification with role=alert and aria-describedby#39245
Open
Aitema-gmbh wants to merge 4 commits intoapache:masterfrom
Open
fix(a11y): WCAG 3.3.1 — add error identification with role=alert and aria-describedby#39245Aitema-gmbh wants to merge 4 commits intoapache:masterfrom
Aitema-gmbh wants to merge 4 commits intoapache:masterfrom
Conversation
6 tasks
dosubot
Bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| } | ||
| const hasErrors = | ||
| this.props.validationErrors && this.props.validationErrors.length > 0; | ||
| const errorId = hasErrors ? `${this.props.controlId || this.props.name || 'text'}-error` : undefined; |
Contributor
There was a problem hiding this comment.
Suggestion: ControlHeader already creates an error element with id ${name}-error, and this new code reuses the same id for another <span>, which creates duplicate DOM ids and makes aria-describedby resolution ambiguous. Use a distinct suffix for this input-level error id so the association is deterministic. [logic error]
Severity Level: Major ⚠️
- ❌ Annotation layer name field renders duplicate error span ids.
- ❌ Annotation formula field renders duplicate error span ids.
- ⚠️ Screen readers may mis-associate formula annotation error text.
Suggested change
| const errorId = hasErrors ? `${this.props.controlId || this.props.name || 'text'}-error` : undefined; | |
| const errorId = hasErrors | |
| ? `${this.props.controlId || this.props.name || 'text'}-input-error` | |
| : undefined; |
Steps of Reproduction ✅
1. Open the Explore view for any chart type where the "Annotations and layers" control
section is present. This section is wired to use `AnnotationLayerControl` via the
`annotations` section configuration in
`superset-frontend/src/explore/controlPanels/sections.tsx:4-22` (config type
`'AnnotationLayerControl'`).
2. In the Explore control panel, under "Annotations and layers", click "Add annotation
layer". This opens a popover rendered by `AnnotationLayerControl.renderPopover` in
`superset-frontend/src/explore/components/controls/AnnotationLayerControl/index.tsx:18-40`,
which mounts the `AnnotationLayer` component.
3. In the popover, interact with fields backed by `TextControl` that supply `name` and
`validationErrors` but no `controlId`, for example: (a) leave the "Name" field empty so
`TextControl` at
`superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.tsx:1064-1071`
gets `validationErrors={!name ? [t('Mandatory')] : []}`, and/or (b) set the annotation
type to "Formula" and enter an invalid formula so the `TextControl` at
`AnnotationLayer.tsx:711-725` receives `validationErrors` when `isValidFormulaAnnotation`
(lines 341-347) returns false.
4. With either field invalid, inspect the DOM for that popover: `TextControl.render` in
`superset-frontend/src/explore/components/controls/TextControl/index.tsx:101-135` calls
`<ControlHeader {...this.props} />`, and `ControlHeader` at
`superset-frontend/src/explore/components/ControlHeader.tsx:167-200` renders a hidden
`<span id={`${name || 'control'}-error`} role="alert">`. The same `TextControl` instance
also renders a visible `<span id={errorId} role="alert">` at
`TextControl/index.tsx:126-133`, where `errorId` is computed as ``${this.props.controlId
|| this.props.name || 'text'}-error`` (line 109). Because `controlId` is undefined and
`name` is set (e.g. `"annotation-layer-name"` or `"annotation-layer-value"`), both spans
share the same id (e.g. `annotation-layer-name-error`), while the input's
`aria-describedby` attribute (line 123) points to this non-unique id. This produces
duplicate DOM ids and makes the screen reader's association of the field with its error
text ambiguous.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/controls/TextControl/index.tsx
**Line:** 109:109
**Comment:**
*Logic Error: `ControlHeader` already creates an error element with id `${name}-error`, and this new code reuses the same id for another `<span>`, which creates duplicate DOM ids and makes `aria-describedby` resolution ambiguous. Use a distinct suffix for this input-level error id so the association is deterministic.
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
+40
to
+44
| <Input | ||
| name="roleName" | ||
| data-test="role-name-input" | ||
| aria-required="true" | ||
| /> |
Contributor
There was a problem hiding this comment.
Suggestion: This input only sets aria-required, so when validation fails there is still no programmatic error association for screen readers. Add dynamic aria-invalid and aria-describedby based on the field error state so invalid state is announced and linked to the form error message. [logic error]
Severity Level: Major ⚠️
- ⚠️ Roles Add/Edit modals lack error association for Role Name.
- ⚠️ Screen reader users get unlinked 'Role name is required' error.
- ⚠️ WCAG 3.3.1 compliance for Roles form is incomplete.
Suggested change
| <Input | |
| name="roleName" | |
| data-test="role-name-input" | |
| aria-required="true" | |
| /> | |
| {(_, meta) => { | |
| const hasError = meta.errors.length > 0; | |
| return ( | |
| <Input | |
| name="roleName" | |
| data-test="role-name-input" | |
| aria-required="true" | |
| aria-invalid={hasError || undefined} | |
| aria-describedby={hasError ? 'roleName_help' : undefined} | |
| /> | |
| ); | |
| }} |
Steps of Reproduction ✅
1. Navigate to the Roles list page at route `/roles/`, which is wired in
`superset-frontend/src/views/routes.tsx:353-356` to load `RolesList` from
`superset-frontend/src/pages/RolesList/index.tsx`.
2. On the Roles list page, click the "Add Role" button (configured in `RolesList` at
`superset-frontend/src/pages/RolesList/index.tsx:320-328`) to open `RoleListAddModal`
(`superset-frontend/src/features/roles/RoleListAddModal.tsx:29-72`), which renders
`<RoleNameField />` (`superset-frontend/src/features/roles/RoleFormItems.tsx:34-45`)
inside a `FormModal`
(`packages/superset-ui-core/src/components/Modal/FormModal.tsx:26-37,108-117`).
3. In the Add Role modal, focus the "Role Name" input rendered by `RoleNameField`
(`superset-frontend/src/features/roles/RoleFormItems.tsx:34-45`), then tab away or attempt
to submit with the field empty so that the `FormItem` validation rule `{ required: true,
message: t('Role name is required') }` at line 38 is violated and an error message is
shown by Ant Design's `Form.Item`.
4. Inspect the rendered input element corresponding to the JSX at
`superset-frontend/src/features/roles/RoleFormItems.tsx:40-44`:
`<Input name="roleName" data-test="role-name-input" aria-required="true" />`. Unlike
other controls that explicitly wire error state into ARIA (e.g. `TextControl` at
`superset-frontend/src/explore/components/controls/TextControl/index.tsx:122-124` where
`aria-invalid` and `aria-describedby` are set when `hasErrors` is true, or
`ModalFormField` at
`superset-frontend/src/components/Modal/ModalFormField.tsx:15-22,37-40` which clones
children with `'aria-invalid': true` and `'aria-describedby': errorId` tied to an error
element with `role="alert"`), this `RoleName` input never receives `aria-invalid` or
`aria-describedby` based on the `FormItem` error state.
5. Because the "Role Name is required" message rendered by the form is not
programmatically associated with the input via `aria-describedby`, and the input itself is
never marked `aria-invalid`, assistive technologies cannot reliably announce that the
error belongs to this specific field, leaving the Role Name field out of the WCAG
3.3.1-compliant pattern implemented elsewhere in the codebase.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/roles/RoleFormItems.tsx
**Line:** 40:44
**Comment:**
*Logic Error: This input only sets `aria-required`, so when validation fails there is still no programmatic error association for screen readers. Add dynamic `aria-invalid` and `aria-describedby` based on the field error state so invalid state is announced and linked to the form error message.
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.
| const markBlurred = (field: string) => | ||
| setBlurred(prev => ({ ...prev, [field]: true })); | ||
| const fieldError = (field: string, value: string | number | undefined) => | ||
| blurred[field] && !value; |
Contributor
There was a problem hiding this comment.
Suggestion: The required-field check uses a plain truthy test (!value), so whitespace-only input (for example " ") is treated as valid and no error message/ARIA invalid state is shown. This causes empty-but-spaced values to bypass the new validation UX and can lead to failed submissions without proper inline error identification. Treat trimmed empty strings as invalid. [logic error]
Severity Level: Major ⚠️
- ❌ SSH tunnel SSH Host/Username fail WCAG error identification.
- ⚠️ Screen readers miss inline errors for whitespace-only SSH fields.
- ⚠️ DatabaseModal SSH tunnel validation inconsistent with required markers.
Suggested change
| blurred[field] && !value; | |
| blurred[field] && | |
| (value === undefined || | |
| (typeof value === 'string' && value.trim().length === 0)); |
Steps of Reproduction ✅
1. Render the database creation/edit flow so that `DatabaseModal` is mounted and the SSH
tunneling UI is visible; `DatabaseModal` at
`superset-frontend/src/features/databases/DatabaseModal/index.tsx:1880-1889` renders
`<SSHTunnelContainer>{renderSSHTunnelForm()}</SSHTunnelContainer>` when `useSSHTunneling`
is true, and `renderSSHTunnelForm` at lines `1768-1807` returns the `<SSHTunnelForm />`
component.
2. In `SSHTunnelForm`
(`superset-frontend/src/features/databases/DatabaseModal/SSHTunnelForm.tsx:55-103`), focus
the "SSH Host" input rendered at lines `79-96` and type only whitespace (e.g. `" "`), then
tab out to blur; the `onChange={onSSHTunnelParametersChange}` handler wired up in
`DatabaseModal/index.tsx:23-31` dispatches `ActionType.ParametersSSHTunnelChange`, and the
reducer case at `DatabaseModal/index.tsx:28-35` stores the literal whitespace string into
`db.ssh_tunnel.server_address` without trimming.
3. The blur event on the SSH Host field calls `onBlur={() =>
markBlurred('server_address')}` (`SSHTunnelForm.tsx:85`), which sets
`blurred.server_address = true` via `markBlurred` at lines `66-67`; on re-render,
`fieldError('server_address', db?.ssh_tunnel?.server_address)` defined at
`SSHTunnelForm.tsx:68-69` evaluates `blurred['server_address'] && !value`, and for a value
of `" "`, the `!value` part is `false`, so `fieldError` returns `false` even though the
field is effectively empty.
4. Because `fieldError` is `false`, the `aria-invalid` and `aria-describedby` attributes
on the SSH Host input (set at `SSHTunnelForm.tsx:86-93`) remain `undefined`, and the
inline error `<span id="server-address-error" role="alert">` at lines `97-101` is not
rendered; the same pattern applies to the "Username" field at `SSHTunnelForm.tsx:141-162`,
so whitespace-only values bypass the required-field error UX and associated ARIA error
identification, despite being semantically empty.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/databases/DatabaseModal/SSHTunnelForm.tsx
**Line:** 69:69
**Comment:**
*Logic Error: The required-field check uses a plain truthy test (`!value`), so whitespace-only input (for example `" "`) is treated as valid and no error message/ARIA invalid state is shown. This causes empty-but-spaced values to bypass the new validation UX and can lead to failed submissions without proper inline error identification. Treat trimmed empty strings as invalid.
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
+185
to
+200
| 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>{' '} |
Contributor
There was a problem hiding this comment.
🟠 Architect Review — HIGH
ControlHeader creates a visually hidden error with id ${name || 'control'}-error and role="alert", while NumberControl and TextControl also render their own error elements with id ${name}-error and role="alert" and set aria-describedby on the input to that id, resulting in duplicate IDs and multiple live regions for the same error.
Suggestion: Ensure each control has a single unique error container and ID by centralizing error ID generation (e.g., letting ControlHeader accept an errorId and not render its own when the control renders one), so inputs reference exactly one live-region error element without duplicating IDs.
| const hasErrors = | ||
| rest.validationErrors && rest.validationErrors.length > 0; | ||
| const errorId = hasErrors | ||
| ? `${rest.name || 'number-control'}-error` |
Contributor
There was a problem hiding this comment.
Suggestion: The generated errorId collides with the ControlHeader error element id (${name}-error) when validation errors exist, producing duplicate DOM ids and making aria-describedby resolution ambiguous. Use a distinct id suffix for the inline NumberControl error element. [logic error]
Severity Level: Major ⚠️
- ⚠️ ECharts Pie NumberControl can render duplicate error element ids.
- ⚠️ aria-describedby may bind to unintended hidden error element.
Suggested change
| ? `${rest.name || 'number-control'}-error` | |
| ? `${rest.name || 'number-control'}-inline-error` |
Steps of Reproduction ✅
1. In the ECharts Pie chart control panel at
`plugins/plugin-chart-echarts/src/Pie/controlPanel.tsx:11-23`, the `threshold_for_other`
control is configured with `type: 'NumberControl'` and `name: 'threshold_for_other'`, so
Explore will render a `NumberControl` with `name="threshold_for_other"`.
2. `ControlPanelsContainer` at `src/explore/components/ControlPanelsContainer.tsx:680-36`
clones this control element, injecting `validationErrors` from the Redux control state
into the `NumberControl` props and thereby into `ControlHeader` via `<ControlHeader
{...rest} />` in `NumberControl`.
3. When any validator or `externalValidationErrors` populates `validationErrors` for
`threshold_for_other` through `getControlState` at
`src/explore/controlUtils/getControlState.ts:4-17`, `ControlHeader` at
`src/explore/components/ControlHeader.tsx:167-199` renders a hidden live-region span with
`id={`${name || 'control'}-error`}`, which for this control is
`id="threshold_for_other-error"`.
4. At the same time, `NumberControl` at
`src/explore/components/controls/NumberControl/index.tsx:74-79` computes `errorId` as
``${rest.name || 'number-control'}-error`` (also `"threshold_for_other-error"`) and uses
it both for `aria-describedby={errorId}` on the `<FullWidthInputNumber>` and for the
visible inline `<span id={errorId} role="alert">`, resulting in two distinct DOM elements
with the same id `"threshold_for_other-error"` and making the `aria-describedby` reference
ambiguous once NumberControl validation is used in real charts.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/controls/NumberControl/index.tsx
**Line:** 77:77
**Comment:**
*Logic Error: The generated `errorId` collides with the `ControlHeader` error element id (`${name}-error`) when validation errors exist, producing duplicate DOM ids and making `aria-describedby` resolution ambiguous. Use a distinct id suffix for the inline NumberControl error element.
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.| {hasErrors && ( | ||
| <span | ||
| id={errorId} | ||
| role="alert" |
Contributor
There was a problem hiding this comment.
Suggestion: This component already renders an alert in ControlHeader for the same validation message, so adding another role="alert" here causes duplicate live-region announcements in screen readers. Keep the inline text but remove the extra alert role to avoid repeated error announcements. [possible bug]
Severity Level: Major ⚠️
- ⚠️ NumberControl error messages may be announced twice to users.
- ⚠️ Duplicate alerts reduce clarity of Explore form a11y feedback.
Suggested change
| role="alert" |
Steps of Reproduction ✅
1. Any Explore control using `NumberControl` (for example `threshold_for_other` in
`plugins/plugin-chart-echarts/src/Pie/controlPanel.tsx:11-23` or `initialTreeDepth` in
`plugins/plugin-chart-echarts/src/Tree/controlPanel.tsx:9-21`) is rendered by
`ControlPanelsContainer` at `src/explore/components/ControlPanelsContainer.tsx:680-36`,
which passes `validationErrors` into the control props.
2. When `validationErrors` for that NumberControl become non-empty via the standard
validation pipeline in `getControlState`
(`src/explore/controlUtils/getControlState.ts:4-17`), `ControlHeader` at
`src/explore/components/ControlHeader.tsx:167-199` renders a screen-reader-only `<span
id={`${name || 'control'}-error`} role="alert">` containing `validationErrors.join('. ')`,
creating an assertive live region for the error message.
3. For the same control instance, `NumberControl` at
`src/explore/components/controls/NumberControl/index.tsx:80-105` also checks `hasErrors`
and, when true, renders an inline `<span id={errorId} role="alert">` whose text is
`rest.validationErrors!.join('. ')`, i.e. the same error message content as
`ControlHeader`.
4. With both `ControlHeader` and `NumberControl` rendering `role="alert"` spans for the
same `validationErrors` message, assistive technologies (e.g. NVDA or VoiceOver) will
announce the identical error text twice when the error appears—once for the hidden header
live region and once for the inline live region—leading to duplicate, noisy announcements
while configuring charts that use `NumberControl`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/controls/NumberControl/index.tsx
**Line:** 100:100
**Comment:**
*Possible Bug: This component already renders an alert in `ControlHeader` for the same validation message, so adding another `role="alert"` here causes duplicate live-region announcements in screen readers. Keep the inline text but remove the extra alert role to avoid repeated error announcements.
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
+137
to
+143
| if (index === 0 && isValidElement(child)) { | ||
| return cloneElement(child as React.ReactElement<any>, { | ||
| 'aria-invalid': true, | ||
| 'aria-describedby': errorId, | ||
| }); | ||
| } | ||
| return child; |
Contributor
There was a problem hiding this comment.
Suggestion: The accessibility attributes are currently injected only into the first direct child, but many call sites pass wrappers like FormItem as that child, so the real input never receives aria-invalid/aria-describedby. This causes error association to fail at runtime for wrapped fields. Apply the attributes to the wrapped control element when the first child is a wrapper with a single valid child. [logic error]
Severity Level: Critical 🚨
- ⚠️ Dashboard name field lacks ARIA linkage to error.
- ⚠️ Screen readers may not announce dashboard name validation errors.
- ⚠️ WCAG 3.3.1 coverage incomplete for wrapped dashboard inputs.
Suggested change
| if (index === 0 && isValidElement(child)) { | |
| return cloneElement(child as React.ReactElement<any>, { | |
| 'aria-invalid': true, | |
| 'aria-describedby': errorId, | |
| }); | |
| } | |
| return child; | |
| if (index !== 0 || !isValidElement(child)) { | |
| return child; | |
| } | |
| const ariaProps = { | |
| 'aria-invalid': true, | |
| 'aria-describedby': errorId, | |
| }; | |
| const wrappedChild = (child as React.ReactElement<any>).props?.children; | |
| if (isValidElement(wrappedChild)) { | |
| return cloneElement(child as React.ReactElement<any>, { | |
| children: cloneElement(wrappedChild as React.ReactElement<any>, ariaProps), | |
| }); | |
| } | |
| return cloneElement(child as React.ReactElement<any>, ariaProps); |
Steps of Reproduction ✅
1. Open any dashboard in the Superset UI and launch the Dashboard Properties modal
(component `PropertiesModal` at
`superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:16-27`), e.g., via
the "Edit dashboard properties" action in the dashboard header.
2. In the "General information" section of this modal (rendered by `Collapse` items at
`index.tsx:8-23` which include `<BasicInfoSection ... />` at `index.tsx:19-22`), locate
the "Name" field implemented in `BasicInfoSection`
(`superset-frontend/src/dashboard/components/PropertiesModal/sections/BasicInfoSection.tsx:40-63`).
This field wraps the actual input inside a `FormItem`:
- `<ModalFormField ... error={hasError ? t('Dashboard name is required') : undefined}>`
(`BasicInfoSection.tsx:40-45`)
- First child: `<FormItem ...><Input ... /></FormItem>` (`BasicInfoSection.tsx:46-62`).
3. With the modal open, clear the "Name" input so it is empty, then trigger validation
(e.g., click the Save/Apply button so `validationStatus.basic?.hasErrors` becomes true and
`error` is passed into `ModalFormField` as shown above). At this point `ModalFormField`
(`superset-frontend/src/components/Modal/ModalFormField.tsx:119-145`) executes:
- Computes `errorId` and then `enhancedChildren = error ? Children.map(children, ...) :
children;` (`ModalFormField.tsx:131-135`).
- For the first child (the `FormItem` wrapper), the `if (index === 0 &&
isValidElement(child))` branch returns `cloneElement(child, { 'aria-invalid': true,
'aria-describedby': errorId })` (`ModalFormField.tsx:137-141`), so the ARIA attributes
are attached to the `FormItem` React element, not its `<Input>` child.
4. Inspect the rendered DOM for the "Name" input in browser devtools: the `<input>`
element rendered by `Input` (child of `FormItem`) does not have `aria-invalid="true"` or
`aria-describedby="<errorId>"`, while those attributes are applied to the DOM node
rendered for `FormItem` (a wrapper element). As a result, the error message container
`<div class="error" id={errorId} role="alert">` (`ModalFormField.tsx:156-159`) is not
referenced by the actual `<input>` via `aria-describedby`, so assistive technologies may
not correctly associate the error with the input, even though the same `ModalFormField`
behaves correctly for direct children like `<Input />` in other call sites (e.g.,
`AlertReportModal` at `superset-frontend/src/features/alerts/AlertReportModal.tsx:20-42,
2100-2219` where no `FormItem` wrapper is used).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Modal/ModalFormField.tsx
**Line:** 137:143
**Comment:**
*Logic Error: The accessibility attributes are currently injected only into the first direct child, but many call sites pass wrappers like `FormItem` as that child, so the real input never receives `aria-invalid`/`aria-describedby`. This causes error association to fail at runtime for wrapped fields. Apply the attributes to the wrapped control element when the first child is a wrapper with a single valid child.
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
+2123
to
+2124
| validationStatus[Sections.General].hasErrors && | ||
| !currentAlert?.name?.length |
Contributor
There was a problem hiding this comment.
Suggestion: The name error is evaluated before form state is initialized, so in edit mode the field renders an error alert while data is still loading. This causes a false role="alert" announcement and incorrect error state before the existing alert data is applied. Gate this check on currentAlert being initialized. [logic error]
Severity Level: Major ⚠️
- ⚠️ Edit Alert modal shows name error before data loads.
- ⚠️ Screen readers announce name-required error incorrectly.
- ⚠️ Affects editing existing Alerts in AlertReportModal.
- ⚠️ Also affects editing existing Reports via same modal.
Suggested change
| validationStatus[Sections.General].hasErrors && | |
| !currentAlert?.name?.length | |
| currentAlert && | |
| validationStatus[Sections.General].hasErrors && | |
| !currentAlert.name?.length |
Steps of Reproduction ✅
1. In tests, render the modal in edit mode using `render(<AlertReportModal
{...generateMockedProps(false, true, false)} />, { useRedux: true });` as in
`superset-frontend/src/features/alerts/AlertReportModal.test.tsx:121-131`, which passes a
non-null `alert` prop and `show: true`.
2. On initial mount, `currentAlert` is `undefined` (initialized via
`useState<Partial<AlertObject> | null>()` at `AlertReportModal.tsx:501`), while
`isEditMode` is `true` (`isEditMode = alert !== null` at `AlertReportModal.tsx:598`). The
initialization effect at `AlertReportModal.tsx:1865-1875` calls `fetchResource(alert.id)`
but does not set `currentAlert`, so it remains `undefined` while the API request is in
flight.
3. The validation effect at `AlertReportModal.tsx:2009-2029` then runs and calls
`validateAll()`, which calls `validateGeneralSection()` at
`AlertReportModal.tsx:1742-1751`. Because `currentAlert` is still `undefined`, both `if
(!currentAlert?.name?.length)` and `if (!currentAlert?.owners?.length)` evaluate to true,
and `updateValidationStatus` sets `validationStatus[Sections.General].hasErrors` to
`true`.
4. Before the resource-driven effect at `AlertReportModal.tsx:1908-1999` has populated
`currentAlert` with the fetched alert, React re-renders and the JSX for the General
section at `AlertReportModal.tsx:70-80` computes the name field error as
`validationStatus[Sections.General].hasErrors && !currentAlert?.name?.length ? ... :
undefined`. With `hasErrors === true` and `currentAlert` still `undefined`,
`!currentAlert?.name?.length` is also truthy (`!undefined`), so the "Alert name is
required"/"Report name is required" message is rendered in the `ModalFormField` error
container (which in this PR is used as an ARIA alert), causing a premature error state and
announcement before the actual alert name from the server is loaded.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/alerts/AlertReportModal.tsx
**Line:** 2123:2124
**Comment:**
*Logic Error: The name error is evaluated before form state is initialized, so in edit mode the field renders an error alert while data is still loading. This causes a false `role="alert"` announcement and incorrect error state before the existing alert data is applied. Gate this check on `currentAlert` being initialized.
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
+2146
to
+2147
| validationStatus[Sections.General].hasErrors && | ||
| !currentAlert?.owners?.length |
Contributor
There was a problem hiding this comment.
Suggestion: The owners error condition also runs before currentAlert is initialized, which can briefly render a required-owner alert while editing an existing alert and data is still loading. This creates a false validation alert and misleading accessibility output. Only evaluate this error after currentAlert is available. [logic error]
Severity Level: Major ⚠️
- ⚠️ Edit Alert modal shows owners error during loading.
- ⚠️ Screen readers hear false owners-required validation message.
- ⚠️ Impacts Alerts and Reports ownership validation UX.
- ⚠️ Confusing accessibility output for valid existing alerts.
Suggested change
| validationStatus[Sections.General].hasErrors && | |
| !currentAlert?.owners?.length | |
| currentAlert && | |
| validationStatus[Sections.General].hasErrors && | |
| !currentAlert.owners?.length |
Steps of Reproduction ✅
1. In tests, render the modal in edit mode with an existing alert using
`render(<AlertReportModal {...generateMockedProps(false, true, false)} />, { useRedux:
true });` as in `superset-frontend/src/features/alerts/AlertReportModal.test.tsx:121-131`,
which sets `alert` to a populated `validAlert` (with owners) and `show: true`.
2. On initial mount, `currentAlert` is still `undefined` (state declared at
`AlertReportModal.tsx:501`), while `isEditMode` is `true`. The initialization effect at
`AlertReportModal.tsx:1865-1875` triggers `fetchResource(alert.id)` but does not seed
`currentAlert` from `alert`, so until the API responds, `currentAlert` remains
`undefined`.
3. The validation effect at `AlertReportModal.tsx:2009-2029` runs and calls
`validateAll()`, which invokes `validateGeneralSection()` at
`AlertReportModal.tsx:1742-1751`. With `currentAlert` still `undefined`, `if
(!currentAlert?.owners?.length)` evaluates to true and `updateValidationStatus` marks
`validationStatus[Sections.General].hasErrors` as `true`, even though the saved alert will
eventually have at least one owner.
4. Before the resource effect at `AlertReportModal.tsx:1908-1999` has set `currentAlert`
from the fetched `resource`, React re-renders and the Owners `ModalFormField` at
`AlertReportModal.tsx:93-101` evaluates its `error` prop as
`validationStatus[Sections.General].hasErrors && !currentAlert?.owners?.length ? t('At
least one owner is required') : undefined`; with `hasErrors === true` and `currentAlert`
still `undefined`, `!currentAlert?.owners?.length` is truthy, so the "At least one owner
is required" error is rendered and (in this PR) announced via the alert/error container
even though the existing owners have not yet been loaded into the form.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/alerts/AlertReportModal.tsx
**Line:** 2146:2147
**Comment:**
*Logic Error: The owners error condition also runs before `currentAlert` is initialized, which can briefly render a required-owner alert while editing an existing alert and data is still loading. This creates a false validation alert and misleading accessibility output. Only evaluate this error after `currentAlert` is available.
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.![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #f7d2d1
Actionable Suggestions - 2
-
superset-frontend/src/explore/components/controls/NumberControl/index.tsx - 1
- Invalid HTML id when name is undefined · Line 94-95
-
superset-frontend/src/explore/components/ControlHeader.tsx - 1
- Duplicate HTML IDs in Error Handling · Line 184-200
Additional Suggestions - 4
-
superset-frontend/src/features/databases/DatabaseModal/SSHTunnelForm.tsx - 1
-
Custom CSS in error display · Line 97-101The error messages use inline styles (color: 'red', fontSize: '12px'), which violates the guideline to avoid custom CSS and follow antd best practices. Consider wrapping the Input components in Form.Item with validateStatus='error' and help props to leverage antd's built-in error styling and accessibility features, removing the need for manual aria attributes and custom spans.
-
-
superset-frontend/src/features/databases/DatabaseModal/SqlAlchemyForm.tsx - 2
-
Custom inline style for error color · Line 73-73The error message div uses an inline style for color, which violates the guideline to avoid custom CSS and follow antd best practices. Consider using the emotion css prop with theme.colorError instead, matching the pattern used elsewhere in the component like the Button's css prop.
Code suggestion
diff --git a/superset-frontend/src/features/databases/DatabaseModal/SqlAlchemyForm.tsx b/superset-frontend/src/features/databases/DatabaseModal/SqlAlchemyForm.tsx index 1234567..abcdef0 100644 --- a/superset-frontend/src/features/databases/DatabaseModal/SqlAlchemyForm.tsx +++ b/superset-frontend/src/features/databases/DatabaseModal/SqlAlchemyForm.tsx @@ -72,7 +72,7 @@ {nameError && ( <div className="helper" id="database-name-error" role="alert" style={{ color: 'var(--ifm-color-danger, #e04f5f)' }}> {t('Display Name is required')} </div> )} + {nameError && ( + <div className="helper" id="database-name-error" role="alert" css={(theme: SupersetTheme) => `color: ${theme.colorError};`}> + {t('Display Name is required')} + </div> + )}
-
Custom inline style for error color · Line 103-103Similar to the name error, this URI error message uses an inline style for color. Replace with emotion css prop using theme.colorError to align with antd best practices and avoid custom CSS.
Code suggestion
diff --git a/superset-frontend/src/features/databases/DatabaseModal/SqlAlchemyForm.tsx b/superset-frontend/src/features/databases/DatabaseModal/SqlAlchemyForm.tsx index 1234567..abcdef0 100644 --- a/superset-frontend/src/features/databases/DatabaseModal/SqlAlchemyForm.tsx +++ b/superset-frontend/src/features/databases/DatabaseModal/SqlAlchemyForm.tsx @@ -102,7 +102,7 @@ {uriError && ( <div className="helper" id="sqlalchemy-uri-error" role="alert" style={{ color: 'var(--ifm-color-danger, #e04f5f)' }}> {t('SQLAlchemy URI is required')} </div> )} + {uriError && ( + <div className="helper" id="sqlalchemy-uri-error" role="alert" css={(theme: SupersetTheme) => `color: ${theme.colorError};`}> + {t('SQLAlchemy URI is required')} + </div> + )}
-
-
superset-frontend/src/features/roles/RoleFormItems.tsx - 1
-
Redundant aria-required attribute · Line 40-44The aria-required="true" attribute appears redundant here, since antd's FormItem with required: true automatically adds aria-required to the input for accessibility. This manual addition is inconsistent with other similar FormItem usages in the codebase (e.g., UserInfoModal.tsx, ApiKeyCreateModal.tsx), which rely on antd's automatic handling. Removing it aligns with existing patterns and avoids potential duplication.
-
Review Details
-
Files reviewed - 14 · Commit Range:
2256819..2256819- superset-frontend/packages/superset-core/src/components/Alert/Alert.test.tsx
- superset-frontend/packages/superset-core/src/components/Alert/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/Form/LabeledErrorBoundInput.tsx
- superset-frontend/src/components/ErrorMessage/BasicErrorAlert.test.tsx
- superset-frontend/src/components/ErrorMessage/BasicErrorAlert.tsx
- superset-frontend/src/components/Modal/ModalFormField.tsx
- superset-frontend/src/explore/components/ControlHeader.tsx
- superset-frontend/src/explore/components/controls/NumberControl/index.tsx
- superset-frontend/src/explore/components/controls/TextControl/index.tsx
- superset-frontend/src/features/alerts/AlertReportModal.tsx
- superset-frontend/src/features/alerts/components/NumberInput.tsx
- superset-frontend/src/features/databases/DatabaseModal/SSHTunnelForm.tsx
- superset-frontend/src/features/databases/DatabaseModal/SqlAlchemyForm.tsx
- superset-frontend/src/features/roles/RoleFormItems.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Comment on lines
+94
to
+95
| aria-describedby={errorId} | ||
| id={rest.name} |
Contributor
There was a problem hiding this comment.
Invalid HTML id when name is undefined
The id attribute is set to rest.name, which can be undefined per the ControlHeaderProps type, resulting in an invalid HTML id="undefined". Provide a fallback to ensure a valid id.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| aria-describedby={errorId} | |
| id={rest.name} | |
| aria-describedby={errorId} | |
| id={rest.name || 'number-control'} |
Code Review Run #f7d2d1
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Comment on lines
+184
to
+200
| <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>{' '} |
Contributor
There was a problem hiding this comment.
Duplicate HTML IDs in Error Handling
The added screen reader span improves accessibility by announcing validation errors, but the id attribute creates duplicate HTML ids when used with controls like NumberControl that already define an error span with the same id. This violates HTML uniqueness rules and could confuse assistive technologies. Removing the id maintains the alert functionality while fixing the conflict.
Code suggestion
Check the AI-generated fix before applying
- <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>{' '}
+ <span
+ 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>{' '}'
Code Review Run #f7d2d1
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Contributor
There was a problem hiding this comment.
Code Review Agent Run #20ee56
Actionable Suggestions - 1
-
superset-frontend/src/features/alerts/AlertReportModal.tsx - 1
- Validation Logic Inconsistency · Line 2123-2125
Review Details
-
Files reviewed - 2 · Commit Range:
2256819..3f2a60f- superset-frontend/src/explore/components/ControlHeader.tsx
- superset-frontend/src/features/alerts/AlertReportModal.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Comment on lines
+2123
to
+2125
| currentAlert && | ||
| validationStatus[Sections.General].hasErrors && | ||
| !currentAlert.name?.length |
Contributor
There was a problem hiding this comment.
Validation Logic Inconsistency
The error display logic now differs from the validation logic. When currentAlert is null/undefined, validation treats it as an error but the UI won't show the message, potentially disabling save without explanation.
Code Review Run #20ee56
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Aitema-gmbh
pushed a commit
to Aitema-gmbh/superset
that referenced
this pull request
Apr 19, 2026
… ModalFormField traversal Address Bito + CodeAnt review findings on PR apache#39245: ControlHeader / NumberControl / TextControl - ControlHeader accepts an external errorId so wrappers share one DOM id per control. Previously both ControlHeader and the input wrapper rendered id="${name}-error" + role="alert", producing duplicate ids and duplicate screen-reader announcements for the same validation error. - NumberControl now computes a stable input id via useId when name is missing, so id="undefined" never hits the DOM. - The visible inline red error below the input keeps the text but drops its id and role="alert" — the live-region inside ControlHeader is the single source of truth for assistive tech. ModalFormField - Injecting aria-invalid / aria-describedby on the first child missed FormItem-wrapped inputs, leaving the real interactive element unlinked to the error. The traversal now hops through a single-child wrapper to decorate the wrapped input. SSHTunnelForm - Required-field check treated whitespace-only input as valid, letting users bypass validation with a space. The check now trims before the emptiness test. Already addressed in prior commit 3f2a60f on this branch: - AlertReportModal name/owners error guards against currentAlert loading - ControlHeader useId fallback for missing name Not changed: - RoleNameField — Ant Design FormItem with rules={[{required:true}]} already sets aria-invalid on the input via FieldContext when validation fails; the explicit render-prop pattern the bot suggested would nest FormItems for no behavior gain.
Contributor
Author
|
Thanks @bito-code-review @codeant-ai-for-open-source — the duplicate-id / duplicate-
Already addressed by prior follow-up One finding not taken: CodeAnt suggested wrapping |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Code Review Agent Run #38f541
Actionable Suggestions - 1
-
superset-frontend/src/explore/components/controls/TextControl/index.tsx - 1
- Duplicate error display · Line 109-115
Review Details
-
Files reviewed - 5 · Commit Range:
3f2a60f..9d0f14c- superset-frontend/src/components/Modal/ModalFormField.tsx
- superset-frontend/src/explore/components/ControlHeader.tsx
- superset-frontend/src/explore/components/controls/NumberControl/index.tsx
- superset-frontend/src/explore/components/controls/TextControl/index.tsx
- superset-frontend/src/features/databases/DatabaseModal/SSHTunnelForm.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Comment on lines
+109
to
+115
| const inputId = this.props.controlId || this.props.name; | ||
| // WCAG 3.3.1: share a single error container id with ControlHeader so | ||
| // the input's aria-describedby resolves to one live-region, not two. | ||
| const errorId = hasErrors && inputId ? `${inputId}-error` : undefined; | ||
| return ( | ||
| <div> | ||
| <ControlHeader {...this.props} /> | ||
| <ControlHeader {...this.props} errorId={errorId} /> |
Contributor
There was a problem hiding this comment.
Duplicate error display
The ControlHeader component displays validation errors when the validationErrors prop is passed, but the new inline span also shows the same error text, resulting in duplicate error messages for users. To resolve this and ensure proper accessibility, omit validationErrors from ControlHeader props and add id and role attributes to the inline error span so it serves as the single error container referenced by aria-describedby.
Code suggestion
Check the AI-generated fix before applying
- let { value } = this.state;
+ let { value } = this.state;
+ const { validationErrors, ...otherProps } = this.props;
@@ -115,1 +116,1 @@
- <ControlHeader {...this.props} errorId={errorId} />
+ <ControlHeader {...otherProps} errorId={errorId} />
@@ -129,1 +130,1 @@
- <span
+ <span id={errorId} role="alert"
style={{ color: 'red', fontSize: '12px', display: 'block', marginTop: '4px' }}
Code Review Run #38f541
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Member
|
Feel free to mark all of the comments that you've addressed as resolved if that's the case.It seems like there's a conflict that needs resolution on this PR, but otherwise I'm inclined to merge it once all of the bot feedback is dealt with. |
|
Hi @rusackas — thanks for the review and the merge signal. Addressing your note on bot feedback and resolving the conflict: Bot triage complete: codeant-ai-for-open-source (9 inline comments):
Rebase/conflict resolution will be done by @Aitema-gmbh locally. Will ping you once the next push lands. |
added 3 commits
May 7, 2026 16:39
…a-invalid, and aria-describedby
… ModalFormField traversal Address Bito + CodeAnt review findings on PR apache#39245: ControlHeader / NumberControl / TextControl - ControlHeader accepts an external errorId so wrappers share one DOM id per control. Previously both ControlHeader and the input wrapper rendered id="${name}-error" + role="alert", producing duplicate ids and duplicate screen-reader announcements for the same validation error. - NumberControl now computes a stable input id via useId when name is missing, so id="undefined" never hits the DOM. - The visible inline red error below the input keeps the text but drops its id and role="alert" — the live-region inside ControlHeader is the single source of truth for assistive tech. ModalFormField - Injecting aria-invalid / aria-describedby on the first child missed FormItem-wrapped inputs, leaving the real interactive element unlinked to the error. The traversal now hops through a single-child wrapper to decorate the wrapped input. SSHTunnelForm - Required-field check treated whitespace-only input as valid, letting users bypass validation with a space. The check now trims before the emptiness test. Already addressed in prior commit 3f2a60f on this branch: - AlertReportModal name/owners error guards against currentAlert loading - ControlHeader useId fallback for missing name Not changed: - RoleNameField — Ant Design FormItem with rules={[{required:true}]} already sets aria-invalid on the input via FieldContext when validation fails; the explicit render-prop pattern the bot suggested would nest FormItems for no behavior gain.
Aitema-gmbh
force-pushed
the
fix/wcag-3.3.1-error-identification
branch
from
9d0f14c to
7938a0d
Compare
…ine error styles Addresses three remaining bot-review items on PR apache#39245: - AlertReportModal: validateGeneralSection now bails early when currentAlert is still null/undefined, mirroring the display-side `currentAlert &&` gate. Previously validation flagged required-field errors during the brief load window, disabling save without showing the error to the user. Validation reruns once setCurrentAlert initializes the form data. - NumberControl, TextControl, SSHTunnelForm: the inline error spans used hardcoded `style={{ color: 'red', fontSize: '12px' }}`. Replaced with a reusable `fieldErrorStyles` helper that pulls colorError, fontSizeSM, and sizeUnit from the SupersetTheme, so error appearance follows theme tokens in dark mode and on custom themes.
Contributor
There was a problem hiding this comment.
Code Review Agent Run #0bd36a
Actionable Suggestions - 1
-
superset-frontend/src/features/databases/DatabaseModal/SSHTunnelForm.tsx - 1
- Inconsistent Error ID Naming · Line 103-170
Review Details
-
Files reviewed - 14 · Commit Range:
b3b0593..c822114- superset-frontend/packages/superset-core/src/components/Alert/Alert.test.tsx
- superset-frontend/packages/superset-core/src/components/Alert/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/Form/LabeledErrorBoundInput.tsx
- superset-frontend/src/components/ErrorMessage/BasicErrorAlert.test.tsx
- superset-frontend/src/components/ErrorMessage/BasicErrorAlert.tsx
- superset-frontend/src/components/Modal/ModalFormField.tsx
- superset-frontend/src/explore/components/ControlHeader.tsx
- superset-frontend/src/explore/components/controls/NumberControl/index.tsx
- superset-frontend/src/explore/components/controls/TextControl/index.tsx
- superset-frontend/src/features/alerts/AlertReportModal.tsx
- superset-frontend/src/features/alerts/components/NumberInput.tsx
- superset-frontend/src/features/databases/DatabaseModal/SSHTunnelForm.tsx
- superset-frontend/src/features/databases/DatabaseModal/SqlAlchemyForm.tsx
- superset-frontend/src/features/roles/RoleFormItems.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Comment on lines
103
to
+170
| @@ -92,8 +123,23 @@ const SSHTunnelForm = ({ | |||
| type="number" | |||
| value={db?.ssh_tunnel?.server_port} | |||
| onChange={onSSHTunnelParametersChange} | |||
| onBlur={() => markBlurred('server_port')} | |||
| aria-invalid={ | |||
| fieldError('server_port', db?.ssh_tunnel?.server_port) || | |||
| undefined | |||
| } | |||
| aria-describedby={ | |||
| fieldError('server_port', db?.ssh_tunnel?.server_port) | |||
| ? 'server-port-error' | |||
| : undefined | |||
| } | |||
| data-test="ssh-tunnel-server_port-input" | |||
| /> | |||
| {fieldError('server_port', db?.ssh_tunnel?.server_port) && ( | |||
| <span id="server-port-error" role="alert" css={fieldErrorStyles}> | |||
| {t('SSH Port is required')} | |||
| </span> | |||
| )} | |||
| </StyledDiv> | |||
| </Col> | |||
| </StyledRow> | |||
| @@ -109,8 +155,22 @@ const SSHTunnelForm = ({ | |||
| placeholder={t('e.g. Analytics')} | |||
| value={db?.ssh_tunnel?.username || ''} | |||
| onChange={onSSHTunnelParametersChange} | |||
| onBlur={() => markBlurred('username')} | |||
| aria-invalid={ | |||
| fieldError('username', db?.ssh_tunnel?.username) || undefined | |||
| } | |||
| aria-describedby={ | |||
| fieldError('username', db?.ssh_tunnel?.username) | |||
| ? 'ssh-username-error' | |||
| : undefined | |||
| } | |||
| data-test="ssh-tunnel-username-input" | |||
| /> | |||
| {fieldError('username', db?.ssh_tunnel?.username) && ( | |||
| <span id="ssh-username-error" role="alert" css={fieldErrorStyles}> | |||
Contributor
There was a problem hiding this comment.
Inconsistent Error ID Naming
Error element IDs for form fields are inconsistently named ('server-address-error', 'server-port-error', 'ssh-username-error'), reducing maintainability in this module-level component. Standardize by removing the 'ssh-' prefix from the username error ID.
Code suggestion
Check the AI-generated fix before applying
- fieldError('username', db?.ssh_tunnel?.username)
- ? 'ssh-username-error'
- : undefined
+ fieldError('username', db?.ssh_tunnel?.username)
+ ? 'username-error'
+ : undefined
@@ -170,1 +170,1 @@
- <span id="ssh-username-error" role="alert" css={fieldErrorStyles}>
+ <span id="username-error" role="alert" css={fieldErrorStyles}>
Code Review Run #0bd36a
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
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
3 participants
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 3.3.1 (Error Identification, Level A).
role="alert"to error containers (BasicErrorAlert, Alert component)aria-invalidandaria-describedbyto form inputs with validation errorsTESTING INSTRUCTIONS
role="alert"aria-invalid="true"andaria-describedbypointing to errorADDITIONAL 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.