Skip to content

Commit 13a9c48

Browse files
adolgachevadolgachev
authored
fix(material/chips): refactor non-interactive actions to prevent adding click handlers (#31664)
1 parent b382b60 commit 13a9c48

File tree

10 files changed

+53
-73
lines changed

10 files changed

+53
-73
lines changed

‎goldens/material/chips/index.api.md

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -524,7 +524,7 @@ export class MatChipSet implements AfterViewInit, OnDestroy {
524524
protected _originatesFromChip(event: Event): boolean;
525525
get role(): string | null;
526526
set role(value: string | null);
527-
protected _skipPredicate(action: MatChipAction): boolean;
527+
protected _skipPredicate(action: MatChipContent): boolean;
528528
protected _syncChipsState(): void;
529529
tabIndex: number;
530530
// (undocumented)
@@ -555,8 +555,7 @@ export interface MatChipTextControl {
555555
}
556556

557557
// @public
558-
export class MatChipTrailingIcon extends MatChipAction {
559-
isInteractive: boolean;
558+
export class MatChipTrailingIcon extends MatChipContent {
560559
// (undocumented)
561560
_isPrimary: boolean;
562561
// (undocumented)

‎src/material/chips/chip-action.ts

Lines changed: 28 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -20,25 +20,22 @@ import {_CdkPrivateStyleLoader} from '@angular/cdk/private';
2020
import {_StructuralStylesLoader} from '../core';
2121

2222
/**
23-
* Section within a chip.
23+
* A non-interactive section of a chip.
2424
* @docs-private
2525
*/
2626
@Directive({
27-
selector: '[matChipAction]',
27+
selector: '[matChipContent]',
2828
host: {
29-
'class': 'mdc-evolution-chip__action mat-mdc-chip-action',
29+
'class':
30+
'mat-mdc-chip-action mdc-evolution-chip__action mdc-evolution-chip__action--presentational',
3031
'[class.mdc-evolution-chip__action--primary]': '_isPrimary',
31-
'[class.mdc-evolution-chip__action--presentational]': '!isInteractive',
3232
'[class.mdc-evolution-chip__action--secondary]': '!_isPrimary',
3333
'[class.mdc-evolution-chip__action--trailing]': '!_isPrimary && !_isLeading',
34-
'[attr.tabindex]': '_getTabindex()',
3534
'[attr.disabled]': '_getDisabledAttribute()',
3635
'[attr.aria-disabled]': 'disabled',
37-
'(click)': '_handleClick($event)',
38-
'(keydown)': '_handleKeydown($event)',
3936
},
4037
})
41-
export class MatChipAction {
38+
export class MatChipContent {
4239
_elementRef = inject<ElementRef<HTMLElement>>(ElementRef);
4340
protected _parentChip = inject<{
4441
_handlePrimaryActionInteraction(): void;
@@ -48,9 +45,6 @@ export class MatChipAction {
4845
_isEditing?: boolean;
4946
}>(MAT_CHIP);
5047

51-
/** Whether the action is interactive. */
52-
@Input() isInteractive = true;
53-
5448
/** Whether this is the primary action in the chip. */
5549
_isPrimary = true;
5650

@@ -88,15 +82,6 @@ export class MatChipAction {
8882
return this.disabled && !this._allowFocusWhenDisabled ? '' : null;
8983
}
9084

91-
/**
92-
* Determine the value of the tabindex attribute for this chip action.
93-
*/
94-
protected _getTabindex(): string | null {
95-
return (this.disabled && !this._allowFocusWhenDisabled) || !this.isInteractive
96-
? null
97-
: this.tabIndex.toString();
98-
}
99-
10085
constructor(...args: unknown[]);
10186

10287
constructor() {
@@ -109,9 +94,31 @@ export class MatChipAction {
10994
focus() {
11095
this._elementRef.nativeElement.focus();
11196
}
97+
}
98+
99+
/**
100+
* Interactive section of a chip.
101+
* @docs-private
102+
*/
103+
@Directive({
104+
selector: '[matChipAction]',
105+
host: {
106+
'[attr.tabindex]': '_getTabindex()',
107+
'[class.mdc-evolution-chip__action--presentational]': 'false',
108+
'(click)': '_handleClick($event)',
109+
'(keydown)': '_handleKeydown($event)',
110+
},
111+
})
112+
export class MatChipAction extends MatChipContent {
113+
/**
114+
* Determine the value of the tabindex attribute for this chip action.
115+
*/
116+
protected _getTabindex(): string | null {
117+
return this.disabled && !this._allowFocusWhenDisabled ? null : this.tabIndex.toString();
118+
}
112119

113120
_handleClick(event: MouseEvent) {
114-
if (!this.disabled && this.isInteractive && this._isPrimary) {
121+
if (!this.disabled && this._isPrimary) {
115122
event.preventDefault();
116123
this._parentChip._handlePrimaryActionInteraction();
117124
}
@@ -121,7 +128,6 @@ export class MatChipAction {
121128
if (
122129
(event.keyCode === ENTER || event.keyCode === SPACE) &&
123130
!this.disabled &&
124-
this.isInteractive &&
125131
this._isPrimary &&
126132
!this._parentChip._isEditing
127133
) {

‎src/material/chips/chip-icons.ts

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

99
import {ENTER, SPACE} from '@angular/cdk/keycodes';
1010
import {Directive} from '@angular/core';
11-
import {MatChipAction} from './chip-action';
11+
import {MatChipAction, MatChipContent} from './chip-action';
1212
import {MAT_CHIP_AVATAR, MAT_CHIP_EDIT, MAT_CHIP_REMOVE, MAT_CHIP_TRAILING_ICON} from './tokens';
1313

1414
/** Avatar image within a chip. */
@@ -32,13 +32,7 @@ export class MatChipAvatar {}
3232
},
3333
providers: [{provide: MAT_CHIP_TRAILING_ICON, useExisting: MatChipTrailingIcon}],
3434
})
35-
export class MatChipTrailingIcon extends MatChipAction {
36-
/**
37-
* MDC considers all trailing actions as a remove icon,
38-
* but we support non-interactive trailing icons.
39-
*/
40-
override isInteractive = false;
41-
35+
export class MatChipTrailingIcon extends MatChipContent {
4236
override _isPrimary = false;
4337
}
4438

‎src/material/chips/chip-listbox.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -398,6 +398,6 @@ export class MatChipListbox
398398
// https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/):
399399
// "For the following composite widget elements, keep them focusable when disabled: Options in a
400400
// Listbox..."
401-
return !action.isInteractive;
401+
return false;
402402
}
403403
}

‎src/material/chips/chip-row.spec.ts

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -458,19 +458,6 @@ describe('Row Chips', () => {
458458
fixture.detectChanges();
459459
expect(chipInstance._hasInteractiveActions()).toBe(true);
460460
});
461-
462-
it('should return false if all actions are non-interactive', () => {
463-
// Make primary action non-interactive for testing purposes.
464-
chipInstance.primaryAction.isInteractive = false;
465-
testComponent.showTrailingIcon = true;
466-
testComponent.removable = false; // remove icon is interactive
467-
fixture.changeDetectorRef.markForCheck();
468-
fixture.detectChanges();
469-
470-
// The trailing icon is not interactive.
471-
expect(chipInstance.trailingIcon.isInteractive).toBe(false);
472-
expect(chipInstance._hasInteractiveActions()).toBe(false);
473-
});
474461
});
475462

476463
describe('with edit icon', () => {

‎src/material/chips/chip-row.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ export interface MatChipEditedEvent extends MatChipEvent {
6161
'[attr.aria-description]': 'null',
6262
'[attr.role]': 'role',
6363
'(focus)': '_handleFocus()',
64-
'(click)': '_handleClick($event)',
64+
'(click)': 'this._hasInteractiveActions() ? _handleClick($event) : null',
6565
'(dblclick)': '_handleDoubleclick($event)',
6666
},
6767
providers: [

‎src/material/chips/chip-set.ts

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ import {
2626
import {Observable, Subject, merge} from 'rxjs';
2727
import {startWith, switchMap, takeUntil} from 'rxjs/operators';
2828
import {MatChip, MatChipEvent} from './chip';
29-
import {MatChipAction} from './chip-action';
29+
import {MatChipAction, MatChipContent} from './chip-action';
3030

3131
/**
3232
* Basic container component for the MatChip component.
@@ -266,10 +266,9 @@ export class MatChipSet implements AfterViewInit, OnDestroy {
266266
* Determines if key manager should avoid putting a given chip action in the tab index. Skip
267267
* non-interactive and disabled actions since the user can't do anything with them.
268268
*/
269-
protected _skipPredicate(action: MatChipAction): boolean {
270-
// Skip chips that the user cannot interact with. `mat-chip-set` does not permit focusing disabled
271-
// chips.
272-
return !action.isInteractive || action.disabled;
269+
protected _skipPredicate(action: MatChipContent): boolean {
270+
// `mat-chip-set` does not permit focusing disabled chips.
271+
return action.disabled;
273272
}
274273

275274
/** Listens to changes in the chip set and syncs up the state of the individual chips. */

‎src/material/chips/chip.html

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
<span class="mat-mdc-chip-focus-overlay"></span>
22

33
<span class="mdc-evolution-chip__cell mdc-evolution-chip__cell--primary">
4-
<span matChipAction [isInteractive]="false">
4+
<span matChipContent>
55
@if (leadingIcon) {
66
<span class="mdc-evolution-chip__graphic mat-mdc-chip-graphic">
77
<ng-content select="mat-chip-avatar, [matChipAvatar]"></ng-content>

‎src/material/chips/chip.spec.ts

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,17 @@ describe('MatChip', () => {
5454

5555
expect(chip.getAttribute('tabindex')).toBe('15');
5656
});
57+
58+
it('should disable the ripple if there are no interactive actions', () => {
59+
fixture = TestBed.createComponent(BasicChip);
60+
fixture.detectChanges();
61+
62+
chipDebugElement = fixture.debugElement.query(By.directive(MatChip))!;
63+
chipInstance = chipDebugElement.injector.get<MatChip>(MatChip);
64+
65+
expect(chipInstance._hasInteractiveActions()).toBe(false);
66+
expect(chipInstance._isRippleDisabled()).toBe(true);
67+
});
5768
});
5869

5970
describe('MatChip', () => {
@@ -117,18 +128,6 @@ describe('MatChip', () => {
117128
expect(primaryAction.hasAttribute('tabindex')).toBe(false);
118129
});
119130

120-
it('should disable the ripple if there are no interactive actions', () => {
121-
// expect(chipInstance._isRippleDisabled()).toBe(false); TODO(andreyd)
122-
123-
// Make primary action non-interactive for testing purposes.
124-
chipInstance.primaryAction.isInteractive = false;
125-
fixture.changeDetectorRef.markForCheck();
126-
fixture.detectChanges();
127-
128-
expect(chipInstance._hasInteractiveActions()).toBe(false);
129-
expect(chipInstance._isRippleDisabled()).toBe(true);
130-
});
131-
132131
it('should return the chip text if value is undefined', () => {
133132
expect(chipInstance.value.trim()).toBe(fixture.componentInstance.name);
134133
});

‎src/material/chips/chip.ts

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ import {
4343
_animationsDisabled,
4444
} from '../core';
4545
import {Subject, Subscription, merge} from 'rxjs';
46-
import {MatChipAction} from './chip-action';
46+
import {MatChipAction, MatChipContent} from './chip-action';
4747
import {MatChipAvatar, MatChipEdit, MatChipRemove, MatChipTrailingIcon} from './chip-icons';
4848
import {
4949
MAT_CHIP,
@@ -93,7 +93,7 @@ export interface MatChipEvent {
9393
encapsulation: ViewEncapsulation.None,
9494
changeDetection: ChangeDetectionStrategy.OnPush,
9595
providers: [{provide: MAT_CHIP, useExisting: MatChip}],
96-
imports: [MatChipAction],
96+
imports: [MatChipContent],
9797
})
9898
export class MatChip implements OnInit, AfterViewInit, AfterContentInit, DoCheck, OnDestroy {
9999
_changeDetectorRef = inject(ChangeDetectorRef);
@@ -386,10 +386,6 @@ export class MatChip implements OnInit, AfterViewInit, AfterContentInit, DoCheck
386386
result.push(this.removeIcon);
387387
}
388388

389-
if (this.trailingIcon) {
390-
result.push(this.trailingIcon);
391-
}
392-
393389
return result;
394390
}
395391

@@ -400,7 +396,7 @@ export class MatChip implements OnInit, AfterViewInit, AfterContentInit, DoCheck
400396

401397
/** Returns whether the chip has any interactive actions. */
402398
_hasInteractiveActions(): boolean {
403-
return this._getActions().some(a => a.isInteractive);
399+
return this._getActions().length > 0;
404400
}
405401

406402
/** Handles interactions with the edit action of the chip. */

0 commit comments

Comments
 (0)