Skip to content

Commit

Permalink
fix(material-experimental/mdc-chips): don't use button element if chi…
Browse files Browse the repository at this point in the history
…p 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)
  • Loading branch information
crisbeto committed Jul 25, 2022
1 parent c3f2252 commit f214a2c
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 14 deletions.
12 changes: 6 additions & 6 deletions src/dev-app/mdc-chips/mdc-chips-demo.html
Expand Up @@ -25,15 +25,15 @@ <h4>With avatar, icons, and color</h4>
<mat-chip disabled>
<mat-icon matChipAvatar>home</mat-icon>
Home
<button matChipRemove>
<button matChipRemove aria-label="Remove chip">
<mat-icon>cancel</mat-icon>
</button>
</mat-chip>

<mat-chip highlighted="true" color="accent">
<mat-chip-avatar>P</mat-chip-avatar>
Portel
<button matChipRemove>
<button matChipRemove aria-label="Remove chip">
<mat-icon>cancel</mat-icon>
</button>
</mat-chip>
Expand All @@ -45,7 +45,7 @@ <h4>With avatar, icons, and color</h4>

<mat-chip>
Koby
<button matChipRemove>
<button matChipRemove aria-label="Remove chip">
<mat-icon>cancel</mat-icon>
</button>
</mat-chip>
Expand Down Expand Up @@ -85,7 +85,7 @@ <h4>With Events</h4>
<mat-chip highlighted="true" color="warn" *ngIf="visible"
(destroyed)="displayMessage('chip destroyed')" (removed)="toggleVisible()">
With Events
<button matChipRemove>
<button matChipRemove aria-label="Remove chip">
<mat-icon>cancel</mat-icon>
</button>
</mat-chip>
Expand Down Expand Up @@ -152,7 +152,7 @@ <h4>Input is last child of chip grid</h4>
(removed)="remove(person)"
(edited)="edit(person, $event)">
{{person.name}}
<button matChipRemove>
<button matChipRemove aria-label="Remove contributor">
<mat-icon>cancel</mat-icon>
</button>
</mat-chip-row>
Expand All @@ -171,7 +171,7 @@ <h4>Input is next sibling child of chip grid</h4>
<mat-chip-grid #chipGrid2 [(ngModel)]="selectedPeople" required [disabled]="disableInputs">
<mat-chip-row *ngFor="let person of people" (removed)="remove(person)">
{{person.name}}
<button matChipRemove>
<button matChipRemove aria-label="Remove contributor">
<mat-icon>cancel</mat-icon>
</button>
</mat-chip-row>
Expand Down
5 changes: 3 additions & 2 deletions src/material-experimental/mdc-chips/chip-row.html
Expand Up @@ -8,8 +8,9 @@


<span class="mdc-evolution-chip__cell mdc-evolution-chip__cell--primary" role="gridcell">
<button
<span
matChipAction
[attr.role]="editable ? 'button' : null"
[tabIndex]="tabIndex"
[disabled]="disabled"
[attr.aria-label]="ariaLabel">
Expand All @@ -27,7 +28,7 @@

<span class="mat-mdc-chip-primary-focus-indicator mat-mdc-focus-indicator"></span>
</span>
</button>
</span>
</span>

<span
Expand Down
10 changes: 10 additions & 0 deletions src/material-experimental/mdc-chips/chip-row.spec.ts
Expand Up @@ -259,6 +259,16 @@ describe('MDC-based Row Chips', () => {
return chipNativeElement.querySelector('.mat-chip-edit-input')!;
}

it('should set the role of the primary action based on whether it is editable', () => {
testComponent.editable = false;
fixture.detectChanges();
expect(primaryAction.hasAttribute('role')).toBe(false);

testComponent.editable = true;
fixture.detectChanges();
expect(primaryAction.getAttribute('role')).toBe('button');
});

it('should not delete the chip on DELETE or BACKSPACE', () => {
spyOn(testComponent, 'chipDestroy');
keyDownOnPrimaryAction(DELETE, 'Delete');
Expand Down
20 changes: 14 additions & 6 deletions src/material-experimental/mdc-chips/chip-row.ts
Expand Up @@ -81,6 +81,13 @@ export interface MatChipEditedEvent extends MatChipEvent {
export class MatChipRow extends MatChip implements AfterViewInit {
protected override basicChipAttrName = 'mat-basic-chip-row';

/**
* The editing action has to be triggered in a timeout. While we're waiting on it, a blur
* event might occur which will interrupt the editing. This flag is used to avoid interruptions
* while the editing action is being initialized.
*/
private _editStartPending = false;

@Input() editable: boolean = false;

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

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

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

// Defer initializing the input so it has time to be added to the DOM.
setTimeout(() => this._getEditInput().initialize(value));
setTimeout(() => {
this._getEditInput().initialize(value);
this._editStartPending = false;
});
}

private _onEditFinish() {
this._isEditing = false;
this.primaryAction.isInteractive = true;
this._editStartPending = false;
this.edited.emit({chip: this, value: this._getEditInput().getValue()});

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

0 comments on commit f214a2c

Please sign in to comment.