Skip to content

Commit

Permalink
fix(slide-toggle): clicks not landing correctly in some cases on Chro…
Browse files Browse the repository at this point in the history
…me (#18285)

Any time the slide toggle host receives focus we forward it immediately to the underlying `input` element. This bouncing around of focus seems to interrupt click events in some cases on Chrome. These changes make it so that we only forward focus for events originating from the keyboard or programmatically.

Fixes #18269.
  • Loading branch information
crisbeto committed Jan 29, 2020
1 parent 27ef842 commit 99b27e8
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 2 deletions.
1 change: 1 addition & 0 deletions src/material/slide-toggle/BUILD.bazel
Expand Up @@ -56,6 +56,7 @@ ng_test_library(
),
deps = [
":slide-toggle",
"//src/cdk/a11y",
"//src/cdk/bidi",
"//src/cdk/observers",
"//src/cdk/testing/private",
Expand Down
15 changes: 15 additions & 0 deletions src/material/slide-toggle/slide-toggle.spec.ts
Expand Up @@ -9,9 +9,11 @@ import {
flushMicrotasks,
TestBed,
tick,
inject,
} 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 @@ -310,6 +312,19 @@ describe('MatSlideToggle without forms', () => {
expect(document.activeElement).toBe(inputElement);
});

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

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

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

it('should set a element class if labelPosition is set to before', () => {
expect(slideToggleElement.classList).not.toContain('mat-slide-toggle-label-before');

Expand Down
9 changes: 7 additions & 2 deletions src/material/slide-toggle/slide-toggle.ts
Expand Up @@ -91,7 +91,6 @@ const _MatSlideToggleMixinBase:
'[class.mat-disabled]': 'disabled',
'[class.mat-slide-toggle-label-before]': 'labelPosition == "before"',
'[class._mat-animation-noopable]': '_animationMode === "NoopAnimations"',
'(focus)': '_inputElement.nativeElement.focus()',
},
templateUrl: 'slide-toggle.html',
styleUrls: ['slide-toggle.css'],
Expand Down Expand Up @@ -193,7 +192,13 @@ export class MatSlideToggle extends _MatSlideToggleMixinBase implements OnDestro
this._focusMonitor
.monitor(this._elementRef, true)
.subscribe(focusOrigin => {
if (!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();
} 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
Expand Down

0 comments on commit 99b27e8

Please sign in to comment.