fix(drawer): drawer running inside the zone keydown event. #10092
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
googlebot
added
the
cla: yes
|
Interesting, had not considered this, good catch! |
mmalerba
reviewed
Feb 22, 2018
src/lib/sidenav/drawer.ts
Outdated
| @@ -273,6 +274,16 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr | |||
| this._restoreFocus(); | |||
| } | |||
| }); | |||
|
|
|||
| // Handles the ESCAPE keyboard event | |||
There was a problem hiding this comment.
Can you expand this comment a little bit to explain why we're doing it this way as opposed to just (keydown)
llorenspujol
force-pushed
the
fix-drawer-keydown-inside-zone
branch
from
c1669ff to
b036141
Compare
llorenspujol
changed the title
|
Thanks! I have updated the comment @mmalerba , what do you think? |
llorenspujol
force-pushed
the
fix-drawer-keydown-inside-zone
branch
from
b036141 to
a0a54aa
Compare
mmalerba
reviewed
Feb 23, 2018
src/lib/sidenav/drawer.ts
Outdated
| @@ -273,6 +274,20 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr | |||
| this._restoreFocus(); | |||
| } | |||
| }); | |||
|
|
|||
| /** | |||
| * Runs outside the zone the 'keydown' event listener. This avoids change detector to be fired | |||
There was a problem hiding this comment.
How about:
/**
* Listen to `keydown` events outside the zone so that change detection is not run every
* time a key is pressed. Instead we re-enter the zone only if the `ESC` key is pressed
* and we don't have close disabled.
*/
Currently the drawer is listening to the event 'keydown' inside the zone. This makes the change detector be fired every time while whe are pressing 'any' key.
llorenspujol
force-pushed
the
fix-drawer-keydown-inside-zone
branch
from
a0a54aa to
c08f00a
Compare
mmalerba
approved these changes
Feb 26, 2018
|
Thanks for the fix! |
|
@mmalerba this one's missing a release target. |
|
marked it |
tinayuangao
pushed a commit
that referenced
this pull request
Mar 5, 2018
Currently the drawer is listening to the event 'keydown' inside the zone. This makes the change detector be fired every time while whe are pressing 'any' key.
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the drawer is listening to the event 'keydown' inside the zone. This makes the change detector be fired every time while we are pressing 'any' key, even it's is not the ESCAPE key.
With this code, we can escape it, and run only inside the zone when there is an escape event.
Do we really need this?
I'm sure most apps would not need that, but for example, in our app, we have a drawer that inside has a bunch of controls that are controlled by keyboard events... so this has an impact to our performance since change detector is fired while pressing. So I think it would be a good change, although it's is not much elegant, but it is functional.