From 3675276f22a925a2341aed6ecfbca7b68013db83 Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Tue, 7 Feb 2017 14:33:38 -0800 Subject: [PATCH 1/7] added tests --- src/lib/core/style/focus-classes.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/core/style/focus-classes.ts b/src/lib/core/style/focus-classes.ts index e8f0057f0ff7..0a813cf104b5 100644 --- a/src/lib/core/style/focus-classes.ts +++ b/src/lib/core/style/focus-classes.ts @@ -74,8 +74,8 @@ export class FocusOriginMonitor { renderer.setElementClass(element, 'cdk-keyboard-focused', this._origin == 'keyboard'); renderer.setElementClass(element, 'cdk-mouse-focused', this._origin == 'mouse'); renderer.setElementClass(element, 'cdk-program-focused', this._origin == 'program'); - subject.next(this._origin); + this._lastFocusOrigin = this._origin; this._origin = null; } From f37d0122660e810402fccef63cab2a045efa8ea1 Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Thu, 9 Feb 2017 12:15:16 -0800 Subject: [PATCH 2/7] add touch support to FocusOriginMonitor --- src/demo-app/style/style-demo.html | 1 + src/demo-app/style/style-demo.scss | 4 ++ src/lib/core/style/focus-classes.ts | 74 +++++++++++++++++++++++++---- 3 files changed, 71 insertions(+), 8 deletions(-) diff --git a/src/demo-app/style/style-demo.html b/src/demo-app/style/style-demo.html index a3aee1f7bb3f..66c6ad6641dd 100644 --- a/src/demo-app/style/style-demo.html +++ b/src/demo-app/style/style-demo.html @@ -2,6 +2,7 @@ + diff --git a/src/demo-app/style/style-demo.scss b/src/demo-app/style/style-demo.scss index a04570ce0dee..95b98232e98f 100644 --- a/src/demo-app/style/style-demo.scss +++ b/src/demo-app/style/style-demo.scss @@ -13,3 +13,7 @@ .demo-button.cdk-program-focused { background: blue; } + +.demo-button.cdk-touch-focused { + background: purple; +} diff --git a/src/lib/core/style/focus-classes.ts b/src/lib/core/style/focus-classes.ts index 0a813cf104b5..92063862234d 100644 --- a/src/lib/core/style/focus-classes.ts +++ b/src/lib/core/style/focus-classes.ts @@ -3,7 +3,10 @@ import {Observable} from 'rxjs/Observable'; import {Subject} from 'rxjs/Subject'; -export type FocusOrigin = 'mouse' | 'keyboard' | 'program'; +const TOUCH_BUFFER_MS = 650; + + +export type FocusOrigin = 'touch' | 'mouse' | 'keyboard' | 'program'; /** Monitors mouse and keyboard events to determine the cause of focus events. */ @@ -18,14 +21,59 @@ export class FocusOriginMonitor { /** Whether the window has just been focused. */ private _windowFocused = false; + /** Whether the current sequence of events was kicked off by a touch event. */ + private get _touchActive() { return this._touchActiveBacking; } + private set _touchActive(value: boolean) { + this._touchActiveBacking = value; + + if (this._touchActiveBacking) { + // Register a listener to unset the backing variable after a focus followed by a blur. + // This is necessary to avoid accidentally counting a programmatic focus that occurs on a + // touchstart event from being counted as a touch focus. + document.addEventListener('focus', this._touchFocusListener, true); + } else { + // Remove the focus and blur listeners so they don't interfere in the future. + document.removeEventListener('focus', this._touchFocusListener, true); + document.removeEventListener('blur', this._touchBlurListener, true); + } + } + private _touchActiveBacking = false; + + /** A focus listener that adds _touchBlurListener listener and then uninstalls itself. */ + private _touchFocusListener = () => { + document.addEventListener('blur', this._touchBlurListener, true); + document.removeEventListener('focus', this._touchFocusListener, true); + }; + + /** Blur listener that unsets the _touchActiveBacking and origin and then uninstalls itself. */ + private _touchBlurListener = () => { + this._touchActiveBacking = false; + this._origin = null; + document.removeEventListener('blur', this._touchBlurListener, true); + }; + constructor() { - // Listen to keydown and mousedown in the capture phase so we can detect them even if the user - // stops propagation. - // TODO(mmalerba): Figure out how to handle touchstart - document.addEventListener( - 'keydown', () => this._setOriginForCurrentEventQueue('keyboard'), true); - document.addEventListener( - 'mousedown', () => this._setOriginForCurrentEventQueue('mouse'), true); + // Note: we listen to events in the capture phase so we can detect them even if the user stops + // propagation. + + // On keydown record the origin and cancel any touch event that may be in progress. + document.addEventListener('keydown', () => { + this._touchActive = false; + this._setOriginForCurrentEventQueue('keyboard'); + }, true); + + // On mousedown record the origin, but do not cancel the touch event. A mousedown can happen as + // a result of a touch event. + document.addEventListener('mousedown', + () => this._setOriginForCurrentEventQueue('mouse'), true); + + // 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 see if + // a focus happens. + document.addEventListener('touchstart', () => { + this._touchActive = true; + setTimeout(() => this._touchActive = false, TOUCH_BUFFER_MS); + }, true); // Make a note of when the window regains focus, so we can restore the origin info for the // focused element. @@ -45,6 +93,10 @@ export class FocusOriginMonitor { /** Focuses the element via the specified focus origin. */ focusVia(element: Node, renderer: Renderer, origin: FocusOrigin) { + // This method may have been called as a result of a touchstart event, so we need to clear + // _touchActive to prevent it from interfering. + this._touchActive = false; + this._setOriginForCurrentEventQueue(origin); renderer.invokeElementMethod(element, 'focus'); } @@ -57,6 +109,10 @@ export class FocusOriginMonitor { /** Handles focus events on a registered element. */ private _onFocus(element: Element, renderer: Renderer, subject: Subject) { + if (this._touchActive) { + this._origin = 'touch'; + } + // If we couldn't detect a cause for the focus event, it's due to one of two reasons: // 1) The window has just regained focus, in which case we want to restore the focused state of // the element from before the window blurred. @@ -71,6 +127,7 @@ export class FocusOriginMonitor { } renderer.setElementClass(element, 'cdk-focused', true); + renderer.setElementClass(element, 'cdk-touch-focused', this._origin == 'touch'); renderer.setElementClass(element, 'cdk-keyboard-focused', this._origin == 'keyboard'); renderer.setElementClass(element, 'cdk-mouse-focused', this._origin == 'mouse'); renderer.setElementClass(element, 'cdk-program-focused', this._origin == 'program'); @@ -83,6 +140,7 @@ export class FocusOriginMonitor { /** Handles blur events on a registered element. */ private _onBlur(element: Element, renderer: Renderer, subject: Subject) { renderer.setElementClass(element, 'cdk-focused', false); + renderer.setElementClass(element, 'cdk-touch-focused', false); renderer.setElementClass(element, 'cdk-keyboard-focused', false); renderer.setElementClass(element, 'cdk-mouse-focused', false); renderer.setElementClass(element, 'cdk-program-focused', false); From 7ff00e44230e44ace2e434ec55e4fa78a95ef575 Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Thu, 9 Feb 2017 12:37:38 -0800 Subject: [PATCH 3/7] add tests --- src/lib/core/style/focus-classes.spec.ts | 62 ++++++++++++++++++++++++ src/lib/core/style/focus-classes.ts | 4 +- 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/src/lib/core/style/focus-classes.spec.ts b/src/lib/core/style/focus-classes.spec.ts index 2ef8c0e9fd9c..80e7a5b101e4 100644 --- a/src/lib/core/style/focus-classes.spec.ts +++ b/src/lib/core/style/focus-classes.spec.ts @@ -90,6 +90,25 @@ describe('FocusOriginMonitor', () => { }, 0); })); + it('should detect focus via touch', async(() => { + // Simulate focus via touch. + dispatchTouchstartEvent(document); + buttonElement.focus(); + fixture.detectChanges(); + + setTimeout(() => { + fixture.detectChanges(); + + expect(buttonElement.classList.length) + .toBe(2, 'button should have exactly 2 focus classes'); + expect(buttonElement.classList.contains('cdk-focused')) + .toBe(true, 'button should have cdk-focused class'); + expect(buttonElement.classList.contains('cdk-touch-focused')) + .toBe(true, 'button should have cdk-touch-focused class'); + expect(changeHandler).toHaveBeenCalledWith('touch'); + }, 650); + })); + it('should detect programmatic focus', async(() => { // Programmatically focus. buttonElement.focus(); @@ -142,6 +161,23 @@ describe('FocusOriginMonitor', () => { }, 0); })); + it('focusVia mouse should simulate mouse focus', async(() => { + focusOriginMonitor.focusVia(buttonElement, buttonRenderer, 'touch'); + fixture.detectChanges(); + + setTimeout(() => { + fixture.detectChanges(); + + expect(buttonElement.classList.length) + .toBe(2, 'button should have exactly 2 focus classes'); + expect(buttonElement.classList.contains('cdk-focused')) + .toBe(true, 'button should have cdk-focused class'); + expect(buttonElement.classList.contains('cdk-touch-focused')) + .toBe(true, 'button should have cdk-touch-focused class'); + expect(changeHandler).toHaveBeenCalledWith('touch'); + }, 0); + })); + it('focusVia program should simulate programmatic focus', async(() => { focusOriginMonitor.focusVia(buttonElement, buttonRenderer, 'program'); fixture.detectChanges(); @@ -251,6 +287,25 @@ describe('cdkFocusClasses', () => { }, 0); })); + it('should detect focus via touch', async(() => { + // Simulate focus via touch. + dispatchTouchstartEvent(document); + buttonElement.focus(); + fixture.detectChanges(); + + setTimeout(() => { + fixture.detectChanges(); + + expect(buttonElement.classList.length) + .toBe(2, 'button should have exactly 2 focus classes'); + expect(buttonElement.classList.contains('cdk-focused')) + .toBe(true, 'button should have cdk-focused class'); + expect(buttonElement.classList.contains('cdk-touch-focused')) + .toBe(true, 'button should have cdk-touch-focused class'); + expect(changeHandler).toHaveBeenCalledWith('touch'); + }, 650); + })); + it('should detect programmatic focus', async(() => { // Programmatically focus. buttonElement.focus(); @@ -312,6 +367,13 @@ function dispatchMousedownEvent(element: Node) { element.dispatchEvent(event); } +/** Dispatches a mousedown event on the specified element. */ +function dispatchTouchstartEvent(element: Node) { + let event = document.createEvent('MouseEvent'); + event.initMouseEvent( + 'touchstart', true, true, window, 0, 0, 0, 0, 0, false, false, false, false, 0, null); + element.dispatchEvent(event); +} /** Dispatches a keydown event on the specified element. */ function dispatchKeydownEvent(element: Node, keyCode: number) { diff --git a/src/lib/core/style/focus-classes.ts b/src/lib/core/style/focus-classes.ts index 92063862234d..e783fad70dd7 100644 --- a/src/lib/core/style/focus-classes.ts +++ b/src/lib/core/style/focus-classes.ts @@ -43,14 +43,14 @@ export class FocusOriginMonitor { private _touchFocusListener = () => { document.addEventListener('blur', this._touchBlurListener, true); document.removeEventListener('focus', this._touchFocusListener, true); - }; + } /** Blur listener that unsets the _touchActiveBacking and origin and then uninstalls itself. */ private _touchBlurListener = () => { this._touchActiveBacking = false; this._origin = null; document.removeEventListener('blur', this._touchBlurListener, true); - }; + } constructor() { // Note: we listen to events in the capture phase so we can detect them even if the user stops From 38103873ee344e5760e2f7c41dd64b3858a11eea Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Fri, 10 Feb 2017 11:04:24 -0800 Subject: [PATCH 4/7] added comment about 650ms --- src/lib/core/style/focus-classes.spec.ts | 6 +++--- src/lib/core/style/focus-classes.ts | 4 +++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/lib/core/style/focus-classes.spec.ts b/src/lib/core/style/focus-classes.spec.ts index 80e7a5b101e4..f7b113da93d3 100644 --- a/src/lib/core/style/focus-classes.spec.ts +++ b/src/lib/core/style/focus-classes.spec.ts @@ -3,7 +3,7 @@ import {Component, Renderer, ViewChild} from '@angular/core'; import {StyleModule} from './index'; import {By} from '@angular/platform-browser'; import {TAB} from '../keyboard/keycodes'; -import {FocusOriginMonitor, FocusOrigin, CdkFocusClasses} from './focus-classes'; +import {FocusOriginMonitor, FocusOrigin, CdkFocusClasses, TOUCH_BUFFER_MS} from './focus-classes'; describe('FocusOriginMonitor', () => { let fixture: ComponentFixture; @@ -106,7 +106,7 @@ describe('FocusOriginMonitor', () => { expect(buttonElement.classList.contains('cdk-touch-focused')) .toBe(true, 'button should have cdk-touch-focused class'); expect(changeHandler).toHaveBeenCalledWith('touch'); - }, 650); + }, TOUCH_BUFFER_MS); })); it('should detect programmatic focus', async(() => { @@ -303,7 +303,7 @@ describe('cdkFocusClasses', () => { expect(buttonElement.classList.contains('cdk-touch-focused')) .toBe(true, 'button should have cdk-touch-focused class'); expect(changeHandler).toHaveBeenCalledWith('touch'); - }, 650); + }, TOUCH_BUFFER_MS); })); it('should detect programmatic focus', async(() => { diff --git a/src/lib/core/style/focus-classes.ts b/src/lib/core/style/focus-classes.ts index e783fad70dd7..31d0b67569bb 100644 --- a/src/lib/core/style/focus-classes.ts +++ b/src/lib/core/style/focus-classes.ts @@ -3,7 +3,9 @@ import {Observable} from 'rxjs/Observable'; import {Subject} from 'rxjs/Subject'; -const TOUCH_BUFFER_MS = 650; +// This is the value used by AngularJS Material. Through trial and error (on iPhone 6S) they found +// that a value of around 650ms seems appropriate. +export const TOUCH_BUFFER_MS = 650; export type FocusOrigin = 'touch' | 'mouse' | 'keyboard' | 'program'; From 0f11366dcedf1f9b22fbac7a7c5510ae84681b66 Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Fri, 10 Feb 2017 11:56:33 -0800 Subject: [PATCH 5/7] switch to comparing event targets instead of checking focus -> blur. --- src/lib/core/style/focus-classes.spec.ts | 8 +-- src/lib/core/style/focus-classes.ts | 81 ++++++++++-------------- 2 files changed, 36 insertions(+), 53 deletions(-) diff --git a/src/lib/core/style/focus-classes.spec.ts b/src/lib/core/style/focus-classes.spec.ts index f7b113da93d3..02cd142a7507 100644 --- a/src/lib/core/style/focus-classes.spec.ts +++ b/src/lib/core/style/focus-classes.spec.ts @@ -73,7 +73,7 @@ describe('FocusOriginMonitor', () => { it('should detect focus via mouse', async(() => { // Simulate focus via mouse. - dispatchMousedownEvent(document); + dispatchMousedownEvent(buttonElement); buttonElement.focus(); fixture.detectChanges(); @@ -92,7 +92,7 @@ describe('FocusOriginMonitor', () => { it('should detect focus via touch', async(() => { // Simulate focus via touch. - dispatchTouchstartEvent(document); + dispatchTouchstartEvent(buttonElement); buttonElement.focus(); fixture.detectChanges(); @@ -270,7 +270,7 @@ describe('cdkFocusClasses', () => { it('should detect focus via mouse', async(() => { // Simulate focus via mouse. - dispatchMousedownEvent(document); + dispatchMousedownEvent(buttonElement); buttonElement.focus(); fixture.detectChanges(); @@ -289,7 +289,7 @@ describe('cdkFocusClasses', () => { it('should detect focus via touch', async(() => { // Simulate focus via touch. - dispatchTouchstartEvent(document); + dispatchTouchstartEvent(buttonElement); buttonElement.focus(); fixture.detectChanges(); diff --git a/src/lib/core/style/focus-classes.ts b/src/lib/core/style/focus-classes.ts index 31d0b67569bb..1c6419aa34c2 100644 --- a/src/lib/core/style/focus-classes.ts +++ b/src/lib/core/style/focus-classes.ts @@ -23,58 +23,38 @@ export class FocusOriginMonitor { /** Whether the window has just been focused. */ private _windowFocused = false; - /** Whether the current sequence of events was kicked off by a touch event. */ - private get _touchActive() { return this._touchActiveBacking; } - private set _touchActive(value: boolean) { - this._touchActiveBacking = value; - - if (this._touchActiveBacking) { - // Register a listener to unset the backing variable after a focus followed by a blur. - // This is necessary to avoid accidentally counting a programmatic focus that occurs on a - // touchstart event from being counted as a touch focus. - document.addEventListener('focus', this._touchFocusListener, true); - } else { - // Remove the focus and blur listeners so they don't interfere in the future. - document.removeEventListener('focus', this._touchFocusListener, true); - document.removeEventListener('blur', this._touchBlurListener, true); - } - } - private _touchActiveBacking = false; + /** The target of the last touch event. */ + private _lastTouchTarget: EventTarget; - /** A focus listener that adds _touchBlurListener listener and then uninstalls itself. */ - private _touchFocusListener = () => { - document.addEventListener('blur', this._touchBlurListener, true); - document.removeEventListener('focus', this._touchFocusListener, true); - } - - /** Blur listener that unsets the _touchActiveBacking and origin and then uninstalls itself. */ - private _touchBlurListener = () => { - this._touchActiveBacking = false; - this._origin = null; - document.removeEventListener('blur', this._touchBlurListener, true); - } + private _touchTimeout: number; constructor() { // Note: we listen to events in the capture phase so we can detect them even if the user stops // propagation. - // On keydown record the origin and cancel any touch event that may be in progress. + // On keydown record the origin and clear any touch event that may be in progress. document.addEventListener('keydown', () => { - this._touchActive = false; + this._lastTouchTarget = null; this._setOriginForCurrentEventQueue('keyboard'); }, true); - // On mousedown record the origin, but do not cancel the touch event. A mousedown can happen as - // a result of a touch event. - document.addEventListener('mousedown', - () => this._setOriginForCurrentEventQueue('mouse'), true); + // On mousedown record the origin only if there is not touch target, since a mousedown can + // happen as a result of a touch event. + document.addEventListener('mousedown', () => { + if (!this._lastTouchTarget) { + this._setOriginForCurrentEventQueue('mouse'); + } + }, true); // 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 see if // a focus happens. - document.addEventListener('touchstart', () => { - this._touchActive = true; - setTimeout(() => this._touchActive = false, TOUCH_BUFFER_MS); + document.addEventListener('touchstart', (event: Event) => { + if (this._touchTimeout != null) { + clearTimeout(this._touchTimeout); + } + this._lastTouchTarget = event.target; + this._touchTimeout = setTimeout(() => this._lastTouchTarget = null, TOUCH_BUFFER_MS); }, true); // Make a note of when the window regains focus, so we can restore the origin info for the @@ -88,17 +68,14 @@ export class FocusOriginMonitor { /** Register an element to receive focus classes. */ registerElementForFocusClasses(element: Element, renderer: Renderer): Observable { let subject = new Subject(); - renderer.listen(element, 'focus', () => this._onFocus(element, renderer, subject)); + renderer.listen(element, 'focus', + (event: Event) => this._onFocus(event, element, renderer, subject)); renderer.listen(element, 'blur', () => this._onBlur(element, renderer, subject)); return subject.asObservable(); } /** Focuses the element via the specified focus origin. */ focusVia(element: Node, renderer: Renderer, origin: FocusOrigin) { - // This method may have been called as a result of a touchstart event, so we need to clear - // _touchActive to prevent it from interfering. - this._touchActive = false; - this._setOriginForCurrentEventQueue(origin); renderer.invokeElementMethod(element, 'focus'); } @@ -109,20 +86,26 @@ export class FocusOriginMonitor { setTimeout(() => this._origin = null, 0); } - /** Handles focus events on a registered element. */ - private _onFocus(element: Element, renderer: Renderer, subject: Subject) { - if (this._touchActive) { - this._origin = 'touch'; - } + private _wasCausedByTouch(event: Event): boolean { + let focusTarget = event.target; + return this._lastTouchTarget instanceof Node && focusTarget instanceof Node && + (focusTarget == this._lastTouchTarget || focusTarget.contains(this._lastTouchTarget)); + } + /** Handles focus events on a registered element. */ + private _onFocus(event: Event, element: Element, renderer: Renderer, + subject: Subject) { // If we couldn't detect a cause for the focus event, it's due to one of two reasons: // 1) The window has just regained focus, in which case we want to restore the focused state of // the element from before the window blurred. - // 2) The element was programmatically focused, in which case we should mark the origin as + // 2) It was caused by a touch event, in which case we mark the origin as 'touch'. + // 3) The element was programmatically focused, in which case we should mark the origin as // 'program'. if (!this._origin) { if (this._windowFocused && this._lastFocusOrigin) { this._origin = this._lastFocusOrigin; + } else if (this._wasCausedByTouch(event)) { + this._origin = 'touch'; } else { this._origin = 'program'; } From d2a3d5e8bf990f35115a26add4bb3b32337f76db Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Mon, 13 Feb 2017 12:40:20 -0800 Subject: [PATCH 6/7] Add comment explaining drawbacks of how check if the focus was caused by touch. --- src/lib/core/style/focus-classes.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/lib/core/style/focus-classes.ts b/src/lib/core/style/focus-classes.ts index 1c6419aa34c2..0c0d1b9bf56d 100644 --- a/src/lib/core/style/focus-classes.ts +++ b/src/lib/core/style/focus-classes.ts @@ -26,6 +26,7 @@ export class FocusOriginMonitor { /** The target of the last touch event. */ private _lastTouchTarget: EventTarget; + /** The timeout id of the touch timeout, used to cancel timeout later. */ private _touchTimeout: number; constructor() { @@ -87,6 +88,23 @@ export class FocusOriginMonitor { } private _wasCausedByTouch(event: Event): boolean { + // Note(mmalerba): This implementation is not quite perfect, there is a small edge case. + // Consider the following dom structure: + // + //
+ //
+ //
+ // + // If the user touches the #child element and the #parent is programmatically focused as a + // result, this code will still consider it to have been caused by the touch event and will + // apply the cdk-touch-focused class rather than the cdk-program-focused class. This is a + // relatively small edge-case that can be worked around by using + // focusVia(parentEl, renderer, 'program') to focus the parent element. + // + // If we decide that we absolutely must handle this case correctly, we can do so by listening + // for the first focus event after the touchstart, and then the first blur event after that + // focus event. When that blur event fires we know that whatever follows is not a result of the + // touchstart. let focusTarget = event.target; return this._lastTouchTarget instanceof Node && focusTarget instanceof Node && (focusTarget == this._lastTouchTarget || focusTarget.contains(this._lastTouchTarget)); From ed8d31d3a5db61acf0e77d80397032f041b05372 Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Mon, 13 Feb 2017 12:41:43 -0800 Subject: [PATCH 7/7] add doc comment --- src/lib/core/style/focus-classes.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/lib/core/style/focus-classes.ts b/src/lib/core/style/focus-classes.ts index 0c0d1b9bf56d..9eefdf85dbc9 100644 --- a/src/lib/core/style/focus-classes.ts +++ b/src/lib/core/style/focus-classes.ts @@ -87,6 +87,7 @@ export class FocusOriginMonitor { setTimeout(() => this._origin = null, 0); } + /** Checks whether the given focus event was caused by a touchstart event. */ private _wasCausedByTouch(event: Event): boolean { // Note(mmalerba): This implementation is not quite perfect, there is a small edge case. // Consider the following dom structure: