Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pickers] Fix aria-labelledby assignment to dialog #7608

Merged
merged 24 commits into from
Jan 26, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
c27a3ed
Fix `aria-labelledby` on mobile picker dialog paper
LukasTy Jan 18, 2023
c4a695a
Handle case when toolbar is hidden and label is empty
LukasTy Jan 18, 2023
27fd8b0
Handle mobile date range picker
LukasTy Jan 18, 2023
b84a5fc
fix lint issues
LukasTy Jan 18, 2023
d166f5e
Handle desktop pickers
LukasTy Jan 18, 2023
86d1d2f
proptypes
LukasTy Jan 18, 2023
b5eb9b8
Actually handle `titleId` privately
LukasTy Jan 18, 2023
8771a10
fix
LukasTy Jan 18, 2023
074460f
Fix to not post warning for DesktopTimePicker without views
LukasTy Jan 18, 2023
2606211
Update tests to include label and avoid warning being thrown
LukasTy Jan 18, 2023
6a1e69a
Add test cases asserting pickers `aria-labelledby` relationship
LukasTy Jan 18, 2023
75ac81e
Code review: Flavien
LukasTy Jan 19, 2023
fc0348b
Code review: Flavien 2
LukasTy Jan 19, 2023
fabf4e0
Merge remote-tracking branch 'origin/next' into fix-pickers-aria-labe…
LukasTy Jan 19, 2023
026af96
Remove redundant omit
LukasTy Jan 19, 2023
10484bb
Merge remote-tracking branch 'origin/next' into fix-pickers-aria-labe…
LukasTy Jan 21, 2023
3e20a2d
Update refactored test file
LukasTy Jan 21, 2023
7a47004
Merge remote-tracking branch 'origin/next' into fix-pickers-aria-labe…
LukasTy Jan 25, 2023
fcedecf
Fix types
LukasTy Jan 25, 2023
c8c4a75
Code review: Alex
LukasTy Jan 26, 2023
b8f10dd
Remove warnings
LukasTy Jan 26, 2023
b34a2aa
Revert no longer relevant test file changes
LukasTy Jan 26, 2023
9f6532e
Add documentation section regarding `aria-labelledby` behavior
LukasTy Jan 26, 2023
a7de910
Apply suggestions from code review
LukasTy Jan 26, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ DateRangePickerToolbar.propTypes = {
onRangePositionChange: PropTypes.func.isRequired,
rangePosition: PropTypes.oneOf(['end', 'start']).isRequired,
readOnly: PropTypes.bool,
titleId: PropTypes.string,
/**
* Toolbar date format.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ import {
PickersModalDialog,
InferError,
ExportedBaseToolbarProps,
useLocaleText,
} from '@mui/x-date-pickers/internals';
import useId from '@mui/utils/useId';
import { buildWarning } from '@mui/x-date-pickers/internals/utils/warning';
import {
MobileRangePickerAdditionalViewProps,
UseMobileRangePickerParams,
Expand All @@ -24,6 +27,12 @@ import { getReleaseInfo } from '../../utils/releaseInfo';
import { DateRange, RangePosition } from '../../models/range';
import { BaseMultiInputFieldProps } from '../../models/fields';

const ariaLabelledByCantBeResolvedWarning = buildWarning([
'MUI: `aria-labelledby` can not be resolved.',
'Make sure that the picker has `start` or `end` fields in `localeText` or the toolbar is not hidden.',
'Alternatively you can provide a custom `aria-labelledby` to the `mobilePaper` slot props.',
]);

const releaseInfo = getReleaseInfo();

export const useMobileRangePicker = <
Expand All @@ -39,7 +48,7 @@ export const useMobileRangePicker = <

const {
slots,
slotProps,
slotProps: innerSlotProps,
className,
sx,
format,
Expand All @@ -50,6 +59,8 @@ export const useMobileRangePicker = <
} = props;

const [rangePosition, setRangePosition] = React.useState<RangePosition>('start');
const labelId = useId();
const contextLocaleText = useLocaleText();

const {
open,
Expand Down Expand Up @@ -93,7 +104,7 @@ export const useMobileRangePicker = <
InferError<TExternalProps>
> = useSlotProps({
elementType: Field,
externalSlotProps: slotProps?.field,
externalSlotProps: innerSlotProps?.field,
additionalProps: {
...pickerFieldProps,
readOnly: readOnly ?? true,
Expand All @@ -110,10 +121,12 @@ export const useMobileRangePicker = <
...fieldProps.slots,
};

const isToolbarHidden = innerSlotProps?.toolbar?.hidden ?? false;

const slotPropsForField: BaseMultiInputFieldProps<DateRange<TDate>, unknown>['slotProps'] = {
...fieldProps.slotProps,
textField: (ownerState) => {
const externalInputProps = resolveComponentProps(slotProps?.textField, ownerState);
const externalInputProps = resolveComponentProps(innerSlotProps?.textField, ownerState);
const inputPropsPassedByField = resolveComponentProps(
fieldProps.slotProps?.textField,
ownerState,
Expand All @@ -122,6 +135,7 @@ export const useMobileRangePicker = <
ownerState.position === 'start' ? fieldSlotProps.startInput : fieldSlotProps.endInput;

return {
...(isToolbarHidden && { id: `${labelId}-${ownerState.position}` }),
...externalInputProps,
...inputPropsPassedByField,
...inputPropsPassedByPicker,
Expand All @@ -132,7 +146,7 @@ export const useMobileRangePicker = <
};
},
root: (ownerState) => {
const externalRootProps = resolveComponentProps(slotProps?.fieldRoot, ownerState);
const externalRootProps = resolveComponentProps(innerSlotProps?.fieldRoot, ownerState);
const rootPropsPassedByField = resolveComponentProps(fieldProps.slotProps?.root, ownerState);
return {
...externalRootProps,
Expand All @@ -141,7 +155,10 @@ export const useMobileRangePicker = <
};
},
separator: (ownerState) => {
const externalSeparatorProps = resolveComponentProps(slotProps?.fieldSeparator, ownerState);
const externalSeparatorProps = resolveComponentProps(
innerSlotProps?.fieldSeparator,
ownerState,
);
const separatorPropsPassedByField = resolveComponentProps(
fieldProps.slotProps?.separator,
ownerState,
Expand All @@ -155,16 +172,47 @@ export const useMobileRangePicker = <
};

const slotPropsForLayout: PickersLayoutSlotsComponentsProps<DateRange<TDate>, TDate, TView> = {
...slotProps,
...innerSlotProps,
toolbar: {
...slotProps?.toolbar,
...innerSlotProps?.toolbar,
titleId: labelId,
rangePosition,
onRangePositionChange: setRangePosition,
} as ExportedBaseToolbarProps,
};

const Layout = slots?.layout ?? PickersLayout;

const finalLocaleText = {
...contextLocaleText,
...localeText,
};
let labelledById = labelId;
if (isToolbarHidden) {
if (!finalLocaleText?.start && !finalLocaleText?.end) {
labelledById = undefined;
} else {
if (finalLocaleText.start) {
labelledById = `${labelId}-start-label`;
}
if (finalLocaleText.end) {
labelledById += ` ${labelId}-end-label`;
}
}
LukasTy marked this conversation as resolved.
Show resolved Hide resolved
}
if (process.env.NODE_ENV !== 'production') {
if (!labelledById && !innerSlotProps?.mobilePaper?.['aria-labelledby']) {
ariaLabelledByCantBeResolvedWarning();
}
}
const slotProps = {
...innerSlotProps,
mobilePaper: {
'aria-labelledby': labelledById,
...innerSlotProps?.mobilePaper,
},
};

const renderPicker = () => (
<LocalizationProvider localeText={localeText}>
<WrapperVariantContext.Provider value="mobile">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@ describe('<AdapterDateFns />', () => {
});

it('should have correct placeholder', () => {
render(<DateTimePicker />);
render(<DateTimePicker label="test" />);

expectInputValue(screen.getByRole('textbox'), localizedTexts[localeKey].placeholder, true);
});

it('should have well formatted value', () => {
render(<DateTimePicker value={adapter.date(testDate)} />);
render(<DateTimePicker label="test" value={adapter.date(testDate)} />);

expectInputValue(screen.getByRole('textbox'), localizedTexts[localeKey].value, true);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,13 @@ describe('<AdapterDateFnsJalali />', () => {
});

it('should have correct placeholder', () => {
render(<DateTimePicker />);
render(<DateTimePicker label="test" />);

expectInputValue(screen.getByRole('textbox'), localizedTexts[localeKey].placeholder, true);
});

it('should have well formatted value', () => {
render(<DateTimePicker value={adapter.date(testDate)} />);
render(<DateTimePicker label="test" value={adapter.date(testDate)} />);

expectInputValue(screen.getByRole('textbox'), localizedTexts[localeKey].value, true);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ describe('<AdapterDayjs />', () => {
});

it('should have correct placeholder', () => {
render(<DateTimePicker />);
render(<DateTimePicker label="test" />);

expectInputValue(
screen.getByRole('textbox'),
Expand All @@ -59,7 +59,7 @@ describe('<AdapterDayjs />', () => {
});

it('should have well formatted value', () => {
render(<DateTimePicker value={adapter.date(testDate)} />);
render(<DateTimePicker label="test" value={adapter.date(testDate)} />);

expectInputValue(screen.getByRole('textbox'), localizedTexts[localeKey].value, true);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,13 @@ describe('<AdapterLuxon />', () => {
});

it('should have correct placeholder', () => {
render(<DateTimePicker />);
render(<DateTimePicker label="test" />);

expectInputValue(screen.getByRole('textbox'), localizedTexts[localeKey].placeholder, true);
});

it('should have well formatted value', () => {
render(<DateTimePicker value={adapter.date(testDate)} />);
render(<DateTimePicker label="test" value={adapter.date(testDate)} />);

expectInputValue(screen.getByRole('textbox'), localizedTexts[localeKey].value, true);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,13 @@ describe('<AdapterMoment />', () => {
});

it('should have correct placeholder', () => {
render(<DateTimePicker />);
render(<DateTimePicker label="test" />);

expectInputValue(screen.getByRole('textbox'), localizedTexts[localeKey].placeholder, true);
});

it('should have well formatted value', () => {
render(<DateTimePicker value={adapter.date(testDate)} />);
render(<DateTimePicker label="test" value={adapter.date(testDate)} />);

expectInputValue(screen.getByRole('textbox'), localizedTexts[localeKey].value, true);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ describe('<AdapterMomentHijri />', () => {
});

it('should have correct placeholder', () => {
render(<DateTimePicker />);
render(<DateTimePicker label="test" />);

expectInputValue(screen.getByRole('textbox'), localizedTexts[localeKey].placeholder, true);
});

it('should have well formatted value', () => {
render(<DateTimePicker value={adapter.date(testDate)} />);
render(<DateTimePicker label="test" value={adapter.date(testDate)} />);

expectInputValue(screen.getByRole('textbox'), localizedTexts[localeKey].value, true);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ describe('<AdapterMomentJalaali />', () => {
});

it('should have correct placeholder', () => {
render(<DateTimePicker />);
render(<DateTimePicker label="test" />);

expectInputValue(screen.getByRole('textbox'), localizedTexts[localeKey].placeholder, true);
});

it('should have well formatted value', () => {
render(<DateTimePicker value={adapter.date(testDate)} />);
render(<DateTimePicker label="test" value={adapter.date(testDate)} />);

expectInputValue(screen.getByRole('textbox'), localizedTexts[localeKey].value, true);
});
Expand Down
8 changes: 4 additions & 4 deletions packages/x-date-pickers/src/DatePicker/DatePicker.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,15 @@ describe('<DatePicker />', () => {
describe('prop: inputRef', () => {
it('should forward ref to the text box', () => {
const inputRef = React.createRef<HTMLInputElement>();
render(<DatePicker inputRef={inputRef} />);
render(<DatePicker label="test" inputRef={inputRef} />);

expect(inputRef.current).to.have.tagName('input');
});
});

describe('rendering', () => {
it('should handle controlled `onChange` in desktop mode', () => {
render(<DatePicker />);
render(<DatePicker label="test" />);
const input: HTMLInputElement = screen.getByRole('textbox');

fireEvent.change(input, { target: { value: '02/22/2022' } });
Expand All @@ -36,7 +36,7 @@ describe('<DatePicker />', () => {
const originalMatchMedia = window.matchMedia;
window.matchMedia = stubMatchMedia(false);

render(<DatePicker />);
render(<DatePicker label="test" />);

expect(screen.getByLabelText(/Choose date/)).to.have.tagName('input');

Expand All @@ -47,7 +47,7 @@ describe('<DatePicker />', () => {
if (isJSDOM) {
this.skip();
}
render(<DatePicker defaultValue={new Date(2019, 5, 5)} openTo="year" />);
render(<DatePicker label="test" defaultValue={new Date(2019, 5, 5)} openTo="year" />);

openPicker({ type: 'date', variant: 'desktop' });
expect(document.activeElement).to.have.text('2019');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ DatePickerToolbar.propTypes = {
PropTypes.func,
PropTypes.object,
]),
titleId: PropTypes.string,
/**
* Toolbar date format.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ DateTimePickerToolbar.propTypes = {
*/
onViewChange: PropTypes.func.isRequired,
readOnly: PropTypes.bool,
titleId: PropTypes.string,
/**
* Toolbar date format.
*/
Expand Down
Loading