-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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(cdk/tree): update active item on focus #27908
fix(cdk/tree): update active item on focus #27908
Conversation
536265a
to
1c01493
Compare
Hi @BobobUnicorn , This is what I came up with for supporting programmatic focus. The first approach I tried was finding a way to use CdkTreeNodeComponent.focus. That was difficult because I couldn’t find a way to get a reference to the CdkTreeNodeComponent of the “Load More” node. It seemed unnecessary for me to maintain a QueryList of tree node component when the tree already had one. I ended up finding it easier to use DOM manipulation to find the dom node I wanted and call .focus on it. I also remove the onClick method on TreeKeyManager so I wouldn’t have to add a duplicate method for onFocus. I exposed updateActiveItem to align with FocusKeyManager. I also make a few changes to “Load More” example so we can test when there are two “Load More” nodes and I took care of misc small details in the demo. WDYT? -Zach |
|
||
if (focusDesination) { | ||
// Restore focus. | ||
(focusDesination as HTMLElement).focus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the one thing I'm concerned about here is whether or not the tabindex updates correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a unit test to confirm that the tabindex updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, that's something else. Let me double check that manually right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the Load More example, it it correctly updates tabindex when clicking "Load More". It focuses the first new item that appears and sets it's tabindex to 0. All other tree nodes have tabindex="-1".
There's also a unit test to verify that CdkTreeNode correctly responds to focus event. Calling .focus()
will trigger native focus event.
5922146
to
a34ad99
Compare
Update CdkTreeNode, TreeKeyManager and Tree "Load More" example regarding progrmatic focus. - In CdkTreeNode, update the active item on the focus event. - expose TreeKeyManager.updateActiveItem - remove TreeKeyManager.onClick - In "Load More" example, when clicking "Load More", focus first node that is added.
1c01493
to
704bc1d
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
fix(cdk/tree): update active item on focus
Update CdkTreeNode, TreeKeyManager and Tree "Load More" example
regarding progrmatic focus.
that is added.