feat(overlay): add keyboard dispatcher for targeting correct overlay #6682

willshowell · 2017-08-28T22:33:01Z

jelbourn

Mostly looks good to me- @crisbeto any thoughts?

jelbourn · 2017-08-30T22:42:28Z

src/cdk/overlay/overlay-ref.ts

@@ -140,6 +145,16 @@ export class OverlayRef implements PortalHost {
    return this._detachments.asObservable();
  }

+  /** Returns an observable that emits when the overlay has been disposed. */


nit: prefer "Gets" to "Returns"

jelbourn · 2017-08-30T22:44:00Z

src/cdk/overlay/overlay.ts

+   * Subscribe to attach and detach events and register them accordingly
+   * with the keyboard dispatcher service.
+   */
+  private _trackOverlayAttachments(overlayRef: OverlayRef): void {


Could each OverlayRef to this itself in its constructor?

willshowell · 2017-08-31T01:28:40Z

@jelbourn comments addressed. Also added a demo to the overlay.

crisbeto · 2017-08-31T05:05:26Z

src/lib/dialog/dialog.ts

+    RxChain.from(overlayRef.keyboardEvents())
+      .call(filter, event => event.type === 'keydown' && event.keyCode === ESCAPE)
+      .subscribe(() => {
+        if (!dialogRef.disableClose) {


Might be a bit more concise to have this in filter, instead of checking it in the subscribe.

crisbeto · 2017-08-31T05:07:43Z

src/cdk/overlay/keyboard/overlay-keyboard-dispatcher.ts

+    const bodyKeyboardEvents = merge(
+        fromEvent(document.body, 'keydown'),
+        fromEvent(document.body, 'keypress'),
+        fromEvent(document.body, 'keyup')) as Observable<KeyboardEvent>;


Instead of casting it to an Observable<KeyboardEvent>, you can pass the KeyboardEvent as a generic: merge<KeyboardEvent>.

crisbeto · 2017-08-31T05:09:52Z

src/cdk/overlay/keyboard/overlay-keyboard-dispatcher.ts

+  private _dispatchKeyboardEvents(): void {
+    const bodyKeyboardEvents = merge(
+        fromEvent(document.body, 'keydown'),
+        fromEvent(document.body, 'keypress'),


I'm not sure that this should handle keydown, keypress and keyup @jelbourn. Since for 99% of cases you'd only need keydown, it means that every time you consume the service, you'd have to have a filter(event => event.type === 'keydown').

I'd be happy with this. I was aiming for generic, but the api could be "rebranded" 'to just keydownEvents.

crisbeto · 2017-08-31T05:12:02Z

src/cdk/overlay/keyboard/overlay-keyboard-dispatcher.ts

+  /** Select the appropriate overlay from a list of overlays and a keyboard event. */
+  private _selectOverlayFromEvent(overlays: OverlayRef[], event: KeyboardEvent): OverlayRef {
+    // Check if any overlays contain the event
+    const targetedOverlay = this._attachedOverlays.find(overlay =>


This doesn't handle if the keyboard event came directly from the overlay element. You can tweak the predicate to include overlay.overlayElement === event.target.

crisbeto · 2017-08-31T05:12:48Z

src/cdk/overlay/keyboard/overlay-keyboard-dispatcher.ts

+  }
+
+  /** Select the appropriate overlay from a list of overlays and a keyboard event. */
+  private _selectOverlayFromEvent(overlays: OverlayRef[], event: KeyboardEvent): OverlayRef {


Since the overlays is always going to be this._attachedOverlays, you can take it from the class property instead of passing it in.

crisbeto · 2017-08-31T05:21:07Z

src/cdk/overlay/overlay-ref.ts

@@ -22,12 +23,16 @@ export class OverlayRef implements PortalHost {
  private _backdropClick: Subject<any> = new Subject();
  private _attachments = new Subject<void>();
  private _detachments = new Subject<void>();
+  private _disposed = new Subject<void>();
+
+  _dispatchedKeyboardEvents = new Subject<KeyboardEvent>();


Instead of having the OverlayRef add and remove itself from the dispatcher, a cleaner approach would be to only have it expose the stream of keyboard events. Then the OverlayRef.keyboardEvents can be simplified to:

// note: don't get too caught up in the naming return this.keyboardDispatcher.events.filter(event => event.target === this.overlayElement);

This leaves less room for mistakes when managing the overlays and it also removes the circular dependency between the OverlayRef and the dispatcher.

Edit: While we're at it, this could avoid having to add a dispatcher in the first place. The OverlayRef can bind the keyboard listener on its own and only filter out relevant events.

Hm I don't entirely follow.

I kind of see how I could remove the Subject; maybe if I created a new event type that paired overlays to keydowns, and then the overlay just filters out ones directed at itself and maps to the keydown.

An important piece of this is dispatching an event to the most recently attached overlay. Often, events land on the body with no other context. That's the whole point of tracking overlays as they are attached and detached.

Alright, I see. I guess that in most cases the top-most overlay would be the one dispatching the keyboard events, but that won't always be the case (like with md-autocomplete). Let's keep it.

One of the original problems was that if you opened a dialog without focusable elements, focus would still be on trigger or wherever it was before, but I see that's since been fixed. This plunker from the original issue shows that clicking on the backdrop still moves focus to the body.

I'll go ahead with the other changes and we can make sure it all still makes sense.

willshowell · 2017-08-31T20:20:47Z

@crisbeto comments addressed. Please take another look!

crisbeto

Mostly LGTM, a few minor notes left.

crisbeto · 2017-08-31T20:30:14Z

src/cdk/overlay/keyboard/overlay-keyboard-dispatcher.ts

+  private _selectOverlayFromEvent(event: KeyboardEvent): OverlayRef {
+    // Check if any overlays contain the event
+    const targetedOverlay = this._attachedOverlays.find(overlay => {
+      return overlay.overlayElement.contains(event.target as HTMLElement) ||


I'd move the overlay.overlayElement === event.target check first since it could potentially exit sooner.

crisbeto · 2017-08-31T20:30:38Z

src/cdk/overlay/keyboard/overlay-keyboard-dispatcher.ts

+    });
+
+    // Use that overlay if it exists, otherwise choose the most recently attached one
+    return targetedOverlay ?


return targetedOverlay || this._attachedOverlays[this._attachedOverlays.length - 1].

crisbeto · 2017-08-31T20:31:00Z

src/cdk/overlay/overlay-ref.ts

@@ -23,11 +24,14 @@ export class OverlayRef implements PortalHost {
  private _attachments = new Subject<void>();
  private _detachments = new Subject<void>();

+  _dispatchedKeydownEvents = new Subject<KeyboardEvent>();


Add a description to this one. Also potentially rename it to something like _keydownEvents.

crisbeto

LGTM

crisbeto · 2017-08-31T21:13:23Z

@willshowell looks like the prerender task is still failing. You can debug it locally using gulp ci:prerender. Ping me to mark as merge ready.

willshowell · 2017-09-01T15:02:41Z

@crisbeto I had issues with fromEvent not working well with the DOCUMENT token, so I opted to make the dispatch start lazily. PTAL.

crisbeto

Sorry, but one final nitpick.

crisbeto · 2017-09-01T16:46:28Z

src/cdk/overlay/keyboard/overlay-keyboard-dispatcher.ts

+   * Subscribe to keydown events that land on the body and dispatch those
+   * events to the appropriate overlay.
+   */
+  private _dispatchKeydownEvents(): void {


Judging by the name of the method, I'd assume that it would dispatch the events immediately, but that's not what it does. Something along the lines of _subscribeToKeydownEvents might be more appropriate.

jelbourn

LGTM

willshowell · 2017-10-03T20:00:30Z

Rebased. cc @kara

willshowell · 2017-10-06T16:19:21Z

@kara is there anything I need to do regarding the presubmit failure?

andrewseguin · 2017-10-12T20:00:33Z

Getting some AOT issue:

Error: Error encountered resolving symbol values statically. Only initialized variables and constants can be referenced because the value of this variable is needed by the template compiler (position 10:22 in the original .ts file), resolving symbol DOCUMENT in /platform-browser/src/dom/dom_tokens.d.ts, resolving symbol DOCUMENT in /platform-browser/src/platform-browser.d.ts, resolving symbol DOCUMENT in /platform-browser/public_api.d.ts, resolving symbol DOCUMENT in /platform-browser/index.d.ts, resolving symbol DOCUMENT in platform_browser_safe/index.d.ts, resolving symbol OVERLAY_KEYBOARD_DISPATCHER_PROVIDER in /src/cdk/overlay/keyboard/overlay-keyboard-dispatcher.ts, resolving symbol OVERLAY_PROVIDERS in /src/cdk/overlay/overlay-module.ts, resolving symbol OverlayModule in /src/cdk/overlay/overlay-module.ts, resolving symbol OverlayModule in /src/cdk/overlay/overlay-module.ts

Can you try building in AOT?

willshowell · 2017-10-13T14:32:37Z

@andrewseguin I wasn't able to reproduce, but I've rebased and updated the license.

I'm passing on gulp ci:lint, aot, prerender (ci:test is broken from something else).

mmalerba · 2017-10-24T00:03:16Z

Is injecting DOCUMENT instead of using document critical here? Removing the DOCUMENT stuff fixes the error @andrewseguin mentioned

jelbourn · 2017-10-24T00:08:26Z

@mmalerba we should only need to use DOCUMENT if we're mocking something in a test

mmalerba · 2017-10-24T00:16:13Z

I think DOCUMENT is special somehow, I tried creating a different injection token, OVERLAY_DOCUMENT, and everything worked fine. So we should switch it and if we need it for testing just create our own provider. @willshowell Once that's done I can try again on presubmitting.

jelbourn · 2017-10-24T00:24:41Z

Oh yeah, now I remember. I had to do the same thing for the tests for Directionality. BidiModule has

{provide: DIR_DOCUMENT, useExisting: DOCUMENT},

willshowell · 2017-10-24T13:32:25Z

Doesn't seem there's any need to mock document for the tests, so I've removed the token.

jelbourn · 2017-10-25T19:23:42Z

@mmalerba should be ready for another spin

After the switch to `ActiveDescendantKeyManager`, the select was no longer handling the escape key functionality internally, but was delegating to the underlying overlay. Since the overlay listens to keyboard events on the document, it means that escape key presses will close any parent overlays along the way. These changes add the escape listener to the select itself and stop the event propagation. **Note:** the overlay directive listener should be scoped to the overlay itself, but it's better if we defer refactoring it until angular#6682 gets in. Fixes angular#7981.

Fix typo and add comment Address comments Add keyboard tracking demo to overlay Address comments Revert no-longer-needed dispose changes Address comments Fix prerender error by lazily starting dispatcher Address naming comment

angular-automatic-lock-bot · 2019-09-07T11:03:40Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

willshowell requested review from crisbeto and jelbourn as code owners August 28, 2017 22:33

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 28, 2017

jelbourn reviewed Aug 30, 2017

View reviewed changes

crisbeto reviewed Aug 31, 2017

View reviewed changes

willshowell force-pushed the feat/key-dispatcher branch from bdf8ca7 to e074fa0 Compare August 31, 2017 20:14

crisbeto reviewed Aug 31, 2017

View reviewed changes

crisbeto approved these changes Aug 31, 2017

View reviewed changes

crisbeto added the pr: lgtm label Aug 31, 2017

crisbeto reviewed Sep 1, 2017

View reviewed changes

jelbourn approved these changes Sep 5, 2017

View reviewed changes

jelbourn added the action: merge The PR is ready for merge by the caretaker label Sep 5, 2017

This was referenced Sep 6, 2017

docs(drawer): Add additional documentation for disableClose #6750

Merged

md-dialog Before close functionality? #6369

Closed

willshowell force-pushed the feat/key-dispatcher branch 2 times, most recently from b1e56ac to e90c917 Compare September 25, 2017 14:42

willshowell mentioned this pull request Sep 27, 2017

[question] cdk keycodes #7358

Closed

andrewseguin added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Sep 29, 2017

willshowell force-pushed the feat/key-dispatcher branch 3 times, most recently from 241cf71 to 95f9016 Compare October 3, 2017 17:10

crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Oct 3, 2017

kara added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Oct 4, 2017

willshowell force-pushed the feat/key-dispatcher branch from 95f9016 to a3d3536 Compare October 10, 2017 19:23

willshowell force-pushed the feat/key-dispatcher branch from a3d3536 to 2034dfc Compare October 13, 2017 14:27

willshowell mentioned this pull request Oct 18, 2017

No way to intercept MdDialog close events #3893

Closed

jelbourn added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Oct 21, 2017

willshowell force-pushed the feat/key-dispatcher branch from 2034dfc to bbbf598 Compare October 24, 2017 13:29

willshowell requested a review from devversion as a code owner October 24, 2017 13:29

jelbourn removed the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Oct 25, 2017

crisbeto mentioned this pull request Oct 26, 2017

fix(select): closing parent overlay when pressing escape #8060

Closed

willshowell force-pushed the feat/key-dispatcher branch from bbbf598 to 43c8490 Compare October 27, 2017 17:15

mmalerba merged commit a2ca4d6 into angular:master Oct 27, 2017

willshowell deleted the feat/key-dispatcher branch October 27, 2017 17:17

willshowell added 3 commits October 27, 2017 13:18

Update license

258dad1

Remove DOCUMENT token

43c8490

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019

feat(overlay): add keyboard dispatcher for targeting correct overlay #6682

feat(overlay): add keyboard dispatcher for targeting correct overlay #6682

Conversation

willshowell commented Aug 28, 2017

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willshowell commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willshowell commented Aug 31, 2017

crisbeto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto left a comment

Choose a reason for hiding this comment

crisbeto commented Aug 31, 2017

willshowell commented Sep 1, 2017

crisbeto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

willshowell commented Oct 3, 2017

willshowell commented Oct 6, 2017

andrewseguin commented Oct 12, 2017 • edited Loading

willshowell commented Oct 13, 2017

mmalerba commented Oct 24, 2017

jelbourn commented Oct 24, 2017

mmalerba commented Oct 24, 2017

jelbourn commented Oct 24, 2017

willshowell commented Oct 24, 2017

jelbourn commented Oct 25, 2017

angular-automatic-lock-bot bot commented Sep 7, 2019

crisbeto Aug 31, 2017 •

edited

Loading

andrewseguin commented Oct 12, 2017 •

edited

Loading