Skip to content

Commit f214a2c

Browse files
committed
fix(material-experimental/mdc-chips): don't use button element if chip row isn't editable (#25327)
Currently we always use a `button` element inside the `mat-chip-row` which can be confusing for users if the row isn't editable, because the button won't do anything. These changes switch to using a `span` whose `role` we toggle based on whether it's editable. The change ended up trickier than expected, because: 1. The focus behavior of the `span` is slightly different from the button we had before. 2. The focus restoration after an edit is finished is currently working by accident. When editing starts, we set `isInteractive = false` on the primary action which clears its `tabindex`. When editing is finished, we reset the flag, but change detection hasn't had the chance to run so the element won't actually have a `tabindex` at the moment when we try to restore focus. It worked by accident because a `button` is still focusable even without a `tabindex`. An alternative approach I was testing out was to swap between a `span` and `button` element based on the `editable` input, but that didn't work, because we have a `ViewChild` query for the primary action which wasn't being updated by the framework. (cherry picked from commit e383efd)
1 parent c3f2252 commit f214a2c

File tree

4 files changed

+33
-14
lines changed

4 files changed

+33
-14
lines changed

src/dev-app/mdc-chips/mdc-chips-demo.html

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,15 +25,15 @@ <h4>With avatar, icons, and color</h4>
2525
<mat-chip disabled>
2626
<mat-icon matChipAvatar>home</mat-icon>
2727
Home
28-
<button matChipRemove>
28+
<button matChipRemove aria-label="Remove chip">
2929
<mat-icon>cancel</mat-icon>
3030
</button>
3131
</mat-chip>
3232

3333
<mat-chip highlighted="true" color="accent">
3434
<mat-chip-avatar>P</mat-chip-avatar>
3535
Portel
36-
<button matChipRemove>
36+
<button matChipRemove aria-label="Remove chip">
3737
<mat-icon>cancel</mat-icon>
3838
</button>
3939
</mat-chip>
@@ -45,7 +45,7 @@ <h4>With avatar, icons, and color</h4>
4545

4646
<mat-chip>
4747
Koby
48-
<button matChipRemove>
48+
<button matChipRemove aria-label="Remove chip">
4949
<mat-icon>cancel</mat-icon>
5050
</button>
5151
</mat-chip>
@@ -85,7 +85,7 @@ <h4>With Events</h4>
8585
<mat-chip highlighted="true" color="warn" *ngIf="visible"
8686
(destroyed)="displayMessage('chip destroyed')" (removed)="toggleVisible()">
8787
With Events
88-
<button matChipRemove>
88+
<button matChipRemove aria-label="Remove chip">
8989
<mat-icon>cancel</mat-icon>
9090
</button>
9191
</mat-chip>
@@ -152,7 +152,7 @@ <h4>Input is last child of chip grid</h4>
152152
(removed)="remove(person)"
153153
(edited)="edit(person, $event)">
154154
{{person.name}}
155-
<button matChipRemove>
155+
<button matChipRemove aria-label="Remove contributor">
156156
<mat-icon>cancel</mat-icon>
157157
</button>
158158
</mat-chip-row>
@@ -171,7 +171,7 @@ <h4>Input is next sibling child of chip grid</h4>
171171
<mat-chip-grid #chipGrid2 [(ngModel)]="selectedPeople" required [disabled]="disableInputs">
172172
<mat-chip-row *ngFor="let person of people" (removed)="remove(person)">
173173
{{person.name}}
174-
<button matChipRemove>
174+
<button matChipRemove aria-label="Remove contributor">
175175
<mat-icon>cancel</mat-icon>
176176
</button>
177177
</mat-chip-row>

src/material-experimental/mdc-chips/chip-row.html

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,9 @@
88

99

1010
<span class="mdc-evolution-chip__cell mdc-evolution-chip__cell--primary" role="gridcell">
11-
<button
11+
<span
1212
matChipAction
13+
[attr.role]="editable ? 'button' : null"
1314
[tabIndex]="tabIndex"
1415
[disabled]="disabled"
1516
[attr.aria-label]="ariaLabel">
@@ -27,7 +28,7 @@
2728

2829
<span class="mat-mdc-chip-primary-focus-indicator mat-mdc-focus-indicator"></span>
2930
</span>
30-
</button>
31+
</span>
3132
</span>
3233

3334
<span

src/material-experimental/mdc-chips/chip-row.spec.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,16 @@ describe('MDC-based Row Chips', () => {
259259
return chipNativeElement.querySelector('.mat-chip-edit-input')!;
260260
}
261261

262+
it('should set the role of the primary action based on whether it is editable', () => {
263+
testComponent.editable = false;
264+
fixture.detectChanges();
265+
expect(primaryAction.hasAttribute('role')).toBe(false);
266+
267+
testComponent.editable = true;
268+
fixture.detectChanges();
269+
expect(primaryAction.getAttribute('role')).toBe('button');
270+
});
271+
262272
it('should not delete the chip on DELETE or BACKSPACE', () => {
263273
spyOn(testComponent, 'chipDestroy');
264274
keyDownOnPrimaryAction(DELETE, 'Delete');

src/material-experimental/mdc-chips/chip-row.ts

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,13 @@ export interface MatChipEditedEvent extends MatChipEvent {
8181
export class MatChipRow extends MatChip implements AfterViewInit {
8282
protected override basicChipAttrName = 'mat-basic-chip-row';
8383

84+
/**
85+
* The editing action has to be triggered in a timeout. While we're waiting on it, a blur
86+
* event might occur which will interrupt the editing. This flag is used to avoid interruptions
87+
* while the editing action is being initialized.
88+
*/
89+
private _editStartPending = false;
90+
8491
@Input() editable: boolean = false;
8592

8693
/** Emitted when the chip is edited. */
@@ -120,7 +127,7 @@ export class MatChipRow extends MatChip implements AfterViewInit {
120127

121128
this.role = 'row';
122129
this._onBlur.pipe(takeUntil(this.destroyed)).subscribe(() => {
123-
if (this._isEditing) {
130+
if (this._isEditing && !this._editStartPending) {
124131
this._onEditFinish();
125132
}
126133
});
@@ -175,18 +182,19 @@ export class MatChipRow extends MatChip implements AfterViewInit {
175182
// The value depends on the DOM so we need to extract it before we flip the flag.
176183
const value = this.value;
177184

178-
// Make the primary action non-interactive so that it doesn't
179-
// navigate when the user presses the arrow keys while editing.
180-
this.primaryAction.isInteractive = false;
181185
this._isEditing = true;
186+
this._editStartPending = true;
182187

183188
// Defer initializing the input so it has time to be added to the DOM.
184-
setTimeout(() => this._getEditInput().initialize(value));
189+
setTimeout(() => {
190+
this._getEditInput().initialize(value);
191+
this._editStartPending = false;
192+
});
185193
}
186194

187195
private _onEditFinish() {
188196
this._isEditing = false;
189-
this.primaryAction.isInteractive = true;
197+
this._editStartPending = false;
190198
this.edited.emit({chip: this, value: this._getEditInput().getValue()});
191199

192200
// If the edit input is still focused or focus was returned to the body after it was destroyed,

0 commit comments

Comments
 (0)