Skip to content

fix(material/chips): refactor non-interactive actions to prevent adding click handlers #31664

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
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
66 changes: 39 additions & 27 deletions src/material/chips/chip-action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,26 +19,28 @@ import {MAT_CHIP} from './tokens';
import {_CdkPrivateStyleLoader} from '@angular/cdk/private';
import {_StructuralStylesLoader} from '../core';

const chipActionHostBase = {
'class': 'mdc-evolution-chip__action mat-mdc-chip-action',
'[class.mdc-evolution-chip__action--primary]': '_isPrimary',
'[class.mdc-evolution-chip__action--secondary]': '!_isPrimary',
'[class.mdc-evolution-chip__action--trailing]': '!_isPrimary && !_isLeading',
'[attr.disabled]': '_getDisabledAttribute()',
'[attr.aria-disabled]': 'disabled',
};

/**
* Section within a chip.
* A non-interactive section of a chip.
* @docs-private
*/
@Directive({
selector: '[matChipAction]',
selector: '[matChipContent]',
host: {
'class': 'mdc-evolution-chip__action mat-mdc-chip-action',
'[class.mdc-evolution-chip__action--primary]': '_isPrimary',
'[class.mdc-evolution-chip__action--presentational]': '!isInteractive',
'[class.mdc-evolution-chip__action--secondary]': '!_isPrimary',
'[class.mdc-evolution-chip__action--trailing]': '!_isPrimary && !_isLeading',
'[attr.tabindex]': '_getTabindex()',
'[attr.disabled]': '_getDisabledAttribute()',
'[attr.aria-disabled]': 'disabled',
'(click)': '_handleClick($event)',
'(keydown)': '_handleKeydown($event)',
...chipActionHostBase,
'[class.mdc-evolution-chip__action--presentational]': 'true',
},
standalone: true,
})
export class MatChipAction {
export class MatChipContent {
_elementRef = inject<ElementRef<HTMLElement>>(ElementRef);
protected _parentChip = inject<{
_handlePrimaryActionInteraction(): void;
Expand All @@ -48,9 +50,6 @@ export class MatChipAction {
_isEditing?: boolean;
}>(MAT_CHIP);

/** Whether the action is interactive. */
@Input() isInteractive = true;

/** Whether this is the primary action in the chip. */
_isPrimary = true;

Expand Down Expand Up @@ -88,15 +87,6 @@ export class MatChipAction {
return this.disabled && !this._allowFocusWhenDisabled ? '' : null;
}

/**
* Determine the value of the tabindex attribute for this chip action.
*/
protected _getTabindex(): string | null {
return (this.disabled && !this._allowFocusWhenDisabled) || !this.isInteractive
? null
: this.tabIndex.toString();
}

constructor(...args: unknown[]);

constructor() {
Expand All @@ -109,9 +99,32 @@ export class MatChipAction {
focus() {
this._elementRef.nativeElement.focus();
}
}

/**
* Interactive section of a chip.
* @docs-private
*/
@Directive({
selector: '[matChipAction]',
host: {
...chipActionHostBase,
'[attr.tabindex]': '_getTabindex()',
'(click)': '_handleClick($event)',
'(keydown)': '_handleKeydown($event)',
},
standalone: true,
})
export class MatChipAction extends MatChipContent {
/**
* Determine the value of the tabindex attribute for this chip action.
*/
protected _getTabindex(): string | null {
return this.disabled && !this._allowFocusWhenDisabled ? null : this.tabIndex.toString();
}

_handleClick(event: MouseEvent) {
if (!this.disabled && this.isInteractive && this._isPrimary) {
if (!this.disabled && this._isPrimary) {
event.preventDefault();
this._parentChip._handlePrimaryActionInteraction();
}
Expand All @@ -121,7 +134,6 @@ export class MatChipAction {
if (
(event.keyCode === ENTER || event.keyCode === SPACE) &&
!this.disabled &&
this.isInteractive &&
this._isPrimary &&
!this._parentChip._isEditing
) {
Expand Down
10 changes: 2 additions & 8 deletions src/material/chips/chip-icons.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

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

/** Avatar image within a chip. */
Expand All @@ -32,13 +32,7 @@ export class MatChipAvatar {}
},
providers: [{provide: MAT_CHIP_TRAILING_ICON, useExisting: MatChipTrailingIcon}],
})
export class MatChipTrailingIcon extends MatChipAction {
/**
* MDC considers all trailing actions as a remove icon,
* but we support non-interactive trailing icons.
*/
override isInteractive = false;

export class MatChipTrailingIcon extends MatChipContent {
override _isPrimary = false;
}

Expand Down
2 changes: 1 addition & 1 deletion src/material/chips/chip-listbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,6 @@ export class MatChipListbox
// https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/):
// "For the following composite widget elements, keep them focusable when disabled: Options in a
// Listbox..."
return !action.isInteractive;
return false;
}
}
13 changes: 0 additions & 13 deletions src/material/chips/chip-row.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -458,19 +458,6 @@ describe('Row Chips', () => {
fixture.detectChanges();
expect(chipInstance._hasInteractiveActions()).toBe(true);
});

it('should return false if all actions are non-interactive', () => {
// Make primary action non-interactive for testing purposes.
chipInstance.primaryAction.isInteractive = false;
testComponent.showTrailingIcon = true;
testComponent.removable = false; // remove icon is interactive
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

// The trailing icon is not interactive.
expect(chipInstance.trailingIcon.isInteractive).toBe(false);
expect(chipInstance._hasInteractiveActions()).toBe(false);
});
});

describe('with edit icon', () => {
Expand Down
2 changes: 1 addition & 1 deletion src/material/chips/chip-row.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export interface MatChipEditedEvent extends MatChipEvent {
'[attr.aria-description]': 'null',
'[attr.role]': 'role',
'(focus)': '_handleFocus()',
'(click)': '_handleClick($event)',
'(click)': 'this._hasInteractiveActions() ? _handleClick($event) : null',
'(dblclick)': '_handleDoubleclick($event)',
},
providers: [
Expand Down
9 changes: 4 additions & 5 deletions src/material/chips/chip-set.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
import {Observable, Subject, merge} from 'rxjs';
import {startWith, switchMap, takeUntil} from 'rxjs/operators';
import {MatChip, MatChipEvent} from './chip';
import {MatChipAction} from './chip-action';
import {MatChipAction, MatChipContent} from './chip-action';

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

/** Listens to changes in the chip set and syncs up the state of the individual chips. */
Expand Down
2 changes: 1 addition & 1 deletion src/material/chips/chip.html
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<span class="mat-mdc-chip-focus-overlay"></span>

<span class="mdc-evolution-chip__cell mdc-evolution-chip__cell--primary">
<span matChipAction [isInteractive]="false">
<span matChipContent>
@if (leadingIcon) {
<span class="mdc-evolution-chip__graphic mat-mdc-chip-graphic">
<ng-content select="mat-chip-avatar, [matChipAvatar]"></ng-content>
Expand Down
23 changes: 11 additions & 12 deletions src/material/chips/chip.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,17 @@ describe('MatChip', () => {

expect(chip.getAttribute('tabindex')).toBe('15');
});

it('should disable the ripple if there are no interactive actions', () => {
fixture = TestBed.createComponent(BasicChip);
fixture.detectChanges();

chipDebugElement = fixture.debugElement.query(By.directive(MatChip))!;
chipInstance = chipDebugElement.injector.get<MatChip>(MatChip);

expect(chipInstance._hasInteractiveActions()).toBe(false);
expect(chipInstance._isRippleDisabled()).toBe(true);
});
});

describe('MatChip', () => {
Expand Down Expand Up @@ -117,18 +128,6 @@ describe('MatChip', () => {
expect(primaryAction.hasAttribute('tabindex')).toBe(false);
});

it('should disable the ripple if there are no interactive actions', () => {
// expect(chipInstance._isRippleDisabled()).toBe(false); TODO(andreyd)

// Make primary action non-interactive for testing purposes.
chipInstance.primaryAction.isInteractive = false;
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

expect(chipInstance._hasInteractiveActions()).toBe(false);
expect(chipInstance._isRippleDisabled()).toBe(true);
});

it('should return the chip text if value is undefined', () => {
expect(chipInstance.value.trim()).toBe(fixture.componentInstance.name);
});
Expand Down
10 changes: 3 additions & 7 deletions src/material/chips/chip.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import {
_animationsDisabled,
} from '../core';
import {Subject, Subscription, merge} from 'rxjs';
import {MatChipAction} from './chip-action';
import {MatChipAction, MatChipContent} from './chip-action';
import {MatChipAvatar, MatChipEdit, MatChipRemove, MatChipTrailingIcon} from './chip-icons';
import {
MAT_CHIP,
Expand Down Expand Up @@ -93,7 +93,7 @@ export interface MatChipEvent {
encapsulation: ViewEncapsulation.None,
changeDetection: ChangeDetectionStrategy.OnPush,
providers: [{provide: MAT_CHIP, useExisting: MatChip}],
imports: [MatChipAction],
imports: [MatChipContent],
})
export class MatChip implements OnInit, AfterViewInit, AfterContentInit, DoCheck, OnDestroy {
_changeDetectorRef = inject(ChangeDetectorRef);
Expand Down Expand Up @@ -386,10 +386,6 @@ export class MatChip implements OnInit, AfterViewInit, AfterContentInit, DoCheck
result.push(this.removeIcon);
}

if (this.trailingIcon) {
result.push(this.trailingIcon);
}

return result;
}

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

/** Returns whether the chip has any interactive actions. */
_hasInteractiveActions(): boolean {
return this._getActions().some(a => a.isInteractive);
return this._getActions().length > 0;
}

/** Handles interactions with the edit action of the chip. */
Expand Down
Loading