From d43c144f40cd450037ee04018fa76af5cb660c24 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sat, 27 Jan 2018 16:05:00 +0100 Subject: [PATCH 1/2] fix(focus-origin): focus origin sometimes invalid in firefox 57 * Sometimes the focus origin is not valid because Firefox seems to focus a tick after the keyboard/interaction event fired a tick before. To ensure the focus origin is always correct, the focus origin will be added to the next tick event loop Fixes #6984 --- .../a11y/focus-monitor/focus-monitor.spec.ts | 32 +++++++++++++------ src/cdk/a11y/focus-monitor/focus-monitor.ts | 7 ++-- src/lib/menu/menu.spec.ts | 14 +++++--- src/lib/sidenav/drawer.spec.ts | 27 ++++++++-------- src/lib/tooltip/tooltip.spec.ts | 4 ++- 5 files changed, 55 insertions(+), 29 deletions(-) diff --git a/src/cdk/a11y/focus-monitor/focus-monitor.spec.ts b/src/cdk/a11y/focus-monitor/focus-monitor.spec.ts index 5d443fb24610..633cdd329ce9 100644 --- a/src/cdk/a11y/focus-monitor/focus-monitor.spec.ts +++ b/src/cdk/a11y/focus-monitor/focus-monitor.spec.ts @@ -54,7 +54,7 @@ describe('FocusMonitor', () => { dispatchKeyboardEvent(document, 'keydown', TAB); buttonElement.focus(); fixture.detectChanges(); - tick(); + flush(); expect(buttonElement.classList.length) .toBe(2, 'button should have exactly 2 focus classes'); @@ -70,7 +70,7 @@ describe('FocusMonitor', () => { dispatchMouseEvent(buttonElement, 'mousedown'); buttonElement.focus(); fixture.detectChanges(); - tick(); + flush(); expect(buttonElement.classList.length) .toBe(2, 'button should have exactly 2 focus classes'); @@ -114,7 +114,7 @@ describe('FocusMonitor', () => { it('focusVia keyboard should simulate keyboard focus', fakeAsync(() => { focusMonitor.focusVia(buttonElement, 'keyboard'); - tick(); + flush(); expect(buttonElement.classList.length) .toBe(2, 'button should have exactly 2 focus classes'); @@ -128,7 +128,7 @@ describe('FocusMonitor', () => { it('focusVia mouse should simulate mouse focus', fakeAsync(() => { focusMonitor.focusVia(buttonElement, 'mouse'); fixture.detectChanges(); - tick(); + flush(); expect(buttonElement.classList.length) .toBe(2, 'button should have exactly 2 focus classes'); @@ -142,7 +142,7 @@ describe('FocusMonitor', () => { it('focusVia mouse should simulate mouse focus', fakeAsync(() => { focusMonitor.focusVia(buttonElement, 'touch'); fixture.detectChanges(); - tick(); + flush(); expect(buttonElement.classList.length) .toBe(2, 'button should have exactly 2 focus classes'); @@ -156,7 +156,7 @@ describe('FocusMonitor', () => { it('focusVia program should simulate programmatic focus', fakeAsync(() => { focusMonitor.focusVia(buttonElement, 'program'); fixture.detectChanges(); - tick(); + flush(); expect(buttonElement.classList.length) .toBe(2, 'button should have exactly 2 focus classes'); @@ -218,13 +218,27 @@ describe('FocusMonitor', () => { focusMonitor.focusVia(buttonElement, 'program', {preventScroll: true}); fixture.detectChanges(); - tick(); + flush(); expect(buttonElement.focus).toHaveBeenCalledWith(jasmine.objectContaining({ preventScroll: true })); })); + it('should not clear the focus origin too early in the current event loop', fakeAsync(() => { + dispatchKeyboardEvent(document, 'keydown', TAB); + + // Simulate the behavior of Firefox 57 where the focus event sometimes happens *one* tick later. + tick(); + + buttonElement.focus(); + + // Since the timeout doesn't clear the focus origin too early as with the `0ms` timeout, the + // focus origin should be reported properly. + expect(changeHandler).toHaveBeenCalledWith('keyboard'); + + flush(); + })); }); @@ -263,7 +277,7 @@ describe('cdkMonitorFocus', () => { dispatchKeyboardEvent(document, 'keydown', TAB); buttonElement.focus(); fixture.detectChanges(); - tick(); + flush(); expect(buttonElement.classList.length) .toBe(2, 'button should have exactly 2 focus classes'); @@ -279,7 +293,7 @@ describe('cdkMonitorFocus', () => { dispatchMouseEvent(buttonElement, 'mousedown'); buttonElement.focus(); fixture.detectChanges(); - tick(); + flush(); expect(buttonElement.classList.length) .toBe(2, 'button should have exactly 2 focus classes'); diff --git a/src/cdk/a11y/focus-monitor/focus-monitor.ts b/src/cdk/a11y/focus-monitor/focus-monitor.ts index a2cb0c337197..f5ad36f973a7 100644 --- a/src/cdk/a11y/focus-monitor/focus-monitor.ts +++ b/src/cdk/a11y/focus-monitor/focus-monitor.ts @@ -185,7 +185,7 @@ export class FocusMonitor implements OnDestroy { }; // When the touchstart event fires the focus event is not yet in the event queue. This means - // we can't rely on the trick used above (setting timeout of 0ms). Instead we wait 650ms to + // we can't rely on the trick used above (setting timeout of 1ms). Instead we wait 650ms to // see if a focus happens. let documentTouchstartListener = (event: TouchEvent) => { if (this._touchTimeoutId != null) { @@ -258,7 +258,10 @@ export class FocusMonitor implements OnDestroy { private _setOriginForCurrentEventQueue(origin: FocusOrigin): void { this._ngZone.runOutsideAngular(() => { this._origin = origin; - this._originTimeoutId = setTimeout(() => this._origin = null); + // Sometimes the focus origin won't be valid in Firefox because Firefox seems to focus *one* + // tick after the interaction event fired. To ensure the focus origin is always correct, + // the focus origin will be determined at the beginning of the next tick. + this._originTimeoutId = setTimeout(() => this._origin = null, 1); }); } diff --git a/src/lib/menu/menu.spec.ts b/src/lib/menu/menu.spec.ts index aa4076975c57..c3a10a41e981 100644 --- a/src/lib/menu/menu.spec.ts +++ b/src/lib/menu/menu.spec.ts @@ -1,4 +1,4 @@ -import {ComponentFixture, fakeAsync, inject, TestBed, tick} from '@angular/core/testing'; +import {ComponentFixture, fakeAsync, flush, inject, TestBed, tick} from '@angular/core/testing'; import {By} from '@angular/platform-browser'; import {NoopAnimationsModule} from '@angular/platform-browser/animations'; import { @@ -176,7 +176,9 @@ describe('MatMenu', () => { dispatchFakeEvent(triggerEl, 'mousedown'); triggerEl.click(); fixture.detectChanges(); - tick(); + + // Flush due to the additional tick that is necessary for the FocusMonitor. + flush(); expect(overlayContainerElement.querySelector('.mat-menu-panel')!.scrollTop).toBe(0); })); @@ -398,7 +400,9 @@ describe('MatMenu', () => { dispatchKeyboardEvent(panel, 'keydown', DOWN_ARROW); fixture.detectChanges(); - tick(); + + // Flush due to the additional tick that is necessary for the FocusMonitor. + flush(); // We skip to the third item, because the second one is disabled. expect(items[2].classList).toContain('cdk-focused'); @@ -495,7 +499,9 @@ describe('MatMenu', () => { fixture.detectChanges(); tick(500); zone!.simulateZoneExit(); - tick(); + + // Flush due to the additional tick that is necessary for the FocusMonitor. + flush(); const item = document.querySelector('.mat-menu-panel [mat-menu-item]')!; diff --git a/src/lib/sidenav/drawer.spec.ts b/src/lib/sidenav/drawer.spec.ts index fa849db920aa..479f79ee48dc 100644 --- a/src/lib/sidenav/drawer.spec.ts +++ b/src/lib/sidenav/drawer.spec.ts @@ -5,6 +5,7 @@ import { ComponentFixture, TestBed, discardPeriodicTasks, + flush, } from '@angular/core/testing'; import {Component, ElementRef, ViewChild} from '@angular/core'; import {By} from '@angular/platform-browser'; @@ -75,7 +76,7 @@ describe('MatDrawer', () => { drawerBackdropElement.nativeElement.style.transition = 'none'; fixture.debugElement.query(By.css('.open')).nativeElement.click(); fixture.detectChanges(); - tick(); + flush(); fixture.detectChanges(); fixture.debugElement.query(By.css('.close')).nativeElement.click(); @@ -84,7 +85,7 @@ describe('MatDrawer', () => { expect(testComponent.closeCount).toBe(0); expect(testComponent.closeStartCount).toBe(0); - tick(); + flush(); expect(testComponent.closeStartCount).toBe(1); fixture.detectChanges(); @@ -112,12 +113,12 @@ describe('MatDrawer', () => { drawer.componentInstance.open(); fixture.detectChanges(); - tick(); + flush(); fixture.detectChanges(); drawer.componentInstance.close().then(result => expect(result).toBe('close')); fixture.detectChanges(); - tick(); + flush(); fixture.detectChanges(); })); @@ -136,7 +137,7 @@ describe('MatDrawer', () => { fixture.debugElement.query(By.css('.close')).nativeElement.click(); fixture.detectChanges(); - tick(); + flush(); fixture.detectChanges(); expect(testComponent.openCount).toBe(1); @@ -160,23 +161,23 @@ describe('MatDrawer', () => { openButtonElement.click(); fixture.detectChanges(); - tick(); + flush(); expect(testComponent.backdropClickedCount).toBe(0); fixture.debugElement.query(By.css('.mat-drawer-backdrop')).nativeElement.click(); fixture.detectChanges(); - tick(); + flush(); expect(testComponent.backdropClickedCount).toBe(1); openButtonElement.click(); fixture.detectChanges(); - tick(); + flush(); fixture.debugElement.query(By.css('.close')).nativeElement.click(); fixture.detectChanges(); - tick(); + flush(); expect(testComponent.backdropClickedCount).toBe(1); })); @@ -200,7 +201,7 @@ describe('MatDrawer', () => { dispatchKeyboardEvent(drawer.nativeElement, 'keydown', ESCAPE); fixture.detectChanges(); - tick(); + flush(); expect(testComponent.closeCount).toBe(1, 'Expected one close event.'); expect(testComponent.closeStartCount).toBe(1, 'Expected one close start event.'); @@ -263,12 +264,12 @@ describe('MatDrawer', () => { openButton.focus(); drawer.open(); fixture.detectChanges(); - tick(); + flush(); drawerButton.focus(); drawer.close(); fixture.detectChanges(); - tick(); + flush(); expect(document.activeElement) .toBe(openButton, 'Expected focus to be restored to the open button on close.'); @@ -521,7 +522,7 @@ describe('MatDrawerContainer', () => { testComponent.drawerContainer.close(); fixture.detectChanges(); - tick(); + flush(); expect(drawers.every(drawer => drawer.componentInstance.opened)).toBe(false); })); diff --git a/src/lib/tooltip/tooltip.spec.ts b/src/lib/tooltip/tooltip.spec.ts index c7b49af59bfa..4206fa1eeb19 100644 --- a/src/lib/tooltip/tooltip.spec.ts +++ b/src/lib/tooltip/tooltip.spec.ts @@ -2,6 +2,7 @@ import { async, ComponentFixture, fakeAsync, + flush, flushMicrotasks, inject, TestBed, @@ -575,7 +576,8 @@ describe('MatTooltip', () => { fixture.detectChanges(); }).not.toThrow(); - tick(0); + // Flush due to the additional tick that is necessary for the FocusMonitor. + flush(); })); it('should not show the tooltip on progammatic focus', fakeAsync(() => { From c79df28a5bcee41d429a3cdacaf8a40be05dab1a Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 3 Jul 2018 20:35:14 +0200 Subject: [PATCH 2/2] Fix test error --- src/lib/sidenav/drawer.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/sidenav/drawer.spec.ts b/src/lib/sidenav/drawer.spec.ts index 479f79ee48dc..be35dfe6418d 100644 --- a/src/lib/sidenav/drawer.spec.ts +++ b/src/lib/sidenav/drawer.spec.ts @@ -670,7 +670,7 @@ describe('MatDrawerContainer', () => { // Close the drawer and resolve the close animation. fixture.componentInstance.drawer.close(); fixture.detectChanges(); - tick(); + flush(); fixture.detectChanges(); expect(content.style.marginLeft).toBe('', 'Margin should be removed after drawer close.');