angular / components

Component infrastructure and Material Design components for Angular
https://material.angular.io
MIT License
24.18k stars 6.68k forks source link

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

Open zarend opened 10 months ago

zarend commented 10 months ago

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.

github-actions[bot] commented 10 months ago

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.

zarend commented 7 months ago

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

zarend commented 7 months ago

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

github-actions[bot] commented 7 months ago

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.

zarend commented 7 months ago

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 commented 6 months ago

Note to reviewers: see unit tests for opt-out behavior:

zarend commented 6 months ago

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 commented 6 months ago

#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?.();
  },
BobobUnicorn commented 3 weeks ago

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 commented 3 weeks ago

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.