From e455bdde68b8886f62732e9e4bd5f5e4079ddd9f Mon Sep 17 00:00:00 2001 From: crisbeto Date: Sat, 26 Nov 2016 12:40:57 +0100 Subject: [PATCH 01/11] feat(sidenav): close via escape key and restore focus to trigger element * Adds the ability to close a sidenav by pressing escape. * Restores focus to the trigger element after a sidenav is closed. --- src/lib/sidenav/sidenav.scss | 1 + src/lib/sidenav/sidenav.spec.ts | 53 +++++++++++++++++++++++++++++++++ src/lib/sidenav/sidenav.ts | 32 +++++++++++++++++++- 3 files changed, 85 insertions(+), 1 deletion(-) diff --git a/src/lib/sidenav/sidenav.scss b/src/lib/sidenav/sidenav.scss index 06923ff59a08..6ec1eec18a56 100644 --- a/src/lib/sidenav/sidenav.scss +++ b/src/lib/sidenav/sidenav.scss @@ -107,6 +107,7 @@ md-sidenav { bottom: 0; z-index: 3; min-width: 5%; + outline: 0; // TODO(kara): revisit scrolling behavior for sidenavs overflow-y: auto; diff --git a/src/lib/sidenav/sidenav.spec.ts b/src/lib/sidenav/sidenav.spec.ts index ad24acffd9ee..ba2b25306562 100644 --- a/src/lib/sidenav/sidenav.spec.ts +++ b/src/lib/sidenav/sidenav.spec.ts @@ -235,6 +235,59 @@ describe('MdSidenav', () => { expect(testComponent.backdropClickedCount).toBe(1); })); + it('should close when pressing escape', fakeAsync(() => { + let fixture = TestBed.createComponent(BasicTestApp); + let testComponent: BasicTestApp = fixture.debugElement.componentInstance; + let sidenav: MdSidenav = fixture.debugElement + .query(By.directive(MdSidenav)).componentInstance; + + sidenav.open(); + + fixture.detectChanges(); + endSidenavTransition(fixture); + tick(); + + expect(testComponent.openCount).toBe(1); + expect(testComponent.closeCount).toBe(0); + + // Simulate pressing the escape key. + sidenav.handleEscapeKey(); + + fixture.detectChanges(); + endSidenavTransition(fixture); + tick(); + + expect(testComponent.closeCount).toBe(1); + })); + + it('should restore focus to the trigger element on close', fakeAsync(() => { + let fixture = TestBed.createComponent(BasicTestApp); + let sidenav: MdSidenav = fixture.debugElement + .query(By.directive(MdSidenav)).componentInstance; + let trigger = document.createElement('button'); + + document.body.appendChild(trigger); + trigger.focus(); + sidenav.open(); + + fixture.detectChanges(); + endSidenavTransition(fixture); + tick(); + + expect(document.activeElement) + .not.toBe(trigger, 'Expected focus to change when the sidenav was opened.'); + + sidenav.close(); + + fixture.detectChanges(); + endSidenavTransition(fixture); + tick(); + + expect(document.activeElement) + .toBe(trigger, 'Expected focus to be restored to the trigger on close.'); + + trigger.parentNode.removeChild(trigger); + })); }); describe('attributes', () => { diff --git a/src/lib/sidenav/sidenav.ts b/src/lib/sidenav/sidenav.ts index e6cc23d6e638..de3139adb7fa 100644 --- a/src/lib/sidenav/sidenav.ts +++ b/src/lib/sidenav/sidenav.ts @@ -40,6 +40,7 @@ export class MdSidenavToggleResult { template: '', host: { '(transitionend)': '_onTransitionEnd($event)', + '(keydown.escape)': 'handleEscapeKey()', // must prevent the browser from aligning text based on value '[attr.align]': 'null', '[class.md-sidenav-closed]': '_isClosed', @@ -51,6 +52,7 @@ export class MdSidenavToggleResult { '[class.md-sidenav-push]': '_modePush', '[class.md-sidenav-side]': '_modeSide', '[class.md-sidenav-invalid]': '!valid', + 'tabIndex': '-1' }, changeDetection: ChangeDetectionStrategy.OnPush, encapsulation: ViewEncapsulation.None, @@ -128,7 +130,18 @@ export class MdSidenav implements AfterContentInit { * @param _elementRef The DOM element reference. Used for transition and width calculation. * If not available we do not hook on transitions. */ - constructor(private _elementRef: ElementRef) {} + constructor(private _elementRef: ElementRef) { + this.onOpen.subscribe(() => { + this._elementFocusedBeforeSidenavWasOpened = document.activeElement as HTMLElement; + this._elementRef.nativeElement.focus(); + }); + + this.onClose.subscribe(() => { + if (this._elementFocusedBeforeSidenavWasOpened) { + this._elementFocusedBeforeSidenavWasOpened.focus(); + } + }); + } ngAfterContentInit() { // This can happen when the sidenav is set to opened in the template and the transition @@ -202,6 +215,13 @@ export class MdSidenav implements AfterContentInit { return this._toggleAnimationPromise; } + /** Handles the user pressing the escape key. */ + handleEscapeKey() { + // TODO(crisbeto): this is in a separate method in order to + // allow for disabling the behavior later. + this.close(); + } + /** * When transition has finished, set the internal state for classes and emit the proper event. * The event passed is actually of type TransitionEvent, but that type is not available in @@ -255,6 +275,16 @@ export class MdSidenav implements AfterContentInit { } return 0; } + + + private _transition: boolean = false; + private _openPromise: Promise; + private _openPromiseResolve: () => void; + private _openPromiseReject: () => void; + private _closePromise: Promise; + private _closePromiseResolve: () => void; + private _closePromiseReject: () => void; + private _elementFocusedBeforeSidenavWasOpened: HTMLElement = null; } /** From 404721d810ae6ac39107c4a920a4411c82a4cacb Mon Sep 17 00:00:00 2001 From: crisbeto Date: Sat, 26 Nov 2016 12:58:26 +0100 Subject: [PATCH 02/11] fix: test failures in IE and blur element if there's no focusable trigger --- src/lib/sidenav/sidenav.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/lib/sidenav/sidenav.ts b/src/lib/sidenav/sidenav.ts index de3139adb7fa..b108e2352cfb 100644 --- a/src/lib/sidenav/sidenav.ts +++ b/src/lib/sidenav/sidenav.ts @@ -137,9 +137,13 @@ export class MdSidenav implements AfterContentInit { }); this.onClose.subscribe(() => { - if (this._elementFocusedBeforeSidenavWasOpened) { + if (this._elementFocusedBeforeSidenavWasOpened instanceof HTMLElement) { this._elementFocusedBeforeSidenavWasOpened.focus(); + } else { + this._elementRef.nativeElement.blur(); } + + this._elementFocusedBeforeSidenavWasOpened = null; }); } From f5baaa2234b7244f4af17444623fcd34d5ceffbf Mon Sep 17 00:00:00 2001 From: crisbeto Date: Fri, 2 Dec 2016 18:07:12 +0100 Subject: [PATCH 03/11] fix: use the keycode instead of (keydown.escape) --- src/lib/core/keyboard/keycodes.ts | 2 ++ src/lib/sidenav/sidenav.spec.ts | 3 ++- src/lib/sidenav/sidenav.ts | 25 ++++++++++++++++++------- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/lib/core/keyboard/keycodes.ts b/src/lib/core/keyboard/keycodes.ts index 6204987a0e8a..165305e2d0c7 100644 --- a/src/lib/core/keyboard/keycodes.ts +++ b/src/lib/core/keyboard/keycodes.ts @@ -18,3 +18,5 @@ export const END = 35; export const ENTER = 13; export const SPACE = 32; export const TAB = 9; + +export const ESCAPE = 27 diff --git a/src/lib/sidenav/sidenav.spec.ts b/src/lib/sidenav/sidenav.spec.ts index ba2b25306562..462721ba2aaa 100644 --- a/src/lib/sidenav/sidenav.spec.ts +++ b/src/lib/sidenav/sidenav.spec.ts @@ -4,6 +4,7 @@ import {By} from '@angular/platform-browser'; import {MdSidenav, MdSidenavModule, MdSidenavToggleResult} from './sidenav'; import {A11yModule} from '../core/a11y/index'; import {PlatformModule} from '../core/platform/platform'; +import {ESCAPE} from '../core/keyboard/keycodes'; function endSidenavTransition(fixture: ComponentFixture) { @@ -251,7 +252,7 @@ describe('MdSidenav', () => { expect(testComponent.closeCount).toBe(0); // Simulate pressing the escape key. - sidenav.handleEscapeKey(); + sidenav.handleyKeydown({ keyCode: ESCAPE } as KeyboardEvent); fixture.detectChanges(); endSidenavTransition(fixture); diff --git a/src/lib/sidenav/sidenav.ts b/src/lib/sidenav/sidenav.ts index b108e2352cfb..7ea45915da85 100644 --- a/src/lib/sidenav/sidenav.ts +++ b/src/lib/sidenav/sidenav.ts @@ -16,9 +16,18 @@ import { ViewChild } from '@angular/core'; import {CommonModule} from '@angular/common'; -import {Dir, coerceBooleanProperty, DefaultStyleCompatibilityModeModule} from '../core'; +import {Dir, MdError, coerceBooleanProperty, DefaultStyleCompatibilityModeModule} from '../core'; import {A11yModule, A11Y_PROVIDERS} from '../core/a11y/index'; import {FocusTrap} from '../core/a11y/focus-trap'; +import {ESCAPE} from '../core/keyboard/keycodes'; + + +/** Exception thrown when two MdSidenav are matching the same side. */ +export class MdDuplicatedSidenavError extends MdError { + constructor(align: string) { + super(`A sidenav was already declared for 'align="${align}"'`); + } +} /** Sidenav toggle promise result. */ @@ -40,7 +49,7 @@ export class MdSidenavToggleResult { template: '', host: { '(transitionend)': '_onTransitionEnd($event)', - '(keydown.escape)': 'handleEscapeKey()', + '(keydown)': 'handleKeydown($event)', // must prevent the browser from aligning text based on value '[attr.align]': 'null', '[class.md-sidenav-closed]': '_isClosed', @@ -219,11 +228,13 @@ export class MdSidenav implements AfterContentInit { return this._toggleAnimationPromise; } - /** Handles the user pressing the escape key. */ - handleEscapeKey() { - // TODO(crisbeto): this is in a separate method in order to - // allow for disabling the behavior later. - this.close(); + /** + * Handles the keyboard events. + */ + handleyKeydown(event: KeyboardEvent) { + if (event.keyCode === ESCAPE) { + this.close(); + } } /** From 860388972e3134cea172b834129f32a04fc708b1 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Fri, 2 Dec 2016 18:12:18 +0100 Subject: [PATCH 04/11] fix: use the renderer for focusing and blurring and fix a typo --- src/lib/sidenav/sidenav.spec.ts | 2 +- src/lib/sidenav/sidenav.ts | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/lib/sidenav/sidenav.spec.ts b/src/lib/sidenav/sidenav.spec.ts index 462721ba2aaa..a187c1ee92c8 100644 --- a/src/lib/sidenav/sidenav.spec.ts +++ b/src/lib/sidenav/sidenav.spec.ts @@ -252,7 +252,7 @@ describe('MdSidenav', () => { expect(testComponent.closeCount).toBe(0); // Simulate pressing the escape key. - sidenav.handleyKeydown({ keyCode: ESCAPE } as KeyboardEvent); + sidenav.handleKeydown({ keyCode: ESCAPE } as KeyboardEvent); fixture.detectChanges(); endSidenavTransition(fixture); diff --git a/src/lib/sidenav/sidenav.ts b/src/lib/sidenav/sidenav.ts index 7ea45915da85..1a411739aed7 100644 --- a/src/lib/sidenav/sidenav.ts +++ b/src/lib/sidenav/sidenav.ts @@ -139,17 +139,17 @@ export class MdSidenav implements AfterContentInit { * @param _elementRef The DOM element reference. Used for transition and width calculation. * If not available we do not hook on transitions. */ - constructor(private _elementRef: ElementRef) { + constructor(private _elementRef: ElementRef, private _renderer: Renderer) { this.onOpen.subscribe(() => { this._elementFocusedBeforeSidenavWasOpened = document.activeElement as HTMLElement; - this._elementRef.nativeElement.focus(); + this._renderer.invokeElementMethod(this._elementRef.nativeElement, 'focus'); }); this.onClose.subscribe(() => { if (this._elementFocusedBeforeSidenavWasOpened instanceof HTMLElement) { this._elementFocusedBeforeSidenavWasOpened.focus(); } else { - this._elementRef.nativeElement.blur(); + this._renderer.invokeElementMethod(this._elementRef.nativeElement, 'blur'); } this._elementFocusedBeforeSidenavWasOpened = null; @@ -231,7 +231,7 @@ export class MdSidenav implements AfterContentInit { /** * Handles the keyboard events. */ - handleyKeydown(event: KeyboardEvent) { + handleKeydown(event: KeyboardEvent) { if (event.keyCode === ESCAPE) { this.close(); } From 77520d8f7a3831e9f9e773eca1f19fdf93e0bdf7 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Fri, 2 Dec 2016 18:14:11 +0100 Subject: [PATCH 05/11] fix a faulty merge --- src/lib/sidenav/sidenav.ts | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/lib/sidenav/sidenav.ts b/src/lib/sidenav/sidenav.ts index 1a411739aed7..ccaa8c970ce4 100644 --- a/src/lib/sidenav/sidenav.ts +++ b/src/lib/sidenav/sidenav.ts @@ -291,14 +291,6 @@ export class MdSidenav implements AfterContentInit { return 0; } - - private _transition: boolean = false; - private _openPromise: Promise; - private _openPromiseResolve: () => void; - private _openPromiseReject: () => void; - private _closePromise: Promise; - private _closePromiseResolve: () => void; - private _closePromiseReject: () => void; private _elementFocusedBeforeSidenavWasOpened: HTMLElement = null; } From 3b44ae182b57451062c3dafdc4a48ef1586aea55 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Fri, 2 Dec 2016 19:06:40 +0100 Subject: [PATCH 06/11] Fix a linter warning. --- src/lib/core/keyboard/keycodes.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/core/keyboard/keycodes.ts b/src/lib/core/keyboard/keycodes.ts index 165305e2d0c7..af4c4b702f9a 100644 --- a/src/lib/core/keyboard/keycodes.ts +++ b/src/lib/core/keyboard/keycodes.ts @@ -19,4 +19,4 @@ export const ENTER = 13; export const SPACE = 32; export const TAB = 9; -export const ESCAPE = 27 +export const ESCAPE = 27; From 6765217a59ccda0daed3da47af22fb9a5970915b Mon Sep 17 00:00:00 2001 From: crisbeto Date: Fri, 2 Dec 2016 20:35:40 +0100 Subject: [PATCH 07/11] Stop the propagation of the keydown event. --- src/lib/sidenav/sidenav.spec.ts | 5 ++++- src/lib/sidenav/sidenav.ts | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/lib/sidenav/sidenav.spec.ts b/src/lib/sidenav/sidenav.spec.ts index a187c1ee92c8..10ea32ee589d 100644 --- a/src/lib/sidenav/sidenav.spec.ts +++ b/src/lib/sidenav/sidenav.spec.ts @@ -252,7 +252,10 @@ describe('MdSidenav', () => { expect(testComponent.closeCount).toBe(0); // Simulate pressing the escape key. - sidenav.handleKeydown({ keyCode: ESCAPE } as KeyboardEvent); + sidenav.handleKeydown({ + keyCode: ESCAPE, + stopPropagation: () => {} + } as KeyboardEvent); fixture.detectChanges(); endSidenavTransition(fixture); diff --git a/src/lib/sidenav/sidenav.ts b/src/lib/sidenav/sidenav.ts index ccaa8c970ce4..3dd455672f45 100644 --- a/src/lib/sidenav/sidenav.ts +++ b/src/lib/sidenav/sidenav.ts @@ -234,6 +234,7 @@ export class MdSidenav implements AfterContentInit { handleKeydown(event: KeyboardEvent) { if (event.keyCode === ESCAPE) { this.close(); + event.stopPropagation(); } } From ca43aee67340ad48c8458ea9e5c37d5fb1b83a9a Mon Sep 17 00:00:00 2001 From: crisbeto Date: Tue, 6 Dec 2016 21:26:02 +0100 Subject: [PATCH 08/11] Pointless commit to resolve git issue. --- gulpfile.js | 1 + 1 file changed, 1 insertion(+) diff --git a/gulpfile.js b/gulpfile.js index 45e68bf5d1f1..78f78cc37f21 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -12,3 +12,4 @@ require('ts-node').register({ }); require('./tools/gulp/gulpfile'); +// From 139801e402e25272e08dedd4f076bf99110e7b2b Mon Sep 17 00:00:00 2001 From: crisbeto Date: Tue, 6 Dec 2016 21:26:18 +0100 Subject: [PATCH 09/11] Revert pointless commit. --- gulpfile.js | 1 - 1 file changed, 1 deletion(-) diff --git a/gulpfile.js b/gulpfile.js index 78f78cc37f21..45e68bf5d1f1 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -12,4 +12,3 @@ require('ts-node').register({ }); require('./tools/gulp/gulpfile'); -// From 9e53fca65738c5f1067819401d4bb7249974946a Mon Sep 17 00:00:00 2001 From: crisbeto Date: Thu, 8 Dec 2016 20:09:34 +0100 Subject: [PATCH 10/11] Fix conflict between the new functionality and the focus trapping. --- src/lib/sidenav/sidenav.spec.ts | 3 --- src/lib/sidenav/sidenav.ts | 8 ++------ 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/lib/sidenav/sidenav.spec.ts b/src/lib/sidenav/sidenav.spec.ts index 10ea32ee589d..5678797639b1 100644 --- a/src/lib/sidenav/sidenav.spec.ts +++ b/src/lib/sidenav/sidenav.spec.ts @@ -278,9 +278,6 @@ describe('MdSidenav', () => { endSidenavTransition(fixture); tick(); - expect(document.activeElement) - .not.toBe(trigger, 'Expected focus to change when the sidenav was opened.'); - sidenav.close(); fixture.detectChanges(); diff --git a/src/lib/sidenav/sidenav.ts b/src/lib/sidenav/sidenav.ts index 3dd455672f45..3cdad1be3233 100644 --- a/src/lib/sidenav/sidenav.ts +++ b/src/lib/sidenav/sidenav.ts @@ -140,14 +140,9 @@ export class MdSidenav implements AfterContentInit { * If not available we do not hook on transitions. */ constructor(private _elementRef: ElementRef, private _renderer: Renderer) { - this.onOpen.subscribe(() => { - this._elementFocusedBeforeSidenavWasOpened = document.activeElement as HTMLElement; - this._renderer.invokeElementMethod(this._elementRef.nativeElement, 'focus'); - }); - this.onClose.subscribe(() => { if (this._elementFocusedBeforeSidenavWasOpened instanceof HTMLElement) { - this._elementFocusedBeforeSidenavWasOpened.focus(); + this._renderer.invokeElementMethod(this._elementFocusedBeforeSidenavWasOpened, 'focus'); } else { this._renderer.invokeElementMethod(this._elementRef.nativeElement, 'blur'); } @@ -215,6 +210,7 @@ export class MdSidenav implements AfterContentInit { } if (!this.isFocusTrapDisabled) { + this._elementFocusedBeforeSidenavWasOpened = document.activeElement as HTMLElement; this._focusTrap.focusFirstTabbableElementWhenReady(); } From f498f42536dcd38d9e502157d35358517d5e1871 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Thu, 8 Dec 2016 20:41:59 +0100 Subject: [PATCH 11/11] Move the focus trapping behavior to the onOpen listener for improved reliability. --- src/lib/sidenav/sidenav.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/lib/sidenav/sidenav.ts b/src/lib/sidenav/sidenav.ts index 3cdad1be3233..a993045b6602 100644 --- a/src/lib/sidenav/sidenav.ts +++ b/src/lib/sidenav/sidenav.ts @@ -140,6 +140,14 @@ export class MdSidenav implements AfterContentInit { * If not available we do not hook on transitions. */ constructor(private _elementRef: ElementRef, private _renderer: Renderer) { + this.onOpen.subscribe(() => { + this._elementFocusedBeforeSidenavWasOpened = document.activeElement as HTMLElement; + + if (!this.isFocusTrapDisabled) { + this._focusTrap.focusFirstTabbableElementWhenReady(); + } + }); + this.onClose.subscribe(() => { if (this._elementFocusedBeforeSidenavWasOpened instanceof HTMLElement) { this._renderer.invokeElementMethod(this._elementFocusedBeforeSidenavWasOpened, 'focus'); @@ -209,11 +217,6 @@ export class MdSidenav implements AfterContentInit { this.onCloseStart.emit(); } - if (!this.isFocusTrapDisabled) { - this._elementFocusedBeforeSidenavWasOpened = document.activeElement as HTMLElement; - this._focusTrap.focusFirstTabbableElementWhenReady(); - } - if (this._toggleAnimationPromise) { this._resolveToggleAnimationPromise(false); }