Skip to content

fix(focus-origin): focus origin sometimes invalid in firefox 57 #8669

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

Merged
merged 2 commits into from
Jul 10, 2018
Merged
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
32 changes: 23 additions & 9 deletions src/cdk/a11y/focus-monitor/focus-monitor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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');
Expand Down Expand Up @@ -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');
Expand All @@ -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');
Expand All @@ -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');
Expand All @@ -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');
Expand Down Expand Up @@ -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();
}));
});


Expand Down Expand Up @@ -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');
Expand All @@ -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');
Expand Down
7 changes: 5 additions & 2 deletions src/cdk/a11y/focus-monitor/focus-monitor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
});
}

Expand Down
14 changes: 10 additions & 4 deletions src/lib/menu/menu.spec.ts
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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);
}));
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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]')!;

Expand Down
29 changes: 15 additions & 14 deletions src/lib/sidenav/drawer.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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();
Expand All @@ -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();

Expand Down Expand Up @@ -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();
}));

Expand All @@ -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);
Expand All @@ -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);
}));
Expand All @@ -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.');
Expand Down Expand Up @@ -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.');
Expand Down Expand Up @@ -521,7 +522,7 @@ describe('MatDrawerContainer', () => {

testComponent.drawerContainer.close();
fixture.detectChanges();
tick();
flush();

expect(drawers.every(drawer => drawer.componentInstance.opened)).toBe(false);
}));
Expand Down Expand Up @@ -669,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.');
Expand Down
4 changes: 3 additions & 1 deletion src/lib/tooltip/tooltip.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {
async,
ComponentFixture,
fakeAsync,
flush,
flushMicrotasks,
inject,
TestBed,
Expand Down Expand Up @@ -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(() => {
Expand Down