Skip to content
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

feat(sidenav): close via escape key and restore focus to trigger element #1990

Merged
merged 11 commits into from
Dec 9, 2016
2 changes: 2 additions & 0 deletions src/lib/core/keyboard/keycodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,5 @@ export const END = 35;
export const ENTER = 13;
export const SPACE = 32;
export const TAB = 9;

export const ESCAPE = 27;
1 change: 1 addition & 0 deletions src/lib/sidenav/sidenav.scss
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
54 changes: 54 additions & 0 deletions src/lib/sidenav/sidenav.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<any>) {
Expand Down Expand Up @@ -235,6 +236,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.handleKeydown({
keyCode: ESCAPE,
stopPropagation: () => {}
} as KeyboardEvent);

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();

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', () => {
Expand Down
49 changes: 43 additions & 6 deletions src/lib/sidenav/sidenav.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand All @@ -40,6 +49,7 @@ export class MdSidenavToggleResult {
template: '<focus-trap [disabled]="isFocusTrapDisabled"><ng-content></ng-content></focus-trap>',
host: {
'(transitionend)': '_onTransitionEnd($event)',
'(keydown)': 'handleKeydown($event)',
// must prevent the browser from aligning text based on value
'[attr.align]': 'null',
'[class.md-sidenav-closed]': '_isClosed',
Expand All @@ -51,6 +61,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,
Expand Down Expand Up @@ -128,7 +139,25 @@ 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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

document.activeElement

what about universal?

Copy link
Member Author

@crisbeto crisbeto Dec 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think Angular has a way of getting this, at the moment. There are other components that use document.activeElement as well.

AFAIK when we start supporting Universal, we can check if the renderer is a browser with this and take the appropriate action:

import { isBrowser, isNode } from 'angular2-universal'

Copy link

@DzmitryShylovich DzmitryShylovich Dec 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for things such as

this._elementRef.nativeElement.focus();

you can use renderer

this.renderer.invokeElementMethod(this._elementRef.nativeElement, 'focus');

Core already have different renderer implementations for browser and server envs.

But Renderer doesn't have methods to retrieve dom elements and afaik the recommended approach is to use DomAdapter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm good point about the renderer methods, I was under the impression that I used it already, but I probably forgot to switch it over after I was done testing. I'll move those. I'll have to look into using the adapter for querying.


if (!this.isFocusTrapDisabled) {
this._focusTrap.focusFirstTabbableElementWhenReady();
}
});

this.onClose.subscribe(() => {
if (this._elementFocusedBeforeSidenavWasOpened instanceof HTMLElement) {
this._renderer.invokeElementMethod(this._elementFocusedBeforeSidenavWasOpened, 'focus');
} else {
this._renderer.invokeElementMethod(this._elementRef.nativeElement, 'blur');
}

this._elementFocusedBeforeSidenavWasOpened = null;
});
}

ngAfterContentInit() {
// This can happen when the sidenav is set to opened in the template and the transition
Expand Down Expand Up @@ -188,10 +217,6 @@ export class MdSidenav implements AfterContentInit {
this.onCloseStart.emit();
}

if (!this.isFocusTrapDisabled) {
this._focusTrap.focusFirstTabbableElementWhenReady();
}

if (this._toggleAnimationPromise) {
this._resolveToggleAnimationPromise(false);
}
Expand All @@ -202,6 +227,16 @@ export class MdSidenav implements AfterContentInit {
return this._toggleAnimationPromise;
}

/**
* Handles the keyboard events.
*/
handleKeydown(event: KeyboardEvent) {
if (event.keyCode === ESCAPE) {
this.close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably want to stop propagation (if we have a dialog with a sidenav, esc should close the sidenav but not the dialog)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

event.stopPropagation();
}
}

/**
* 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
Expand Down Expand Up @@ -255,6 +290,8 @@ export class MdSidenav implements AfterContentInit {
}
return 0;
}

private _elementFocusedBeforeSidenavWasOpened: HTMLElement = null;
}

/**
Expand Down