aminghadersohi commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4201535377
##########
superset-frontend/src/explore/components/controls/CheckboxControl.tsx:
##########
@@ -50,14 +53,30 @@ const CheckBoxControlWrapper = styled.div`
export default function CheckboxControl({
value = false,
label,
+ disabled = false,
+ disabledReason,
+ resetLabel,
onChange = () => {},
...restProps
}: CheckboxControlProps): JSX.Element {
+ const explanationId = useId();
const handleChange = useCallback((): void => {
- onChange(!value);
- }, [onChange, value]);
+ if (!disabled) {
+ onChange(!value);
+ }
+ }, [disabled, onChange, value]);
- const checkbox = <Checkbox onChange={handleChange} checked={!!value} />;
+ const checkbox = (
+ <Checkbox
+ onChange={handleChange}
+ checked={!!value}
+ disabled={disabled}
+ aria-describedby={disabled && disabledReason ? explanationId : undefined}
+ />
Review Comment:
Moving the label out of `<Checkbox>` leaves the disabled checkbox with no
accessible name (it had one at 277094100e), so a screen reader announces an
unnamed dimmed checkbox plus the reason; the outer `<label htmlFor={name}>`
matches no id.
```suggestion
aria-describedby={disabled && disabledReason ? explanationId :
undefined}
aria-label={typeof label === 'string' ? label : undefined}
/>
```
##########
superset-frontend/src/explore/components/controls/CheckboxControl.test.tsx:
##########
@@ -49,4 +49,37 @@ describe('CheckboxControl', () => {
await userEvent.click(label);
expect(defaultProps.onChange).toHaveBeenCalled();
});
+
+ test('explains a disabled checkbox without a label', () => {
+ render(
+ setup({
+ label: undefined,
+ disabled: true,
+ disabledReason: 'Unavailable',
+ }),
+ );
+
+ const checkbox = screen.getByRole('checkbox');
+ expect(checkbox).toHaveAccessibleDescription('Unavailable');
+ expect(screen.getByText('Unavailable')).toBeInTheDocument();
+ });
+
+ test('retains the control header and description when disabled', () => {
+ const { container } = render(
+ setup({
+ disabled: true,
+ description: 'Why this control is disabled',
+ hovered: true,
+ renderTrigger: true,
+ }),
+ );
+
+ expect(
+ container.querySelector('[data-test="show_legend-header"]'),
+ ).toBeInTheDocument();
+ expect(
+ container.querySelector('[data-test="show_legend-description-icon"]'),
+ ).toBeInTheDocument();
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+ });
Review Comment:
Disabled label clicks are blocked twice (`onClick` undefined and `if
(!disabled)`); removing both still passes every test, so nothing pins it.
```suggestion
});
test('ignores label clicks while disabled', async () => {
const onChange = jest.fn();
render(setup({ disabled: true, onChange }));
await userEvent.click(screen.getByText('checkbox label'));
expect(onChange).not.toHaveBeenCalled();
});
```
--
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]
