feat(sidenav): close via escape key and restore focus to trigger element #1990
Conversation
crisbeto
commented
Nov 26, 2016
crisbeto
commented
- Adds the ability to close a sidenav by pressing escape.
- Restores focus to the trigger element after a sidenav is closed.
googlebot
added
the
cla: yes
|
R: @mmalerba |
| @@ -38,6 +38,7 @@ export class MdDuplicatedSidenavError extends MdError { | |||
| template: '<ng-content></ng-content>', | |||
| host: { | |||
| '(transitionend)': '_onTransitionEnd($event)', | |||
| '(keydown.escape)': 'handleEscapeKey()', | |||
There was a problem hiding this comment.
unfortunately we can't use this fancy syntax, since not all of the browsers we target support the key property of KeyboardEvent. We have a file of keycodes here: https://github.com/angular/material2/blob/master/src/lib/core/keyboard/keycodes.ts
There was a problem hiding this comment.
Good to know, I saw that the dialog was using it so I went with it. I'll switch it back to use the keyCode.
Regarding the reasoning in the keycodes.ts, I don't know if this is still an issue since there's an e2e test on the dialog that uses the escape key.
| constructor(private _elementRef: ElementRef) {} | ||
| constructor(private _elementRef: ElementRef) { | ||
| this.onOpen.subscribe(() => { | ||
| this._elementFocusedBeforeSidenavWasOpened = document.activeElement as HTMLElement; |
There was a problem hiding this comment.
document.activeElement
what about universal?
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
crisbeto
force-pushed
the
sidenav-escape-to-close
branch
from
630b7fa
to
2d47439
Compare
|
Addressed the feedback and rebased @mmalerba. |
| */ | ||
| handleKeydown(event: KeyboardEvent) { | ||
| if (event.keyCode === ESCAPE) { | ||
| this.close(); |
There was a problem hiding this comment.
probably want to stop propagation (if we have a dialog with a sidenav, esc should close the sidenav but not the dialog)
crisbeto
force-pushed
the
sidenav-escape-to-close
branch
2 times, most recently
from
b1d8b16
to
8b73513
Compare
|
It seems the tests are failing after the sidenav focus trap stuff in #1695 was merged |
|
Weird that it's only in Mobile Safari. I'll re-run the build and take a look. |
* Adds the ability to close a sidenav by pressing escape. * Restores focus to the trigger element after a sidenav is closed.
crisbeto
force-pushed
the
sidenav-escape-to-close
branch
from
8b73513
to
139801e
Compare
|
I sorted it out @mmalerba. It was conflicting with the focus trapping. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
