fix(menu): not handling keyboard events when opened by mouse by crisbeto · Pull Request #4843 · angular/components

crisbeto · 2017-05-27T12:34:46Z

Fixes the menu keyboard interactions not working when it is opened by a click.

jelbourn · 2017-05-30T16:27:29Z

 this._overlayRef.attach(this._portal);
 this._subscribeToBackdrop();
+
+ this._globalKeydownHandler = this._renderer.listen('document', 'keydown',


How will this global listener play with, say, a menu inside of a dialog?

Hmm, it'll close both the menu and the dialog. What about focusing the menu panel, instead of the first option, when the menu is opened by mouse?

The options are focused for a11y

Yes, but it seems pretty deliberate that we're not shifting focus when the user opened it by mouse. If we want to keep that behavior, we could opt to move focus to the panel, if the menu was opened by mouse, or focus the first option if it was opened by keyboard. Otherwise we can also focus the first option for all cases.

@kara do you remember why we don't move focus to the options when opening via mouse? If it was just to prevent the focus style from showing, we could probably just do that through css.

I believe it was just for the visual treatment. I don't think there's any reason not to focus the options in itself.

How should I proceed in this case? Given that we also have #4991, should I just enable the keyboard navigation no matter how you opened the menu?

We should make it always focus the options no matter how you open the panel, but only show the initial focus style (before subsequent key presses) when it's triggered by keyboard

crisbeto · 2017-06-18T10:49:41Z

@jelbourn @kara, I got around to reworking this. If the menu was opened by mouse, the panel will receive focus, which allows the user to use escape and go into the options by pressing the arrow keys.

kara · 2017-06-26T23:12:31Z

@crisbeto Can you rebase this?

crisbeto · 2017-06-27T04:26:48Z

Done @kara.

kara

I noticed something odd while testing this. Here's how to repro:

Tab to the menu trigger and hit enter to open.
Navigate down to the second option with the arrow keys.
Hit enter.
Now open the trigger with your mouse.
Hit the down arrow.

The focus will be on the next enabled option (the 4th) rather than resetting focus to the first option.

kara · 2017-06-27T22:19:19Z

+ // If the menu was opened by mouse, we focus the root node, which allows for the keyboard
+ // interactions to work. Otherwise, if the menu was opened by keyboard, we focus the first item.
+ if (this._openedByMouse) {
+ let rootNode = this._overlayRef.overlayElement.firstElementChild as HTMLElement;


The _overlayRef type is sometimes null, so the strictNullCheck in the build is complaining. Add a ! ?

crisbeto · 2017-06-28T20:31:22Z

@kara can you take another look?

kara · 2017-06-29T17:30:08Z

@crisbeto It looks like after you open with the mouse, the first DOWN arrow highlights the second option. @jelbourn and I were thinking the first option might make more sense in that case. Update?

crisbeto · 2017-06-29T17:51:59Z

@kara Implemented the approach as discussed.

kara · 2017-08-15T00:54:35Z

@crisbeto Sorry for the delayed review on this. Do you mind rebasing again?

kara

LGTM

kara · 2017-08-21T20:50:50Z

Note: needs internal updates. Breaking.

Fixes the menu keyboard interactions not working when it is opened by a click. Fixesangular#4991.

angular-automatic-lock-bot · 2019-09-07T09:02:34Z

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.}

crisbeto added the pr: needs review label May 27, 2017

crisbeto assigned kara May 27, 2017

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

willshowell mentioned this pull request May 30, 2017
feat(dialog): expose close attempts #3460
Closed

jelbourn reviewed May 30, 2017
View reviewed changes

crisbeto force-pushed the menu-escape branch from d2e11a8 to a5cc967Compare June 18, 2017 10:47

crisbeto changed the title ~~fix(menu): not closing on escape when opened by mouse~~fix(menu): not handling keyboard events when opened by mouseJun 18, 2017

crisbeto force-pushed the menu-escape branch from a5cc967 to 0b4fe40Compare June 27, 2017 04:26

kara suggested changes Jun 27, 2017
View reviewed changes

kara assigned crisbeto and unassigned kara Jun 27, 2017

kara added the pr: needs cleanup / tests label Jun 27, 2017

crisbeto force-pushed the menu-escape branch 2 times, most recently from 4e4f6c3 to 7b56290Compare June 28, 2017 20:11

crisbeto removed the pr: needs cleanup / tests label Jun 28, 2017

crisbeto assigned kara and unassigned crisbeto Jun 28, 2017

kara assigned crisbeto and unassigned kara Jun 29, 2017

crisbeto force-pushed the menu-escape branch from 7b56290 to 6c727feCompare June 29, 2017 17:51

crisbeto assigned kara and unassigned crisbeto Jun 29, 2017

kara removed their assignment Aug 15, 2017

kara assigned crisbeto Aug 15, 2017

kara added the pr: needs rebase label Aug 15, 2017

crisbeto force-pushed the menu-escape branch from 6c727fe to 408a05fCompare August 15, 2017 18:36

crisbeto removed the pr: needs rebase label Aug 15, 2017

kara self-assigned this Aug 15, 2017

kara approved these changes Aug 15, 2017
View reviewed changes

kara unassigned kara and crisbeto Aug 15, 2017

crisbeto force-pushed the menu-escape branch from 408a05f to ed99a0aCompare August 27, 2017 16:49

crisbeto force-pushed the menu-escape branch from ed99a0a to 25fbc95Compare September 14, 2017 20:10

fix(menu): not handling keyboard events when opened by mouse
7eda6c4
Fixes the menu keyboard interactions not working when it is opened by a click. Fixesangular#4991.

crisbeto force-pushed the menu-escape branch from 25fbc95 to 7eda6c4Compare October 7, 2017 15:47

andrewseguin merged commit d822a39 into angular:masterOct 10, 2017

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

Uh oh!

Conversation

crisbeto commented May 27, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jelbournMay 30, 2017

Choose a reason for hiding this comment

Uh oh!

crisbetoMay 30, 2017

Choose a reason for hiding this comment

Uh oh!

jelbournMay 30, 2017

Choose a reason for hiding this comment

Uh oh!

crisbetoMay 30, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbournMay 30, 2017

Choose a reason for hiding this comment

Uh oh!

karaJun 8, 2017

Choose a reason for hiding this comment

Uh oh!

crisbetoJun 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbournJun 8, 2017

Choose a reason for hiding this comment

Uh oh!

crisbeto commented Jun 18, 2017

Uh oh!

kara commented Jun 26, 2017

Uh oh!

crisbeto commented Jun 27, 2017

Uh oh!

kara left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karaJun 27, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto commented Jun 28, 2017

Uh oh!

kara commented Jun 29, 2017

Uh oh!

crisbeto commented Jun 29, 2017

Uh oh!

kara commented Aug 15, 2017

Uh oh!

kara left a comment

Choose a reason for hiding this comment

Uh oh!

kara commented Aug 21, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

crisbeto commented May 27, 2017•
edited
Loading

crisbetoMay 30, 2017•
edited
Loading

crisbetoJun 8, 2017•
edited
Loading

kara left a comment •
edited
Loading

karaJun 27, 2017•
edited
Loading

kara commented Aug 21, 2017•
edited
Loading