fix(menu): keyboard controls not respecting DOM order when items are added at a later point by crisbeto · Pull Request #11720 · angular/components

crisbeto · 2018-06-08T19:20:36Z

A while ago we reworked the menu not to pick up the items of other child menus. As a result, we had to use a custom way of registering and de-registering the menu items, however that method ends up adding any newly-created items to the end of the item list. This will throw off the keyboard navigation if an item is inserted in the beginning or the middle of the list. With these changes we switch to picking up the items using ContentChildren and filtering out all of the indirect descendants.

Fixes#11652.

jelbourn · 2018-06-28T17:27:20Z


 ngAfterContentInit() {
- this._keyManager = new FocusKeyManager<MatMenuItem>(this._items).withWrap().withTypeAhead();
+ this._allItems.changes


Add comment here that explains what you're doing (collecting direct descendants) and why they're needed? Might also be good to break this out into its own method like _updateDirectDescendants

crisbeto · 2018-06-29T16:14:54Z

Addressed the feedback.

jelbourn

LGTM

jelbourn · 2018-08-26T16:43:12Z

@crisbeto github shows this PR as having no conflicts, but our presubmit script thinks it does. Could you rebase it?

crisbeto · 2018-08-26T16:49:49Z

Done.

ngbot · 2018-11-03T00:30:54Z

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

devversion · 2018-11-08T21:52:55Z

+
+ /**
+ * @deprecated To be removed.
+ * @deletion-target 8.0.0


Suggested change
* @deletion-target8.0.0
* @breaking-change8.0.0
There are two lint failures for removeItem and addItem

Thanks, fixed.

dmk1111 · 2018-12-12T12:35:43Z

Hi @devversion @jelbourn @crisbeto !
Are there any updates on this? We faced this issue with dynamic menu items, so it would be nice to know when it will be released approximately.

Thanks!

scriby · 2019-01-16T03:23:22Z

I hit this bug today (working on a Google product at Google)

mmalerba · 2019-03-13T15:57:48Z

there are some internal apps that need to be cleaned up before this can be merged

andrewseguin · 2019-07-08T18:30:56Z

@crisbeto Can you check out the test failures? I'm working with a team internally to fix a test so we can get this merged

…added at a later point A while ago we reworked the menu not to pick up the items of other child menus. As a result, we had to use a custom way of registering and de-registering the menu items, however that method ends up adding any newly-created items to the end of the item list. This will throw off the keyboard navigation if an item is inserted in the beginning or the middle of the list. With these changes we switch to picking up the items using `ContentChildren` and filtering out all of the indirect descendants. Fixesangular#11652.

crisbeto · 2019-07-08T18:55:30Z

Fixed @andrewseguin. Looks like there were some breaking changes in 8.0 I hadn't accounted for.

…ems are added at a later point (angular#11720)" This reverts commit 49e8c59.

angular-automatic-lock-bot · 2019-09-11T01:02:49Z

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 assigned jelbourn Jun 8, 2018

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 8, 2018

jelbourn reviewed Jun 28, 2018
View reviewed changes

crisbeto force-pushed the 11652/menu-children-order branch from fcfef34 to 7b64465Compare June 29, 2018 16:14

crisbeto added the target: patch This PR is targeted for the next patch release label Jun 29, 2018

jelbourn approved these changes Jun 29, 2018
View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jun 29, 2018

crisbeto force-pushed the 11652/menu-children-order branch from 7b64465 to 1276018Compare August 26, 2018 16:49

crisbeto mentioned this pull request Sep 7, 2018
*ngIf affects button focus order #13034
Closed

crisbeto added the G This is is related to a Google internal issue label Sep 10, 2018

crisbeto force-pushed the 11652/menu-children-order branch from 1276018 to 03b01ecCompare November 8, 2018 10:03

crisbeto added the Accessibility This issue is related to accessibility (a11y) label Nov 8, 2018

crisbeto mentioned this pull request Nov 8, 2018
fix(material/menu): focus lost if active item is removed #14039
Merged

devversion reviewed Nov 8, 2018
View reviewed changes

crisbeto force-pushed the 11652/menu-children-order branch from 03b01ec to 91e67fcCompare November 8, 2018 22:34

crisbeto force-pushed the 11652/menu-children-order branch from 91e67fc to 2fd886aCompare December 15, 2018 16:48

crisbeto force-pushed the 11652/menu-children-order branch 2 times, most recently from 7cdd32a to 2eb0545Compare January 16, 2019 20:37

crisbeto force-pushed the 11652/menu-children-order branch 2 times, most recently from 9f5ef98 to 410fc9dCompare February 22, 2019 15:52

crisbeto added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Feb 26, 2019

jelbourn added P2 The issue is important to a large percentage of users, with a workaround and removed P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful labels Mar 6, 2019

mmalerba added the blocked This issue is blocked by some external factor, such as a prerequisite PR label Mar 13, 2019

mmalerba added aaa and removed aaa labels Apr 25, 2019

andrewseguin added the pr: needs rebase label Jul 1, 2019

crisbeto removed the pr: needs rebase label Jul 7, 2019

crisbeto force-pushed the 11652/menu-children-order branch from 410fc9d to e431613Compare July 7, 2019 15:05

crisbeto force-pushed the 11652/menu-children-order branch from e431613 to 243bd2fCompare July 8, 2019 18:41

jelbourn removed the blocked This issue is blocked by some external factor, such as a prerequisite PR label Jul 9, 2019

jelbourn merged commit 49e8c59 into angular:masterJul 9, 2019

jelbourn added a commit to jelbourn/components that referenced this pull request Jul 11, 2019
Revert "fix(menu): keyboard controls not respecting DOM order when it…
49f5428
…ems are added at a later point (angular#11720)" This reverts commit 49e8c59.

jelbourn mentioned this pull request Jul 11, 2019
Revert "fix(menu): keyboard controls not respecting DOM order when it… #16502
Closed

osuritz mentioned this pull request Jul 18, 2019
Menu items are not accessible via keyboard when using ng-content #14874
Closed

dgsmith2 mentioned this pull request Aug 1, 2019
mat-menu generation using dynamic data not showing arrow and nested sub-menus on hover #16457
Closed

crisbeto mentioned this pull request Aug 12, 2019
addItem method is empty in node_modules/@angular/material/esm2015/menu.js #16753
Closed

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

Uh oh!

Conversation

crisbeto commented Jun 8, 2018

Uh oh!

jelbournJun 28, 2018

Choose a reason for hiding this comment

Uh oh!

crisbeto commented Jun 29, 2018

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Aug 26, 2018

Uh oh!

crisbeto commented Aug 26, 2018

Uh oh!

ngbotBot commented Nov 3, 2018

Uh oh!

devversionNov 8, 2018

Choose a reason for hiding this comment

Uh oh!

crisbetoNov 8, 2018

Choose a reason for hiding this comment

Uh oh!

dmk1111 commented Dec 12, 2018

Uh oh!

scriby commented Jan 16, 2019

Uh oh!

mmalerba commented Mar 13, 2019

Uh oh!

andrewseguin commented Jul 8, 2019

Uh oh!

crisbeto commented Jul 8, 2019

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants