Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

Conversation

Splaktar
Copy link
Contributor

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Pressing tab inside of a menu panel (md-menu-content) can cause the focus to escape the panel and move to other parts of the page (with the menu panel still open).

Issue Number:
Fixes #11123

What is the new behavior?

Use only arrow keys to navigate menus (not tabs).
Resolve issue where tab can escape the panel by closing the menu when the TAB keyCode is detected.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

N/A

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Feb 18, 2018
@Splaktar Splaktar requested a review from crisbeto February 18, 2018 03:11
@Splaktar Splaktar added a11y This issue is related to accessibility P1: urgent Urgent issues that should be addressed in the next minor or patch release. g3: reported The issue was reported by an internal or external product team. labels Feb 18, 2018
@Splaktar Splaktar added this to the 1.1.8 milestone Feb 18, 2018
@@ -289,6 +289,7 @@ function MenuProvider($$interimElementProvider) {
var handled;
switch (ev.keyCode) {
case $mdConstant.KEY_CODE.ESCAPE:
case $mdConstant.KEY_CODE.TAB:
opts.mdMenuCtrl.close(false, { closeAll: true });
handled = true;
Copy link
Member

@crisbeto crisbeto Feb 18, 2018

Choose a reason for hiding this comment

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

Looking at the logic below, when an event is marked as handled, it'll prevent the default action. This shouldn't be the case for TAB, because it'll prevent people from moving forward.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, good catch.

@Splaktar Splaktar added the pr: merge ready This PR is ready for a caretaker to review label Feb 20, 2018
@Splaktar
Copy link
Contributor Author

Feedback addressed. Ready for presubmit.

@@ -292,6 +292,10 @@ function MenuProvider($$interimElementProvider) {
opts.mdMenuCtrl.close(false, { closeAll: true });
handled = true;
break;
case $mdConstant.KEY_CODE.TAB:
opts.mdMenuCtrl.close(false, { closeAll: true });
handled = false;
Copy link
Member

Choose a reason for hiding this comment

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

Add comment that explains why handled = false here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

use only arrow keys to navigate menus
resolve issue where tab can escape the panel

Fixes #11123
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@josephperrott josephperrott merged commit 7e5b7f4 into master Feb 20, 2018
@Splaktar Splaktar deleted the closeMenuOnTab branch March 8, 2018 06:02
chmelevskij pushed a commit to chmelevskij/material that referenced this pull request Jun 19, 2018
use only arrow keys to navigate menus
resolve issue where tab can escape the panel

Fixes angular#11123
Splaktar added a commit that referenced this pull request Jul 31, 2018
use only arrow keys to navigate menus
resolve issue where tab can escape the panel

Fixes #11123
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a11y This issue is related to accessibility cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ g3: reported The issue was reported by an internal or external product team. P1: urgent Urgent issues that should be addressed in the next minor or patch release. pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

menu: should close when pressing tab
5 participants