Aitema-gmbh commented on code in PR #39245:
URL: https://github.com/apache/superset/pull/39245#discussion_r3232736376
##########
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}
Review Comment:
ADDRESSED: NumberControl now derives `inputId` from `rest.name || useId()`,
so the input always receives a valid id (commit f5aa012, line 72–73). The
collision/undefined-id concern no longer applies.
##########
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:
ADDRESSED: ControlHeader now accepts an `errorId` prop from the calling
control and renders a single live-region with that id (commit 7938a0d,
ControlHeader.tsx lines 77–81 + 193–207). TextControl and NumberControl pass
the same id they put on `aria-describedby`, so there is one error element per
control rather than two.
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -2119,6 +2119,15 @@ const AlertReportModal:
FunctionComponent<AlertReportModalProps> = ({
<ModalFormField
label={isReport ? t('Report name') : t('Alert name')}
required
+ error={
+ currentAlert &&
+ validationStatus[Sections.General].hasErrors &&
+ !currentAlert.name?.length
Review Comment:
ADDRESSED: The validation function and the UI gating use the same
`currentAlert` guard. Both the disable-save flag and the inline error are
skipped while `currentAlert` is still loading, so the save button is not
disabled without an explanation visible to the user (commit c822114). Once
`currentAlert` is initialized, validation and error display run in sync.
##########
superset-frontend/src/explore/components/controls/TextControl/index.tsx:
##########
@@ -104,9 +104,15 @@ export default class TextControl<
this.initialValue = this.props.value;
value = safeStringify(this.props.value);
}
+ const hasErrors =
+ this.props.validationErrors && this.props.validationErrors.length > 0;
+ 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} />
Review Comment:
INVALID: The two error renderings are intentional and complementary, not
duplicates. ControlHeader emits a visually-hidden `<span role="alert"
id="${inputId}-error">` for assistive tech, while TextControl renders the
visible inline error text without role/id. Screen readers receive one
announcement (from ControlHeader), and sighted users see the inline message.
Removing either would break one of the two channels. WCAG 3.3.1 + 4.1.3 both
satisfied.
##########
superset-frontend/src/features/databases/DatabaseModal/SSHTunnelForm.tsx:
##########
@@ -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}>
Review Comment:
WONTFIX: The `ssh-` prefix on `ssh-username-error` is intentional — the
DatabaseModal also has a top-level `username` field for the database itself,
and `username-error` would collide with that. The other two ids
(`server-address-error`, `server-port-error`) are unique to the SSH form, so
they don't need the prefix. Renaming the prefixed one would re-introduce the
collision.
--
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]
