Skip to content

fix(datepicker): add role to date range input #19717

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 29, 2020
Merged

fix(datepicker): add role to date range input #19717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 0 additions & 6 deletions src/material/datepicker/date-range-input-parts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ export interface MatDateRangeInputParent<D> {
_startInput: MatDateRangeInputPartBase<D>;
_endInput: MatDateRangeInputPartBase<D>;
_groupDisabled: boolean;
_ariaDescribedBy: string | null;
_ariaLabelledBy: string | null;
_handleChildValueChange: () => void;
_openDatepicker: () => void;
}
Expand Down Expand Up @@ -187,8 +185,6 @@ const _MatDateRangeInputBase:
'(change)': '_onChange()',
'(keydown)': '_onKeydown($event)',
'[attr.id]': '_rangeInput.id',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need also remove that from here and add the id to the actual range input. In the current state it will announce (for ChromeVox) the label, and then mention edit text group w/ label again.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that associating a label with a non-input element is invalid. Also the idea here was to allow for clicking on the label to move focus automatically to the start input.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right that the label for would not work for a role="group". Might be a general issue in our form-field as we always set the label for while we support custom controls that don't necessarily use a labelable element.

The focus for the start input should still work through the form-field onContainerClick, so that might not be an issue here. I think we should be good with the latest changes, but we might need to revisit this when we think more about how we want to handle the <label for> attribute in form fields.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crisbeto we do the same already for the select where label points to a custom control. That is a noop for screenreaders, but definitely not 100% valid markup. Also the select might not be the best candidate for accessibility comparisons. I'll leave that up to you. I don't feel too strongly about it, but would definitely prefer label not being announced twice.

'[attr.aria-labelledby]': '_rangeInput._ariaLabelledBy',
'[attr.aria-describedby]': '_rangeInput._ariaDescribedBy',
'[attr.aria-haspopup]': '_rangeInput.rangePicker ? "dialog" : null',
'[attr.aria-owns]': '(_rangeInput.rangePicker?.opened && _rangeInput.rangePicker.id) || null',
'[attr.min]': '_getMinDate() ? _dateAdapter.toIso8601(_getMinDate()) : null',
Expand Down Expand Up @@ -269,8 +265,6 @@ export class MatStartDate<D> extends _MatDateRangeInputBase<D> implements CanUpd
'(input)': '_onInput($event.target.value)',
'(change)': '_onChange()',
'(keydown)': '_onKeydown($event)',
'[attr.aria-labelledby]': '_rangeInput._ariaLabelledBy',
'[attr.aria-describedby]': '_rangeInput._ariaDescribedBy',
'[attr.aria-haspopup]': '_rangeInput.rangePicker ? "dialog" : null',
'[attr.aria-owns]': '(_rangeInput.rangePicker?.opened && _rangeInput.rangePicker.id) || null',
'[attr.min]': '_getMinDate() ? _dateAdapter.toIso8601(_getMinDate()) : null',
Expand Down
21 changes: 13 additions & 8 deletions src/material/datepicker/date-range-input.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,13 @@ describe('MatDateRangeInput', () => {
expect(fixture.componentInstance.end.nativeElement.getAttribute('type')).toBe('text');
});

it('should set the correct role on the range input', () => {
const fixture = createComponent(StandardRangePicker);
fixture.detectChanges();
const rangeInput = fixture.nativeElement.querySelector('.mat-date-range-input');
expect(rangeInput.getAttribute('role')).toBe('group');
});

it('should mark the entire range input as disabled if both inputs are disabled', () => {
const fixture = createComponent(StandardRangePicker);
fixture.detectChanges();
Expand Down Expand Up @@ -151,26 +158,24 @@ describe('MatDateRangeInput', () => {
expect(label.getAttribute('aria-owns')).toBe(start.id);
});

it('should point the input aria-labelledby to the form field label', () => {
it('should point the range input aria-labelledby to the form field label', () => {
const fixture = createComponent(StandardRangePicker);
fixture.detectChanges();
const labelId = fixture.nativeElement.querySelector('label').id;
const {start, end} = fixture.componentInstance;
const rangeInput = fixture.nativeElement.querySelector('.mat-date-range-input');

expect(labelId).toBeTruthy();
expect(start.nativeElement.getAttribute('aria-labelledby')).toBe(labelId);
expect(end.nativeElement.getAttribute('aria-labelledby')).toBe(labelId);
expect(rangeInput.getAttribute('aria-labelledby')).toBe(labelId);
});

it('should point the input aria-labelledby to the form field hint element', () => {
it('should point the range input aria-labelledby to the form field hint element', () => {
const fixture = createComponent(StandardRangePicker);
fixture.detectChanges();
const labelId = fixture.nativeElement.querySelector('.mat-hint').id;
const {start, end} = fixture.componentInstance;
const rangeInput = fixture.nativeElement.querySelector('.mat-date-range-input');

expect(labelId).toBeTruthy();
expect(start.nativeElement.getAttribute('aria-describedby')).toBe(labelId);
expect(end.nativeElement.getAttribute('aria-describedby')).toBe(labelId);
expect(rangeInput.getAttribute('aria-describedby')).toBe(labelId);
});

it('should float the form field label when either input is focused', () => {
Expand Down
3 changes: 3 additions & 0 deletions src/material/datepicker/date-range-input.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ let nextUniqueId = 0;
'class': 'mat-date-range-input',
'[class.mat-date-range-input-hide-placeholders]': '_shouldHidePlaceholders()',
'[attr.id]': 'null',
'role': 'group',
'[attr.aria-labelledby]': '_ariaLabelledBy',
'[attr.aria-describedby]': '_ariaDescribedBy',
},
changeDetection: ChangeDetectionStrategy.OnPush,
encapsulation: ViewEncapsulation.None,
Expand Down