Skip to content

Commit

Permalink
fix(material-experimental/mdc-slide-toggle): align focus behavior wit…
Browse files Browse the repository at this point in the history
…h standard version (#20772)

The focus behavior of the existing `mat-slide-toggle` was changed slightly after the MDC
version was implemented. These changes align the behavior and add some missing tests.

(cherry picked from commit f854196)
  • Loading branch information
crisbeto authored and annieyw committed Oct 16, 2020
1 parent 4d8a7c9 commit 77358cc
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 27 deletions.
1 change: 1 addition & 0 deletions src/material-experimental/mdc-slide-toggle/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ ng_test_library(
),
deps = [
":mdc-slide-toggle",
"//src/cdk/a11y",
"//src/cdk/bidi",
"//src/cdk/testing/private",
"//src/material/slide-toggle",
Expand Down
6 changes: 2 additions & 4 deletions src/material-experimental/mdc-slide-toggle/slide-toggle.html
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,12 @@
[attr.aria-label]="ariaLabel"
[attr.aria-labelledby]="ariaLabelledby"
(change)="_onChangeEvent($event)"
(click)="_onInputClick($event)"
(blur)="_onBlur()"
(focus)="_focused = true">
(click)="_onInputClick($event)">
</div>
</div>
</div>

<label [for]="inputId" (click)="$event.stopPropagation()">
<label [for]="inputId">
<ng-content></ng-content>
</label>
</div>
60 changes: 56 additions & 4 deletions src/material-experimental/mdc-slide-toggle/slide-toggle.spec.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,18 @@
import {BidiModule, Direction} from '@angular/cdk/bidi';
import {dispatchFakeEvent} from '@angular/cdk/testing/private';
import {Component} from '@angular/core';
import {ComponentFixture, fakeAsync, flushMicrotasks, TestBed, tick} from '@angular/core/testing';
import {
ComponentFixture,
fakeAsync,
flush,
flushMicrotasks,
inject,
TestBed,
tick,
} from '@angular/core/testing';
import {FormControl, FormsModule, NgModel, ReactiveFormsModule} from '@angular/forms';
import {By} from '@angular/platform-browser';
import {FocusMonitor} from '@angular/cdk/a11y';
import {MatSlideToggle, MatSlideToggleChange, MatSlideToggleModule} from './index';
import {MAT_SLIDE_TOGGLE_DEFAULT_OPTIONS} from './slide-toggle-config';

Expand Down Expand Up @@ -99,6 +108,24 @@ describe('MDC-based MatSlideToggle without forms', () => {
expect(inputElement.getAttribute('aria-checked')).toBe('true');
});

it('should not trigger the click event multiple times', fakeAsync(() => {
// By default, when clicking on a label element, a generated click will be dispatched
// on the associated input element.
// Since we're using a label element and a visual hidden input, this behavior can led
// to an issue, where the click events on the slide-toggle are getting executed twice.

expect(slideToggle.checked).toBe(false);
expect(slideToggleElement.classList).not.toContain('mat-mdc-slide-toggle-checked');

labelElement.click();
fixture.detectChanges();
tick();

expect(slideToggleElement.classList).toContain('mat-mdc-slide-toggle-checked');
expect(slideToggle.checked).toBe(true);
expect(testComponent.onSlideClick).toHaveBeenCalledTimes(1);
}));

it('should trigger the change event properly', () => {
expect(inputElement.checked).toBe(false);
expect(slideToggleElement.classList).not.toContain('mat-mdc-slide-toggle-checked');
Expand Down Expand Up @@ -261,6 +288,31 @@ describe('MDC-based MatSlideToggle without forms', () => {
expect(document.activeElement).toBe(inputElement);
});

it('should focus on underlying input element when the host is focused', fakeAsync(() => {
expect(document.activeElement).not.toBe(inputElement);

slideToggleElement.focus();
fixture.detectChanges();
tick();

expect(document.activeElement).toBe(inputElement);
}));

it('should not manually move focus to underlying input when focus comes from mouse or touch',
fakeAsync(inject([FocusMonitor], (focusMonitor: FocusMonitor) => {
expect(document.activeElement).not.toBe(inputElement);

focusMonitor.focusVia(slideToggleElement, 'mouse');
fixture.detectChanges();
flush();
expect(document.activeElement).not.toBe(inputElement);

focusMonitor.focusVia(slideToggleElement, 'touch');
fixture.detectChanges();
flush();
expect(document.activeElement).not.toBe(inputElement);
})));

it('should set a element class if labelPosition is set to before', () => {
const formField = slideToggleElement.querySelector('.mdc-form-field')!;

Expand All @@ -272,7 +324,7 @@ describe('MDC-based MatSlideToggle without forms', () => {
expect(formField.classList).toContain('mdc-form-field--align-end');
});

it('should show ripples on switch element', () => {
it('should show ripples', () => {
const rippleSelector = '.mat-ripple-element';
const switchElement = slideToggleElement.querySelector('.mdc-switch')!;

Expand Down Expand Up @@ -342,13 +394,13 @@ describe('MDC-based MatSlideToggle without forms', () => {
expect(switchEl.classList).toContain('mdc-switch--checked');
});

it('should remove the tabindex from the host element', fakeAsync(() => {
it('should set the tabindex of the host element to -1', fakeAsync(() => {
const fixture = TestBed.createComponent(SlideToggleWithTabindexAttr);

fixture.detectChanges();

const slideToggle = fixture.debugElement.query(By.directive(MatSlideToggle))!.nativeElement;
expect(slideToggle.hasAttribute('tabindex')).toBe(false);
expect(slideToggle.getAttribute('tabindex')).toBe('-1');
}));

it('should remove the tabindex from the host element when disabled', fakeAsync(() => {
Expand Down
49 changes: 32 additions & 17 deletions src/material-experimental/mdc-slide-toggle/slide-toggle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
import {ANIMATION_MODULE_TYPE} from '@angular/platform-browser/animations';
import {ThemePalette, RippleAnimationConfig} from '@angular/material/core';
import {numbers} from '@material/ripple';
import {FocusMonitor} from '@angular/cdk/a11y';
import {
MAT_SLIDE_TOGGLE_DEFAULT_OPTIONS,
MatSlideToggleDefaultOptions,
Expand Down Expand Up @@ -71,7 +72,8 @@ export class MatSlideToggleChange {
host: {
'class': 'mat-mdc-slide-toggle',
'[id]': 'id',
'[attr.tabindex]': 'null',
// Needs to be `-1` so it can still receive programmatic focus.
'[attr.tabindex]': 'disabled ? null : -1',
'[attr.aria-label]': 'null',
'[attr.aria-labelledby]': 'null',
'[class.mat-primary]': 'color === "primary"',
Expand All @@ -80,7 +82,6 @@ export class MatSlideToggleChange {
'[class.mat-mdc-slide-toggle-focused]': '_focused',
'[class.mat-mdc-slide-toggle-checked]': 'checked',
'[class._mat-animation-noopable]': '_animationMode === "NoopAnimations"',
'(focus)': '_inputElement.nativeElement.focus()',
},
exportAs: 'matSlideToggle',
encapsulation: ViewEncapsulation.None,
Expand Down Expand Up @@ -194,7 +195,9 @@ export class MatSlideToggle implements ControlValueAccessor, AfterViewInit, OnDe
/** Reference to the MDC switch element. */
@ViewChild('switch') _switchElement: ElementRef<HTMLElement>;

constructor(private _changeDetectorRef: ChangeDetectorRef,
constructor(private _elementRef: ElementRef,
private _focusMonitor: FocusMonitor,
private _changeDetectorRef: ChangeDetectorRef,
@Attribute('tabindex') tabIndex: string,
@Inject(MAT_SLIDE_TOGGLE_DEFAULT_OPTIONS)
public defaults: MatSlideToggleDefaultOptions,
Expand All @@ -206,9 +209,35 @@ export class MatSlideToggle implements ControlValueAccessor, AfterViewInit, OnDe
const foundation = this._foundation = new MDCSwitchFoundation(this._adapter);
foundation.setDisabled(this.disabled);
foundation.setChecked(this.checked);

this._focusMonitor
.monitor(this._elementRef, true)
.subscribe(focusOrigin => {
// Only forward focus manually when it was received programmatically or through the
// keyboard. We should not do this for mouse/touch focus for two reasons:
// 1. It can prevent clicks from landing in Chrome (see #18269).
// 2. They're already handled by the wrapping `label` element.
if (focusOrigin === 'keyboard' || focusOrigin === 'program') {
this._inputElement.nativeElement.focus();
this._focused = true;
} else if (!focusOrigin) {
// When a focused element becomes disabled, the browser *immediately* fires a blur event.
// Angular does not expect events to be raised during change detection, so any state
// change (such as a form control's ng-touched) will cause a changed-after-checked error.
// See https://github.com/angular/angular/issues/17793. To work around this, we defer
// telling the form control it has been touched until the next tick.
Promise.resolve().then(() => {
this._focused = false;
this._onTouched();
this._changeDetectorRef.markForCheck();
});
}
});
}

ngOnDestroy() {
this._focusMonitor.stopMonitoring(this._elementRef);

if (this._foundation) {
this._foundation.destroy();
}
Expand Down Expand Up @@ -285,20 +314,6 @@ export class MatSlideToggle implements ControlValueAccessor, AfterViewInit, OnDe
this._onChange(this.checked);
}

/** Handles blur events on the native input. */
_onBlur() {
// When a focused element becomes disabled, the browser *immediately* fires a blur event.
// Angular does not expect events to be raised during change detection, so any state change
// (such as a form control's 'ng-touched') will cause a changed-after-checked error.
// See https://github.com/angular/angular/issues/17793. To work around this, we defer
// telling the form control it has been touched until the next tick.
Promise.resolve().then(() => {
this._focused = false;
this._onTouched();
this._changeDetectorRef.markForCheck();
});
}

static ngAcceptInputType_tabIndex: NumberInput;
static ngAcceptInputType_required: BooleanInput;
static ngAcceptInputType_checked: BooleanInput;
Expand Down
5 changes: 3 additions & 2 deletions src/material/slide-toggle/slide-toggle.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ describe('MatSlideToggle without forms', () => {
expect(slideToggleElement.classList).toContain('mat-slide-toggle-label-before');
});

it('should show ripples on label mousedown', () => {
it('should show ripples', () => {
const rippleSelector = '.mat-ripple-element:not(.mat-slide-toggle-persistent-ripple)';

expect(slideToggleElement.querySelectorAll(rippleSelector).length).toBe(0);
Expand Down Expand Up @@ -408,7 +408,8 @@ describe('MatSlideToggle without forms', () => {
});

describe('custom action configuration', () => {
it('should not change value on click when click action is noop', () => {
it('should not change value on click when click action is noop when using custom a ' +
'action configuration', () => {
TestBed
.resetTestingModule()
.configureTestingModule({
Expand Down

0 comments on commit 77358cc

Please sign in to comment.