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

fix(material-experimental/mdc-chips): Mirror aria-describedby to matChipInput #24551

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 22 additions & 7 deletions src/material-experimental/mdc-chips/chip-grid.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -853,13 +853,18 @@ describe('MDC-based MatChipGrid', () => {
let errorTestComponent: ChipGridWithFormErrorMessages;
let containerEl: HTMLElement;
let chipGridEl: HTMLElement;
let inputEl: HTMLElement;

beforeEach(() => {
beforeEach(fakeAsync(() => {
fixture = createComponent(ChipGridWithFormErrorMessages);
flush();
fixture.detectChanges();

errorTestComponent = fixture.componentInstance;
containerEl = fixture.debugElement.query(By.css('mat-form-field'))!.nativeElement;
chipGridEl = fixture.debugElement.query(By.css('mat-chip-grid'))!.nativeElement;
});
inputEl = fixture.debugElement.query(By.css('input'))!.nativeElement;
}));

it('should not show any errors if the user has not interacted', () => {
expect(errorTestComponent.formControl.untouched)
Expand Down Expand Up @@ -908,6 +913,7 @@ describe('MDC-based MatChipGrid', () => {
.toBe(0);

dispatchFakeEvent(fixture.debugElement.query(By.css('form'))!.nativeElement, 'submit');
flush();
fixture.detectChanges();

fixture.whenStable().then(() => {
Expand All @@ -924,10 +930,12 @@ describe('MDC-based MatChipGrid', () => {
.withContext('Expected aria-invalid to be set to "true".')
.toBe('true');
});
flush();
}));

it('should hide the errors and show the hints once the chip grid becomes valid', fakeAsync(() => {
errorTestComponent.formControl.markAsTouched();
flush();
fixture.detectChanges();

fixture.whenStable().then(() => {
Expand All @@ -942,6 +950,7 @@ describe('MDC-based MatChipGrid', () => {
.toBe(0);

errorTestComponent.formControl.setValue('something');
flush();
fixture.detectChanges();

fixture.whenStable().then(() => {
Expand All @@ -956,6 +965,8 @@ describe('MDC-based MatChipGrid', () => {
.withContext('Expected one hint to be shown once the input is valid.')
.toBe(1);
});

flush();
});
}));

Expand All @@ -966,27 +977,31 @@ describe('MDC-based MatChipGrid', () => {
expect(containerEl.querySelector('mat-error')!.getAttribute('aria-live')).toBe('polite');
});

it('sets the aria-describedby to reference errors when in error state', () => {
it('sets the aria-describedby on the input to reference errors when in error state', fakeAsync(() => {
let hintId = fixture.debugElement
.query(By.css('.mat-mdc-form-field-hint'))!
.nativeElement.getAttribute('id');
let describedBy = chipGridEl.getAttribute('aria-describedby');
let describedBy = inputEl.getAttribute('aria-describedby');

expect(hintId).withContext('hint should be shown').toBeTruthy();
expect(describedBy).toBe(hintId);

fixture.componentInstance.formControl.markAsTouched();
fixture.detectChanges();

// Flush the describedby timer and detect changes caused by it.
flush();
fixture.detectChanges();

let errorIds = fixture.debugElement
.queryAll(By.css('.mat-mdc-form-field-error'))
.map(el => el.nativeElement.getAttribute('id'))
.join(' ');
describedBy = chipGridEl.getAttribute('aria-describedby');
let errorDescribedBy = inputEl.getAttribute('aria-describedby');

expect(errorIds).withContext('errors should be shown').toBeTruthy();
expect(describedBy).toBe(errorIds);
});
expect(errorDescribedBy).toBe(errorIds);
}));
});

function createComponent<T>(
Expand Down
21 changes: 18 additions & 3 deletions src/material-experimental/mdc-chips/chip-grid.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,6 @@ const _MatChipGridMixinBase = mixinErrorState(MatChipGridBase);
'class': 'mat-mdc-chip-set mat-mdc-chip-grid mdc-evolution-chip-set',
'[attr.role]': 'role',
'[tabIndex]': '_chips && _chips.length === 0 ? -1 : tabIndex',
// TODO: replace this binding with use of AriaDescriber
'[attr.aria-describedby]': '_ariaDescribedby || null',
'[attr.aria-disabled]': 'disabled.toString()',
'[attr.aria-invalid]': 'errorState',
'[class.mat-mdc-chip-list-disabled]': 'disabled',
Expand Down Expand Up @@ -145,6 +143,11 @@ export class MatChipGrid

protected override _defaultRole = 'grid';

/**
* List of element ids to propagate to the chipInput's aria-describedby attribute.
*/
private _ariaDescribedbyIds: string[] = [];

/**
* Function when touched. Set as part of ControlValueAccessor implementation.
* @docs-private
Expand Down Expand Up @@ -337,6 +340,7 @@ export class MatChipGrid
/** Associates an HTML input element with this chip grid. */
registerInput(inputElement: MatChipTextControl): void {
this._chipInput = inputElement;
this._chipInput.setDescribedByIds(this._ariaDescribedbyIds);
}

/**
Expand Down Expand Up @@ -378,7 +382,18 @@ export class MatChipGrid
* @docs-private
*/
setDescribedByIds(ids: string[]) {
this._ariaDescribedby = ids.join(' ');
// We must keep this up to date to handle the case where ids are set
// before the chip input is registered.
this._ariaDescribedbyIds = ids;

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering whether we should be setting aria-describedby on the chip grid at all, considering that it never receives focus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point.

I don't see any harm in keeping it on the chip grid itself, but I also don't see any harm in removing it either.

The only (extremely minor) concern I have is if the focus management of the grid instance changes to focus the grid element itself at some point. That's not the case here, though, and I don't really think it's a big deal if we remove it.

All of that is to say: I can remove the attribute from the grid if you'd like, just let me know!

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 we should get rid of it, otherwise it's misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me! We still need to keep the property present on the control to satisfy the MatFormFieldControl interface, but we can at least remove the property binding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually maybe not lemme dig further into that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's defined on MatChipSet, which means we'll need to keep the property itself but can remove the binding.

if (this._chipInput) {
// Use a setTimeout in case this is being run during change detection
// and the chip input has already determined its host binding for
// aria-describedBy.
setTimeout(() => {
this._chipInput.setDescribedByIds(ids);
}, 0);
}
}

/**
Expand Down
74 changes: 45 additions & 29 deletions src/material-experimental/mdc-chips/chip-input.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,39 +25,35 @@ describe('MDC-based MatChipInput', () => {
let chipInputDirective: MatChipInput;
let dir = 'ltr';

beforeEach(
waitForAsync(() => {
TestBed.configureTestingModule({
imports: [PlatformModule, MatChipsModule, MatFormFieldModule, NoopAnimationsModule],
declarations: [TestChipInput],
providers: [
{
provide: Directionality,
useFactory: () => {
return {
value: dir.toLowerCase(),
change: new Subject(),
};
},
beforeEach(waitForAsync(() => {
TestBed.configureTestingModule({
imports: [PlatformModule, MatChipsModule, MatFormFieldModule, NoopAnimationsModule],
declarations: [TestChipInput],
providers: [
{
provide: Directionality,
useFactory: () => {
return {
value: dir.toLowerCase(),
change: new Subject(),
};
},
],
});
},
],
});

TestBed.compileComponents();
}),
);
TestBed.compileComponents();
}));

beforeEach(
waitForAsync(() => {
fixture = TestBed.createComponent(TestChipInput);
testChipInput = fixture.debugElement.componentInstance;
fixture.detectChanges();
beforeEach(waitForAsync(() => {
fixture = TestBed.createComponent(TestChipInput);
testChipInput = fixture.debugElement.componentInstance;
fixture.detectChanges();

inputDebugElement = fixture.debugElement.query(By.directive(MatChipInput))!;
chipInputDirective = inputDebugElement.injector.get<MatChipInput>(MatChipInput);
inputNativeElement = inputDebugElement.nativeElement;
}),
);
inputDebugElement = fixture.debugElement.query(By.directive(MatChipInput))!;
chipInputDirective = inputDebugElement.injector.get<MatChipInput>(MatChipInput);
inputNativeElement = inputDebugElement.nativeElement;
}));

describe('basic behavior', () => {
it('emits the (chipEnd) on enter keyup', () => {
Expand Down Expand Up @@ -230,6 +226,26 @@ describe('MDC-based MatChipInput', () => {
dispatchKeyboardEvent(inputNativeElement, 'keydown', ENTER, undefined, {shift: true});
expect(testChipInput.add).not.toHaveBeenCalled();
});

it('should set aria-describedby correctly when a non-empty list of ids is passed to setDescribedByIds', fakeAsync(() => {
const ids = ['a', 'b', 'c'];

testChipInput.chipGridInstance.setDescribedByIds(ids);
flush();
fixture.detectChanges();

expect(inputNativeElement.getAttribute('aria-describedby')).toEqual('a b c');
}));

it('should set aria-describedby correctly when an empty list of ids is passed to setDescribedByIds', fakeAsync(() => {
const ids: string[] = [];

testChipInput.chipGridInstance.setDescribedByIds(ids);
flush();
fixture.detectChanges();

expect(inputNativeElement.getAttribute('aria-describedby')).toBeNull();
}));
});
});

Expand Down
8 changes: 8 additions & 0 deletions src/material-experimental/mdc-chips/chip-input.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ let nextUniqueId = 0;
'[attr.disabled]': 'disabled || null',
'[attr.placeholder]': 'placeholder || null',
'[attr.aria-invalid]': '_chipGrid && _chipGrid.ngControl ? _chipGrid.ngControl.invalid : null',
'[attr.aria-describedby]': '_ariaDescribedby || null',
'[attr.aria-required]': '_chipGrid && _chipGrid.required || null',
'[attr.required]': '_chipGrid && _chipGrid.required || null',
},
Expand All @@ -73,6 +74,9 @@ export class MatChipInput implements MatChipTextControl, AfterContentInit, OnCha
/** Used to prevent focus moving to chips while user is holding backspace */
private _focusLastChipOnBackspace: boolean;

/** Value for ariaDescribedby property */
_ariaDescribedby?: string;

/** Whether the control is focused. */
focused: boolean = false;
_chipGrid: MatChipGrid;
Expand Down Expand Up @@ -240,6 +244,10 @@ export class MatChipInput implements MatChipTextControl, AfterContentInit, OnCha
this._focusLastChipOnBackspace = true;
}

setDescribedByIds(ids: string[]): void {
this._ariaDescribedby = ids.join(' ');
}

/** Checks whether a keycode is one of the configured separators. */
private _isSeparatorKey(event: KeyboardEvent) {
return !hasModifierKey(event) && new Set(this.separatorKeyCodes).has(event.keyCode);
Expand Down
5 changes: 0 additions & 5 deletions src/material-experimental/mdc-chips/chip-set.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,6 @@ const _MatChipSetMixinBase = mixinTabIndex(MatChipSetBase);
host: {
'class': 'mat-mdc-chip-set mdc-evolution-chip-set',
'[attr.role]': 'role',
// TODO: replace this binding with use of AriaDescriber
'[attr.aria-describedby]': '_ariaDescribedby || null',
},
encapsulation: ViewEncapsulation.None,
changeDetection: ChangeDetectionStrategy.OnPush,
Expand Down Expand Up @@ -138,9 +136,6 @@ export class MatChipSet
},
};

/** The aria-describedby attribute on the chip list for improved a11y. */
_ariaDescribedby: string;

/**
* Map from class to whether the class is enabled.
* Enabled classes are set on the MDC chip-set div.
Expand Down
3 changes: 3 additions & 0 deletions src/material-experimental/mdc-chips/chip-text-control.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,7 @@ export interface MatChipTextControl {

/** Focuses the text control. */
focus(): void;

/** Sets the list of ids the input is described by. */
setDescribedByIds(ids: string[]): void;
}