Skip to content

Conversation

kara
Copy link
Contributor

@kara kara commented Aug 30, 2016

This PR adds keyboard events and proper focus management to the menu.

r: @jelbourn

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 30, 2016
@kara kara added in progress This issue is currently in progress and removed pr: needs review labels Aug 30, 2016
@kara kara added pr: needs review and removed in progress This issue is currently in progress labels Aug 30, 2016
* to focus the first item when the menu is opened by the ENTER key.
* TODO: internal
*/
_focusFirstItem() { this.items.first.focus(); }
Copy link
Member

Choose a reason for hiding this comment

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

Personally I prefer to always have the newlines for method definitions

@jelbourn jelbourn changed the title feat(menu): add keyboard events and accessibility feat(menu): add keyboard events and improve accessibility Aug 31, 2016
@kara kara added in progress This issue is currently in progress and removed pr: needs review labels Aug 31, 2016
@kara kara force-pushed the keyboard-pr branch 3 times, most recently from 5a9ba46 to 0bb6e93 Compare September 1, 2016 01:45
@kara kara added pr: needs review and removed in progress This issue is currently in progress labels Sep 1, 2016
@kara
Copy link
Contributor Author

kara commented Sep 1, 2016

@jelbourn Comments should be addressed, and updated the keyboard behavior.

also adds `aria-hasPopup="true"` to the trigger element.

#### Keyboard events:
- <kbd>DOWN_ARROW</kbd> or <kbd>TAB</kbd>: Focus next menu item
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to remove Tab shortcuts now?

Copy link
Contributor Author

@kara kara Sep 1, 2016

Choose a reason for hiding this comment

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

Yes! Good catch. Updating..

@jelbourn
Copy link
Member

jelbourn commented Sep 1, 2016

LGTM aside from that one comment

@kara kara added the action: merge The PR is ready for merge by the caretaker label Sep 1, 2016
@kara kara merged commit 3669f06 into angular:master Sep 1, 2016
@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 6, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants