fix(overlay): always dispatch keyboard events to top overlay in OverlayKeyboardDispatcher #10807
Conversation
googlebot
added
the
cla: yes
crisbeto
force-pushed
the
10799/top-level-overlay-dispatch
branch
from
61841ad to
d7b71fa
Compare
| @@ -85,8 +73,8 @@ export class OverlayKeyboardDispatcher implements OnDestroy { | |||
| /** Keyboard event listener that will be attached to the body. */ | |||
| private _keydownListener = (event: KeyboardEvent) => { | |||
| if (this._attachedOverlays.length) { | |||
| // Dispatch keydown event to the correct overlay. | |||
| this._selectOverlayFromEvent(event)._keydownEvents.next(event); | |||
| // Dispatch the keydown event to the top overlay. | |||
There was a problem hiding this comment.
Expand this with the rationale for why we always use the most recently opened overlay?
| @@ -86,28 +86,6 @@ describe('OverlayKeyboardDispatcher', () => { | |||
| button.parentNode!.removeChild(button); | |||
| }); | |||
|
|
|||
| it('should dispatch targeted keyboard events to the overlay containing that target', () => { | |||
There was a problem hiding this comment.
Should there be a test case with multiple open overlays?
There was a problem hiding this comment.
There's one called should dispatch body keyboard events to the most recently attached overlay a bit above.
crisbeto
force-pushed
the
10799/top-level-overlay-dispatch
branch
from
d7b71fa to
5a14063
Compare
|
Addressed the feedback @jelbourn. |
jelbourn
added
pr: lgtm
action: merge
|
I see that you just added the If you want your PR to be merged, it has to pass all the CI checks. If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help. |
|
Can you see whats up with CircleCI breakage? |
…ayKeyboardDispatcher Currently when dispatching events through the `OverlayKeyboardDispatcher` we try to match one of the overlays to the element that triggered the event. This is problematic, because some components will open an overlay, but will keep focus on the trigger element (e.g. `mat-autocomplete` and `mat-select`). These changes switch the logic so the keyboard events are always dispatched to the top-level overlay. Fixes angular#10799.
crisbeto
force-pushed
the
10799/top-level-overlay-dispatch
branch
from
5a14063 to
2ec751b
Compare
|
Looks like it was a flake @andrewseguin. It went away after a rebase. |
|
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. |
Currently when dispatching events through the
OverlayKeyboardDispatcherwe try to match one of the overlays to the element that triggered the event. This is problematic, because some components will open an overlay, but will keep focus on the trigger element (e.g.mat-autocompleteandmat-select). These changes switch the logic so the keyboard events are always dispatched to the top-level overlay.Fixes #10799.