-
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(material/tree): add levelAccessor, childrenAccessor, TreeKeyManag… …er; a11y and docs improvements #27626
Conversation
Deployed dev-app for e7a5b31 to: https://ng-dev-previews-comp--pr-angular-components-27626-gieu7oe0.web.app Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt. |
e65f49a
to
3665278
Compare
d451c18
to
aedb0b4
Compare
38e48c7
to
b608e1f
Compare
b608e1f
to
3a8089d
Compare
3a8089d
to
efd22b0
Compare
f567a4f
to
b9a847f
Compare
export interface TreeKeyManagerOptions<T extends TreeKeyManagerItem> { | ||
/** | ||
* Sets the predicate function that determines which items should be skipped by the tree key | ||
* manager. By default, disabled items are skipped. |
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.
It feels weird that the interface documentation talks about defaults, since it doesn't have any implementation.
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.
done
private readonly _letterKeyStream = new Subject<string>(); | ||
private _typeaheadSubscription = Subscription.EMPTY; | ||
|
||
/** |
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.
Should the rest of the preceding private variables have similar jsdocs?
Fix miscellaneous PR comments. Take a first pass to fix comments that are easiest to fix. This commit message will be squashed away when merging. * move type definitions in tree-key-manager.ts to a new file, tree-key-manager-strategy.ts * remove unnecessary comment * fix JSDoc comment syntax * remove isNotNullish * consolidate event handling code in CdkTreeNodeToggle * remove unnecessary type coerscion * simply logic for detecting TreeControlMissingError and MultipleTreeControlsError * adopt Input transform * remove unnecessary nesting in _updateActiveItemIndex
Use OnPush change detection in Tree examples.
refactor(cdk/tree): simplify code in lifecycle hooks Refactor CdkTree implementation to simplify code in lifecycle hooks. Create a RenderingData type definition to simplify the Tree's rendering pipeline. The render pipeline looks like this. It has two entry points ngAfterContentChecked and whener the datasource is switched. 1. ngAfterContentChecked OR datasource changed 2. call _subscribeToDataChanges From here, the pipeline diverges. Tree subscribes to changes in the datasource and expansion state. From there, the pipeline goes on two paths: emit changes to expansion state, and render the changes from the datasource. Add RenderingData type to make it more clear what stages are in the pipeline. This commit message will be squashed away.
Fix miscelaneous focus management details in response to PR feedback. * omit tabindex attribute on role="tree" element * Remove TreeKeyManagerStrategy#onInitialFocus * Implement focus/unfocus pattern for TreeKeyManagerOption and use a binding to set tabindex on tree nodes * algin with https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/#focusabilityofdisabledcontrols * Add tree-using-legacy-key-manager.spec.ts
…8142) factor out a typeahead class for key managers
Make miscelaneous fixes to documentation. Rename variables. Responding to PR feedback. * remove comment about API contract of CdkTreeNodeToggle * rename CdkTree#_groups to CdkTree#_ariaSets * move documentation about default key manager configuration from TreeKeyManagerStrategy interface to TreeKeyManager class. * add JSDoc style comments for NoopTreeKeyManager
Add unit tests to cover the use of LegacyTreeKeyManager, which opts out of updated focus management behavior.
Note to reviewers: see unit tests for opt-out behavior: |
@@ -27,7 +27,7 @@ Navigation through options can be made to wrap via the `withWrap` method | |||
this.keyManager = new FocusKeyManager(...).withWrap(); | |||
``` | |||
|
|||
#### Types of key managers | |||
#### Types of list key managers | |||
|
|||
There are two varieties of `ListKeyManager`, `FocusKeyManager` and `ActiveDescendantKeyManager`. |
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.
Should this sentence also be updated?
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'm not sure what needs updating here?
`TreeKeyManager` manages the active option in a tree view. This is intended to be used with | ||
components that correspond to a `role="tree"` pattern. |
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.
`TreeKeyManager` manages the active option in a tree view. This is intended to be used with | |
components that correspond to a `role="tree"` pattern. | |
`TreeKeyManager` manages the active option in a tree view. USe this key manager for | |
components that implement a `role="tree"` pattern. |
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.
done
|
||
#### Basic usage | ||
|
||
Any component that uses a `TreeKeyManager` will generally do three things: |
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.
Any component that uses a `TreeKeyManager` will generally do three things: | |
Any component that uses a `TreeKeyManager` should do three things: |
nit: technical docs should already be present tense. Also replaced "generally do" with the stronger "should" (which I think is accurate here)
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.
done
@@ -150,6 +150,7 @@ describe('Key managers', () => { | |||
|
|||
keyManager.setActiveItem(0); | |||
itemList.reset([new FakeFocusable('zero'), ...itemList.toArray()]); | |||
itemList.notifyOnChanges(); |
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.
Why is this manually calling notifyOnChanges
? IIRC, this wasn't meant to be part of the public API of QueryList
; tests would generally want to cause change detection instead (though it's possible I'm missing something here)
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.
It does notifyOnChanges
instead of change detection because there is no test fixture. This test file doesn't create any kind of test component or use TestBed at all. It's vanilla test, so change detection is not available.
skipPredicate?: (item: T) => boolean | undefined; | ||
} | ||
|
||
export class Typeahead<T extends TypeaheadItem> { |
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.
JsDoc for the responsibility of this class?
return ( | ||
this.treeControl?.isExpanded(dataNode) ?? | ||
this._expansionModel?.isSelected(this._getExpansionKey(dataNode)) ?? | ||
false | ||
); |
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.
return ( | |
this.treeControl?.isExpanded(dataNode) ?? | |
this._expansionModel?.isSelected(this._getExpansionKey(dataNode)) ?? | |
false | |
); | |
return !!( | |
this.treeControl?.isExpanded(dataNode) || | |
this._expansionModel?.isSelected(this._getExpansionKey(dataNode))); |
optional: I find the logical ||
with boolean coercion more readable in this situation
if (levelAccessor) { | ||
return combineLatest([isExpanded, this._flattenedNodes]).pipe( | ||
map(([expanded, flattenedNodes]) => { | ||
if (!expanded) { | ||
return []; | ||
} | ||
const startIndex = flattenedNodes.findIndex(node => this._getExpansionKey(node) === key); | ||
const level = levelAccessor(dataNode) + 1; | ||
const results: T[] = []; | ||
|
||
// Goes through flattened tree nodes in the `flattenedNodes` array, and get all direct | ||
// descendants. The level of descendants of a tree node must be equal to the level of the | ||
// given tree node + 1. | ||
// If we reach a node whose level is equal to the level of the tree node, we hit a sibling. | ||
// If we reach a node whose level is greater than the level of the tree node, we hit a | ||
// sibling of an ancestor. | ||
for (let i = startIndex + 1; i < flattenedNodes.length; i++) { | ||
const currentLevel = levelAccessor(flattenedNodes[i]); | ||
if (level > currentLevel) { | ||
break; | ||
} | ||
if (level === currentLevel) { | ||
results.push(flattenedNodes[i]); | ||
} | ||
} | ||
return results; | ||
}), | ||
); | ||
} | ||
const childrenAccessor = this._getChildrenAccessor(); | ||
if (childrenAccessor) { | ||
return coerceObservable(childrenAccessor(dataNode) ?? []); | ||
} | ||
throw getTreeControlMissingError(); |
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.
It would be a bit easier to follow if there was a breakout function like getDirectChildrenByLevel
if (this.treeControl) { | ||
return observableOf(this.treeControl.getDescendants(dataNode)); | ||
} | ||
if (this.levelAccessor) { |
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.
Similar comment here about breakout function like getDescendantsByLevel
* | ||
* This also computes parent, level, and group data. | ||
*/ | ||
private _convertData( |
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.
This function name (and description) is pretty vague- could it be more specific?
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.
That this method actual does is map data from the DataSource into RenderingData
type.
It also needs to handle all the different cases for nodeType. The Tree infers the type of the nodes based on the data its DataSource gives it. Handling nodeType is part of why there are so many conditional cases here.
|
||
#### Connecting the tree to a data source | ||
|
||
Similar to `mat-table`, data is provided to the tree through a `DataSource`. When the tree receives |
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.
Similar to `mat-table`, data is provided to the tree through a `DataSource`. When the tree receives | |
Similar to `mat-table`, you can provide data to the tree through a `DataSource`. When the tree receives |
nit: passive voice
@jelbourn what's the intention you have in mind with I have a suggestion. Delete
|
I have a better idea, which is to create an adapter for
|
* fix(cdk/tree): fix errors from testing * fix(cdk/tree): tests * fix(cdk/tree): update api docs * fix(cdk/a11y): allows disabled items to receive initial focus * fix(cdk/tree): don't focus on click, corrected updating aria-sets * fix(cdk/tree): update api goldens
Implement typeahead labels for Tree. Follow the established pattern that Menu and List use. - implement CdkTreeNode#cdkTreeNodeTypeaheadLabel - implement CdkTreeNode#getLabel
re: |
This PR will be continued in #29062; I've tried to address the remaining commentary here in that PR, though unfortunately we don't have continuity with the comment chains. |
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. |
Update multiple facets of Tree component. Add APIs to manage data models, improve existing behaviors, add keyboard functionality and update documentation.
Add APIs options to the Tree data model by introducing levelAccessor and childrenAccessor. See “Api Addition” for usage. Currently, Tree component use TreeControl to manage data model. When applied, add levelAccessor and childrenAccessor functions as alternatives to TreeControl.
Add TreeKeyManager, which provides keyboard functionality. Currently Tree component allows developers to manage focus by setting tabindex on each tree node. When applied, Tree manages its own focus using key manager pattern. Keyboard commands match WAI ARIA Tree View Pattern. See “Deprecated” for adopting changes to existing applications.
Correct the ARIA semantics of Tree and Tree node components.
Document updated APIs and behaviors. Refine documentation of existing APIs and behaviors.
Changes to Cdk Tree API also apply to Mat Tree API. See “Deprecated” for adopting changes to existing applications.
Accessibility:
Documentation:
API ADDITION: add CdkTree#childrenAccessor and CdkTree#levelAccessor
See “Deprecated” for updating apps using treeControl.
API ADDITION: control expanded state of tree nodes using isExpandable and isExpanded
For trees using treeControl, recommend providing isExpandable if not already provided. See “Deprecated” for more information on updating applications.
API ADDITION: use CdkTree to manage expansion state
API ADDITION: add injection token for tree-key-manager
BEHAVIOR CHANGE: MatTree and CdkTree components respond to keyboard navigation.
DEPRECATED: Tree controller deprecated. Use one of levelAccessor or childrenAccessor instead. To be removed in a future version.
Note when upgrading: isExpandable works differently on Trees using treeControl than trees using childrenAccessor or levelAccessor. Nodes on trees that have a treeControl are expandable by default. Nodes on trees using childrenAccessor or levelAccessor are not expandable by default. Provide isExpandable to override default behavior.
DEPRECATED: Setting tabindex of tree nodes deprecated. By default, Tree ignores tabindex passed to tree nodes.
Note when upgrading: an opt-out is available for keyboard functionality changes. Provide LEGACY_TREE_KEY_MANAGER_FACTORY_PROVIDER to opt-out of Tree managing its own focus. When provided, Tree does not manage it’s own focus and respects tabindex passed to TreeNode. When provided, have the same focus behavior as before this commit is applied.
Add Legacy Keyboard Interface demo, which shows usage of LEGACY_TREE_KEY_MANAGER_FACTORY_PROVIDER. Add Custom Key Manager, which shows usage of injecting a TreeKeyManagerStrategy
DEPRECATED: disabled renamed to isDisabled.