Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(menu): not handling keyboard events when opened by mouse #4843

Merged
merged 1 commit into from
Oct 10, 2017

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented May 27, 2017

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

Fixes #4991.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 27, 2017
@@ -104,6 +107,14 @@ export class MdMenuTrigger implements AfterViewInit, OnDestroy {
this._createOverlay();
this._overlayRef.attach(this._portal);
this._subscribeToBackdrop();

this._globalKeydownHandler = this._renderer.listen('document', 'keydown',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The options are focused for a11y

Copy link
Member Author

@crisbeto crisbeto May 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@crisbeto crisbeto Jun 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 crisbeto changed the title fix(menu): not closing on escape when opened by mouse fix(menu): not handling keyboard events when opened by mouse Jun 18, 2017
@crisbeto
Copy link
Member Author

@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
Copy link
Contributor

kara commented Jun 26, 2017

@crisbeto Can you rebase this?

@crisbeto
Copy link
Member Author

Done @kara.

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

// 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;
Copy link
Contributor

@kara kara Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@crisbeto
Copy link
Member Author

@kara can you take another look?

@kara
Copy link
Contributor

kara commented Jun 29, 2017

@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
Copy link
Member Author

@kara Implemented the approach as discussed.

@crisbeto crisbeto assigned kara and unassigned crisbeto Jun 29, 2017
@kara
Copy link
Contributor

kara commented Aug 15, 2017

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

@kara kara removed their assignment Aug 15, 2017
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kara kara unassigned kara and crisbeto Aug 15, 2017
@kara kara added pr: lgtm action: merge The PR is ready for merge by the caretaker presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed pr: needs review presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged labels Aug 15, 2017
@kara
Copy link
Contributor

kara commented Aug 21, 2017

Note: needs internal updates. Breaking.

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

Fixes angular#4991.
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

menu up/down arrow does not navigate the overlay
5 participants