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.

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

feat(material/select): allow user-defined aria-describedby #24644

Merged
merged 1 commit into from
Apr 20, 2022
Merged

feat(material/select): allow user-defined aria-describedby #24644

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
22 changes: 21 additions & 1 deletion src/material-experimental/mdc-select/select.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,22 @@ describe('MDC-based MatSelect', () => {
expect(select.getAttribute('tabindex')).toEqual('0');
}));

it('should set `aria-describedby` to the id of the mat-hint', fakeAsync(() => {
expect(select.getAttribute('aria-describedby')).toBeNull();

fixture.componentInstance.hint = 'test';
fixture.detectChanges();
const hint = fixture.debugElement.query(By.css('mat-hint')).nativeElement;
expect(select.getAttribute('aria-describedby')).toBe(hint.getAttribute('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 that you can drop this assertion since it's covered by the one below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This asserts that it's exactly the same as the hint's ID, whereas the next assertion asserts only that it matches a certain format: expect(select.getAttribute('aria-describedby')).toMatch(/^mat-mdc-hint-\d+$/);

expect(select.getAttribute('aria-describedby')).toMatch(/^mat-mdc-hint-\d+$/);
}));

it('should support user binding to `aria-describedby`', fakeAsync(() => {
fixture.componentInstance.ariaDescribedBy = 'test';
fixture.detectChanges();
expect(select.getAttribute('aria-describedby')).toBe('test');
}));

it('should be able to override the tabindex', fakeAsync(() => {
fixture.componentInstance.tabIndexOverride = 3;
fixture.detectChanges();
Expand Down Expand Up @@ -4223,13 +4239,15 @@ describe('MDC-based MatSelect', () => {
<mat-form-field>
<mat-label *ngIf="hasLabel">Select a food</mat-label>
<mat-select placeholder="Food" [formControl]="control" [required]="isRequired"
[tabIndex]="tabIndexOverride" [aria-label]="ariaLabel" [aria-labelledby]="ariaLabelledby"
[tabIndex]="tabIndexOverride" [aria-describedby]="ariaDescribedBy"
[aria-label]="ariaLabel" [aria-labelledby]="ariaLabelledby"
[panelClass]="panelClass" [disableRipple]="disableRipple"
[typeaheadDebounceInterval]="typeaheadDebounceInterval">
<mat-option *ngFor="let food of foods" [value]="food.value" [disabled]="food.disabled">
{{ food.viewValue }}
</mat-option>
</mat-select>
<mat-hint *ngIf="hint">{{ hint }}</mat-hint>
</mat-form-field>
<div [style.height.px]="heightBelow"></div>
`,
Expand All @@ -4250,7 +4268,9 @@ class BasicSelect {
heightAbove = 0;
heightBelow = 0;
hasLabel = true;
hint: string;
tabIndexOverride: number;
ariaDescribedBy: string;
ariaLabel: string;
ariaLabelledby: string;
panelClass = ['custom-one', 'custom-two'];
Expand Down
1 change: 0 additions & 1 deletion src/material-experimental/mdc-select/select.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ export class MatSelectTrigger {}
'[attr.aria-required]': 'required.toString()',
'[attr.aria-disabled]': 'disabled.toString()',
'[attr.aria-invalid]': 'errorState',
'[attr.aria-describedby]': '_ariaDescribedby || null',
'[attr.aria-activedescendant]': '_getAriaActiveDescendant()',
'[class.mat-mdc-select-disabled]': 'disabled',
'[class.mat-mdc-select-invalid]': 'errorState',
Expand Down
22 changes: 21 additions & 1 deletion src/material/select/select.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,22 @@ describe('MatSelect', () => {
expect(select.getAttribute('aria-labelledby')?.trim()).toBe(valueId);
});

it('should set `aria-describedby` to the id of the mat-hint', fakeAsync(() => {
expect(select.getAttribute('aria-describedby')).toBeNull();

fixture.componentInstance.hint = 'test';
fixture.detectChanges();
const hint = fixture.debugElement.query(By.css('.mat-hint')).nativeElement;
expect(select.getAttribute('aria-describedby')).toBe(hint.getAttribute('id'));
expect(select.getAttribute('aria-describedby')).toMatch(/^mat-hint-\d+$/);
}));

it('should support user binding to `aria-describedby`', fakeAsync(() => {
fixture.componentInstance.ariaDescribedBy = 'test';
fixture.detectChanges();
expect(select.getAttribute('aria-describedby')).toBe('test');
}));

it('should select options via the UP/DOWN arrow keys on a closed select', fakeAsync(() => {
const formControl = fixture.componentInstance.control;
const options = fixture.componentInstance.options.toArray();
Expand Down Expand Up @@ -5186,13 +5202,15 @@ describe('MatSelect', () => {
<div [style.height.px]="heightAbove"></div>
<mat-form-field>
<mat-select placeholder="Food" [formControl]="control" [required]="isRequired"
[tabIndex]="tabIndexOverride" [aria-label]="ariaLabel" [aria-labelledby]="ariaLabelledby"
[tabIndex]="tabIndexOverride" [aria-describedby]="ariaDescribedBy"
[aria-label]="ariaLabel" [aria-labelledby]="ariaLabelledby"
[panelClass]="panelClass" [disableRipple]="disableRipple"
[typeaheadDebounceInterval]="typeaheadDebounceInterval">
<mat-option *ngFor="let food of foods" [value]="food.value" [disabled]="food.disabled">
{{ food.viewValue }}
</mat-option>
</mat-select>
<mat-hint *ngIf="hint">{{ hint }}</mat-hint>
</mat-form-field>
<div [style.height.px]="heightBelow"></div>
`,
Expand All @@ -5212,7 +5230,9 @@ class BasicSelect {
isRequired: boolean;
heightAbove = 0;
heightBelow = 0;
hint: string;
tabIndexOverride: number;
ariaDescribedBy: string;
ariaLabel: string;
ariaLabelledby: string;
panelClass = ['custom-one', 'custom-two'];
Expand Down
16 changes: 11 additions & 5 deletions src/material/select/select.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,8 +303,11 @@ export abstract class _MatSelectBase<C>
/** Emits whenever the component is destroyed. */
protected readonly _destroy = new Subject<void>();

/** The aria-describedby attribute on the select for improved a11y. */
_ariaDescribedby: string;
/**
* Implemented as part of MatFormFieldControl.
* @docs-private
*/
@Input('aria-describedby') userAriaDescribedBy: string;

/** Deals with the selection logic. */
_selectionModel: SelectionModel<MatOption>;
Expand Down Expand Up @@ -611,7 +614,7 @@ export abstract class _MatSelectBase<C>
ngOnChanges(changes: SimpleChanges) {
// Updating the disabled state is handled by `mixinDisabled`, but we need to additionally let
// the parent form field know to run change detection when the disabled state changes.
if (changes['disabled']) {
if (changes['disabled'] || changes['userAriaDescribedBy']) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change necessary? Since the aria-describedby is on the select it shouldn't be required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without it, this test fails:

it('should support user binding to `aria-describedby`', fakeAsync(() => {
  fixture.componentInstance.ariaDescribedBy = 'test';
  fixture.detectChanges();
  expect(select.getAttribute('aria-describedby')).toBe('test');
}));

As a result of this PR (and #24292 for MatChipList and #19587 for MatInput), the aria-describedby is no longer set in @Component({ host }). Rather, it is set using nativeElement.setAttribute in MatSelect#setDescribedByIds, which is called by MatFormField#_syncDescribedByIds, which in turn is called when stateChanges updates.

So, userAriaDescribedBy has to trigger a stateChanges.next() in order for the aria-describedby to actually be set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...and for that matter, MatChipList needs unit tests for this too! I've submitted #24657. In that one I used a setter for userAriaDescribedBy to call stateChanges.next(). Let me know if you'd prefer that here too.

this.stateChanges.next();
}

Expand Down Expand Up @@ -1146,7 +1149,11 @@ export abstract class _MatSelectBase<C>
* @docs-private
*/
setDescribedByIds(ids: string[]) {
this._ariaDescribedby = ids.join(' ');
if (ids.length) {
this._elementRef.nativeElement.setAttribute('aria-describedby', ids.join(' '));
} else {
this._elementRef.nativeElement.removeAttribute('aria-describedby');
}
}

/**
Expand Down Expand Up @@ -1191,7 +1198,6 @@ export abstract class _MatSelectBase<C>
'[attr.aria-required]': 'required.toString()',
'[attr.aria-disabled]': 'disabled.toString()',
'[attr.aria-invalid]': 'errorState',
'[attr.aria-describedby]': '_ariaDescribedby || null',
'[attr.aria-activedescendant]': '_getAriaActiveDescendant()',
'[class.mat-select-disabled]': 'disabled',
'[class.mat-select-invalid]': 'errorState',
Expand Down
4 changes: 2 additions & 2 deletions tools/public_api_guard/material/select.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ export const matSelectAnimations: {
// @public
export abstract class _MatSelectBase<C> extends _MatSelectMixinBase implements AfterContentInit, OnChanges, OnDestroy, OnInit, DoCheck, ControlValueAccessor, CanDisable, HasTabIndex, MatFormFieldControl<any>, CanUpdateErrorState, CanDisableRipple {
constructor(_viewportRuler: ViewportRuler, _changeDetectorRef: ChangeDetectorRef, _ngZone: NgZone, _defaultErrorStateMatcher: ErrorStateMatcher, elementRef: ElementRef, _dir: Directionality, _parentForm: NgForm, _parentFormGroup: FormGroupDirective, _parentFormField: MatFormField, ngControl: NgControl, tabIndex: string, scrollStrategyFactory: any, _liveAnnouncer: LiveAnnouncer, _defaultOptions?: MatSelectConfig | undefined);
_ariaDescribedby: string;
ariaLabel: string;
ariaLabelledby: string;
protected _canOpen(): boolean;
Expand Down Expand Up @@ -203,6 +202,7 @@ export abstract class _MatSelectBase<C> extends _MatSelectMixinBase implements A
get triggerValue(): string;
get typeaheadDebounceInterval(): number;
set typeaheadDebounceInterval(value: NumberInput);
userAriaDescribedBy: string;
get value(): any;
set value(newValue: any);
readonly valueChange: EventEmitter<any>;
Expand All @@ -211,7 +211,7 @@ export abstract class _MatSelectBase<C> extends _MatSelectMixinBase implements A
protected _viewportRuler: ViewportRuler;
writeValue(value: any): void;
// (undocumented)
static ɵdir: i0.ɵɵDirectiveDeclaration<_MatSelectBase<any>, never, never, { "panelClass": "panelClass"; "placeholder": "placeholder"; "required": "required"; "multiple": "multiple"; "disableOptionCentering": "disableOptionCentering"; "compareWith": "compareWith"; "value": "value"; "ariaLabel": "aria-label"; "ariaLabelledby": "aria-labelledby"; "errorStateMatcher": "errorStateMatcher"; "typeaheadDebounceInterval": "typeaheadDebounceInterval"; "sortComparator": "sortComparator"; "id": "id"; }, { "openedChange": "openedChange"; "_openedStream": "opened"; "_closedStream": "closed"; "selectionChange": "selectionChange"; "valueChange": "valueChange"; }, never>;
static ɵdir: i0.ɵɵDirectiveDeclaration<_MatSelectBase<any>, never, never, { "userAriaDescribedBy": "aria-describedby"; "panelClass": "panelClass"; "placeholder": "placeholder"; "required": "required"; "multiple": "multiple"; "disableOptionCentering": "disableOptionCentering"; "compareWith": "compareWith"; "value": "value"; "ariaLabel": "aria-label"; "ariaLabelledby": "aria-labelledby"; "errorStateMatcher": "errorStateMatcher"; "typeaheadDebounceInterval": "typeaheadDebounceInterval"; "sortComparator": "sortComparator"; "id": "id"; }, { "openedChange": "openedChange"; "_openedStream": "opened"; "_closedStream": "closed"; "selectionChange": "selectionChange"; "valueChange": "valueChange"; }, never>;
// (undocumented)
static ɵfac: i0.ɵɵFactoryDeclaration<_MatSelectBase<any>, [null, null, null, null, null, { optional: true; }, { optional: true; }, { optional: true; }, { optional: true; }, { optional: true; self: true; }, { attribute: "tabindex"; }, null, null, { optional: true; }]>;
}
Expand Down