components icon indicating copy to clipboard operation
components copied to clipboard

fix(material/tree): add levelAccessor, childrenAccessor, TreeKeyManag… …er; a11y and docs improvements

Open zarend opened this issue 1 year ago • 10 comments

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:

  • add CdkTreeKeyManager to provide keyboard navigation for CdkTree and MatTree
  • Improve keyboard usability of CdkTreeNodeToggle.
  • Improve ARIA semantics of CdkTree, CdkTreeNode, Tree and TreeNode components
  • Fix miscellaneous accessibility issues in tree and cdk-tree examples
  • Add accessibility instructions to documentation

Documentation:

  • Add API and usage examples for TreeKeyManager
  • Update @angular/cdk/tree and @angular/material/tree to be more consistent
  • Update examples to use levelAccessor and childrenAccessor
  • Add example for (activation) on MatTreeNode and CdkTreeNode

API ADDITION: add CdkTree#childrenAccessor and CdkTree#levelAccessor

  • Add CdkTree#childrenAccessor. Given a data node, childrenAccessor determines the children of that node.
  • Add CdkTree#levelAccessor. Given a data node, levelAccessor determines the level of the node in the parent hierarchy.
  • CdkTreeNode#levelAcessor and CdkTreeNode#childrenAccessor replace CdkTreeNode#treeControl.

See “Deprecated” for updating apps using treeControl.

API ADDITION: control expanded state of tree nodes using isExpandable and isExpanded

  • Add CdkTreeNode#isExpandable, determines if argument tree node can be expanded or collapsed.
  • CdkTreeNode#isExpanded to specify the expanded state. Has no effect if node is not expandable.
  • Add NestedTreeControlOptions#isExpandable function, determines if argument tree node can be expanded or collapsed.

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

  • Add CdkTree#isExpanded method.
  • Add CdkTree#toggle, CdkTree#expand and CdkTree#collapse methods.
  • Add CdkTree#toggleDescendants, CdkTree#expandDescendants, and CdkTree#collapseDescendants methods to CdkTree
  • Add CdkTree#expandAll and CdkTree#collapseAll methods
  • Add expandedChange Output to CdkTreeNode

API ADDITION: add injection token for tree-key-manager

  • Add TREE_KEY_MANAGER injection token. When provided, tree uses given key manager
  • TreeKeyManagerStrategy interface, which defines API contract of TREE_KEY_MANAGER

BEHAVIOR CHANGE: MatTree and CdkTree components respond to keyboard navigation.

  • CdkTree and MatTree respond to arrow keys, page up, page down, etc.; Keyboard commands match WAI ARIA Tree View Pattern.
  • Can no longer set the tabindex on MatTreeNode. See “Deprecated” for adopting existing applications.
  • Add TreeKeyManager to cdk/a11y

DEPRECATED: Tree controller deprecated. Use one of levelAccessor or childrenAccessor instead. To be removed in a future version.

  • BaseTreeControl, TreeControl, FlatTreeControl, and NestedTreeControl deprecated
  • CdkTree#treeControl deprecated. Provide one of CdkTree#levelAccessor or CdkTree#childrenAccessor instead.
  • MatTreeFlattener deprecated. Use MatTree#childrenAccessor and MatTreeNode#isExpandable instead.
  • MatTreeFlatDataSource deprecated. Use one of levelAccessor or childrenAccessor instead of TreeControl.

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.

  • MatTreeNode#tabIndex deprecated. MatTreeNode ignores Input tabIndex and manages its own focus behavior.
  • MatTreeNode#defaultTabIndex deprecated. MatTreeNode ignores defaultTabIndex and manages its own focus behavior.
  • MatNestedTreeNode#tabIndex deprecated. MatTreeNode ignores Input defaultTabIndex and manages its own focus behavior.
  • LegacyTreeKeyManager and LEGACY_TREE_KEY_MANAGER_FACTORY_PROVIDER deprecated. Inject a TreeKeyManagerFactory to customize keyboard behavior.

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.

  • CdkTreeNode#disabled deprecated and alias to CdkTreeNode#isDisabled

zarend avatar Aug 11 '23 16:08 zarend

Deployed dev-app for e7a5b31193cc87789d9fd759bc0bd3f0ccaa5307 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.

github-actions[bot] avatar Aug 11 '23 16:08 github-actions[bot]

"Squashing" commit from fix(cdk/tree): cleanup tree-key-manager token.

zarend avatar Oct 30 '23 21:10 zarend

I plan on no longer squashing this branch and force pushing. That makes it easier for reviews.

zarend avatar Oct 31 '23 20:10 zarend

Deployed dev-app for ab5cb56f63a70fb663a5d22a99fe1a7702efc709 to: https://ng-dev-previews-comp--pr-angular-components-27626-dev-acw8og4u.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

github-actions[bot] avatar Nov 08 '23 22:11 github-actions[bot]

Since this was last reviewed: resolved merged conflicts.

Resolved conflicts by first rebasing with target branch then merging target branch into this branch.

This is planned to merge as a single commit. Additional commits will be squashed away.

zarend avatar Nov 08 '23 22:11 zarend

https://github.com/angular/components/pull/27626#discussion_r1423264735

@jelbourn what's the intention you have in mind with coerceObservable? It doesn't do anything that's not trivial, so I don't see the value with making it reusable. If we did want to put something in coercion, then it would make sense if it also worked wiht promises ((x: T | Promise<T> | Observable<T>) => Observable<T>). I don't think I quite see how moving it would help giving that rxjs already has library functions for coercion, and I don't think cdk/coercion is intended to be in scope of this pr.

I have a suggestion. Delete coerceObservable because we don't really need it. Instead, repurpose the _getChildrenAccessor method to do the type coercion.

/// **note to reader: rename `_getChildrenAccessor` to `_getChildren()`
  /** Children accessor, used for compatibility between the old Tree and new Tree */
  _getChildren() {
    const getChildren = this.treeControl?.getChildren ?? this.childrenAccessor;
    const children = getChildren();
    return isObservable(children) ? children : observableOf(children);
  }

zarend avatar Dec 12 '23 19:12 zarend

#27626 (comment)

@jelbourn what's the intention you have in mind with coerceObservable? It doesn't do anything that's not trivial, so I don't see the value with making it reusable. If we did want to put something in coercion, then it would make sense if it also worked wiht promises ((x: T | Promise<T> | Observable<T>) => Observable<T>). I don't think I quite see how moving it would help giving that rxjs already has library functions for coercion, and I don't think cdk/coercion is intended to be in scope of this pr.

I have a suggestion. Delete coerceObservable because we don't really need it. Instead, repurpose the _getChildrenAccessor method to do the type coercion.

/// **note to reader: rename `_getChildrenAccessor` to `_getChildren()`
  /** Children accessor, used for compatibility between the old Tree and new Tree */
  _getChildren() {
    const getChildren = this.treeControl?.getChildren ?? this.childrenAccessor;
    const children = getChildren();
    return isObservable(children) ? children : observableOf(children);
  }

I have a better idea, which is to create an adapter for interface TreeKeyManagerItem.

  getChildren<T extends TreeKeyManagerItem>(item: T): Observable<TreeKeyManagerItem[]> {
    const children = item.getChildren();
    return isObservable(children) ? children : observableOf(children);
  },
  isDisabled<T extends TreeKeyManagerItem>(item: T): boolean {
    return typeof item.isDisabled === 'boolean' ? item.isDisabled : !!item.isDisabled?.();
  },
  isExpanded<T extends TreeKeyManagerItem>(item: T): boolean {
    return typeof item.isExpanded === 'boolean' ? item.isExpanded : !!item.isExpanded?.();
  },

zarend avatar Dec 14 '23 22:12 zarend

re: coerceObservable, I think it's still better off as its own private function and does essentially fit with the rest of cdk/coercion as they're all simple helper functions anyway

BobobUnicorn avatar May 21 '24 20:05 BobobUnicorn

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.

BobobUnicorn avatar May 21 '24 20:05 BobobUnicorn