Aitema-gmbh commented on code in PR #39245:
URL: https://github.com/apache/superset/pull/39245#discussion_r3232733766
##########
superset-frontend/src/explore/components/controls/TextControl/index.tsx:
##########
@@ -104,6 +104,9 @@ 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 errorId = hasErrors ? `${this.props.controlId || this.props.name ||
'text'}-error` : undefined;
Review Comment:
ADDRESSED: ControlHeader now accepts an `errorId` prop and TextControl
passes the same id it sets on `aria-describedby` (commit 7938a0d, follow-up
f5aa012). There is one error container per control with a unique id, not
duplicates.
##########
superset-frontend/src/features/roles/RoleFormItems.tsx:
##########
@@ -37,7 +37,11 @@ export const RoleNameField = () => (
label={t('Role Name')}
rules={[{ required: true, message: t('Role name is required') }]}
>
- <Input name="roleName" data-test="role-name-input" />
+ <Input
+ name="roleName"
+ data-test="role-name-input"
+ aria-required="true"
+ />
Review Comment:
WONTFIX: `RoleNameField` uses AntD `FormItem` with `rules`, which renders
the error in `<div id="${name}_help">` and automatically applies
`aria-invalid`/`aria-describedby` to the wrapped `Input` via Form context.
Adding render-prop-based wiring on top of that would create duplicate
associations. The PR only added `aria-required` to make the required state
programmatically determinable independent of the visual asterisk; AntD covers
the dynamic invalid state.
##########
superset-frontend/src/features/databases/DatabaseModal/SSHTunnelForm.tsx:
##########
@@ -62,6 +62,11 @@ const SSHTunnelForm = ({
setSSHTunnelLoginMethod: (method: AuthType) => void;
}) => {
const [usePassword, setUsePassword] = useState<AuthType>(AuthType.Password);
+ const [blurred, setBlurred] = useState<Record<string, boolean>>({});
+ const markBlurred = (field: string) =>
+ setBlurred(prev => ({ ...prev, [field]: true }));
+ const fieldError = (field: string, value: string | number | undefined) =>
+ blurred[field] && !value;
Review Comment:
ADDRESSED: `fieldError` now treats whitespace-only strings as empty (`typeof
value === 'string' && value.trim().length === 0`) — see SSHTunnelForm.tsx line
75–80 (commit 7938a0d).
##########
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 and the
input-rendering controls (TextControl, NumberControl) pass a single shared id
(commit 7938a0d). Each control renders one live-region with role="alert" inside
ControlHeader; the inline visible `<span>` inside TextControl/NumberControl has
no role and no id, so screen readers receive a single announcement per control.
--
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]
