-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix(material/tree): add levelAccessor, childrenAccessor, TreeKeyManag… …er; a11y and docs improvements #27626
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
Changes from all commits
e20bac8
ea41d88
9310d0f
104c403
c4aa307
372f9df
e5d0d9a
5356d69
4bc94e0
ab5cb56
ee50445
38cfe94
5d001ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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`. | ||||||||||
|
||||||||||
|
@@ -55,6 +55,64 @@ interface Highlightable extends ListKeyManagerOption { | |||||||||
|
||||||||||
Each item must also have an ID bound to the listbox's or menu's `aria-activedescendant`. | ||||||||||
|
||||||||||
### TreeKeyManager | ||||||||||
|
||||||||||
`TreeKeyManager` manages the active option in a tree view. This is intended to be used with | ||||||||||
components that correspond to a `role="tree"` pattern. | ||||||||||
Comment on lines
+60
to
+61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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 commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||||||
* Create a `@ViewChildren` query for the tree items being managed. | ||||||||||
* Initialize the `TreeKeyManager`, passing in the options. | ||||||||||
* Forward keyboard events from the managed component to the `TreeKeyManager` via `onKeydown`. | ||||||||||
|
||||||||||
Each tree item should implement the `TreeKeyManagerItem` interface: | ||||||||||
```ts | ||||||||||
interface TreeKeyManagerItem { | ||||||||||
/** Whether the item is disabled. */ | ||||||||||
isDisabled?: (() => boolean) | boolean; | ||||||||||
|
||||||||||
/** The user-facing label for this item. */ | ||||||||||
getLabel?(): string; | ||||||||||
|
||||||||||
/** Perform the main action (i.e. selection) for this item. */ | ||||||||||
activate(): void; | ||||||||||
|
||||||||||
/** Retrieves the parent for this item. This is `null` if there is no parent. */ | ||||||||||
getParent(): TreeKeyManagerItem | null; | ||||||||||
|
||||||||||
/** Retrieves the children for this item. */ | ||||||||||
getChildren(): TreeKeyManagerItem[] | Observable<TreeKeyManagerItem[]>; | ||||||||||
|
||||||||||
/** Determines if the item is currently expanded. */ | ||||||||||
isExpanded: (() => boolean) | boolean; | ||||||||||
|
||||||||||
/** Collapses the item, hiding its children. */ | ||||||||||
collapse(): void; | ||||||||||
|
||||||||||
/** Expands the item, showing its children. */ | ||||||||||
expand(): void; | ||||||||||
|
||||||||||
/** | ||||||||||
* Focuses the item. This should provide some indication to the user that this item is focused. | ||||||||||
*/ | ||||||||||
focus(): void; | ||||||||||
} | ||||||||||
``` | ||||||||||
|
||||||||||
#### Focus management | ||||||||||
|
||||||||||
The `TreeKeyManager` will handle focusing the appropriate item on keyboard interactions. However, | ||||||||||
the component should call `onInitialFocus` when the component is focused for the first time (i.e. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you expand on this a bit? What is the tabindex situation before it's focused for the first time? Will that initial focus land on the first item? the wrapper element? I guess I'm wondering where I need to add this call and if there's any special tabindex management I need to do myself before the key manager takes over There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MatTree implements the WAI ARIA Tree View Pattern. We intend to follow the keyboard behaviors in that document:
If you are making your own key manager, then you need to be familiar with I don't think The important part is that MatTree manages it's own focus. Inject a TreeKeyManager to change focus behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: tabIndex; currently we do require MatTree (or other tree implementations using the key manager) to manage tabIndex There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is no longer relevant; we handle initial focus internally without API intervention |
||||||||||
when there is no active item). | ||||||||||
|
||||||||||
`tabindex` should also be set by the component when the active item changes. This can be listened to | ||||||||||
via the `change` property on the `TreeKeyManager`. In particular, the tree should only have a | ||||||||||
`tabindex` set if there is no active item, and should not have a `tabindex` set if there is an | ||||||||||
active item. Only the HTML node corresponding to the active item should have a `tabindex` set to | ||||||||||
`0`, with all other items set to `-1`. | ||||||||||
|
||||||||||
|
||||||||||
### FocusTrap | ||||||||||
|
||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Why is this manually calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does |
||
keyManager.setActiveItem(0); | ||
|
||
expect(spy).toHaveBeenCalledTimes(1); | ||
|
@@ -342,6 +343,7 @@ describe('Key managers', () => { | |
const items = itemList.toArray(); | ||
items[1].disabled = true; | ||
itemList.reset(items); | ||
itemList.notifyOnChanges(); | ||
|
||
// Next event should skip past disabled item from 0 to 2 | ||
keyManager.onKeydown(this.nextKeyEvent); | ||
|
@@ -367,6 +369,7 @@ describe('Key managers', () => { | |
items[1].disabled = undefined; | ||
items[2].disabled = undefined; | ||
itemList.reset(items); | ||
itemList.notifyOnChanges(); | ||
|
||
keyManager.onKeydown(this.nextKeyEvent); | ||
expect(keyManager.activeItemIndex) | ||
|
@@ -416,6 +419,7 @@ describe('Key managers', () => { | |
const items = itemList.toArray(); | ||
items[2].disabled = true; | ||
itemList.reset(items); | ||
itemList.notifyOnChanges(); | ||
|
||
keyManager.onKeydown(this.nextKeyEvent); | ||
expect(keyManager.activeItemIndex) | ||
|
@@ -558,6 +562,7 @@ describe('Key managers', () => { | |
const items = itemList.toArray(); | ||
items[0].disabled = true; | ||
itemList.reset(items); | ||
itemList.notifyOnChanges(); | ||
|
||
keyManager.setFirstItemActive(); | ||
expect(keyManager.activeItemIndex) | ||
|
@@ -580,6 +585,7 @@ describe('Key managers', () => { | |
const items = itemList.toArray(); | ||
items[2].disabled = true; | ||
itemList.reset(items); | ||
itemList.notifyOnChanges(); | ||
|
||
keyManager.setLastItemActive(); | ||
expect(keyManager.activeItemIndex) | ||
|
@@ -602,6 +608,7 @@ describe('Key managers', () => { | |
const items = itemList.toArray(); | ||
items[1].disabled = true; | ||
itemList.reset(items); | ||
itemList.notifyOnChanges(); | ||
|
||
expect(keyManager.activeItemIndex) | ||
.withContext(`Expected first item of the list to be active.`) | ||
|
@@ -629,6 +636,7 @@ describe('Key managers', () => { | |
const items = itemList.toArray(); | ||
items[1].disabled = true; | ||
itemList.reset(items); | ||
itemList.notifyOnChanges(); | ||
|
||
keyManager.onKeydown(fakeKeyEvents.downArrow); | ||
keyManager.onKeydown(fakeKeyEvents.downArrow); | ||
|
@@ -706,6 +714,7 @@ describe('Key managers', () => { | |
const items = itemList.toArray(); | ||
items.forEach(item => (item.disabled = true)); | ||
itemList.reset(items); | ||
itemList.notifyOnChanges(); | ||
|
||
keyManager.onKeydown(fakeKeyEvents.downArrow); | ||
}); | ||
|
@@ -730,6 +739,7 @@ describe('Key managers', () => { | |
const items = itemList.toArray(); | ||
items[1].disabled = true; | ||
itemList.reset(items); | ||
itemList.notifyOnChanges(); | ||
|
||
expect(keyManager.activeItemIndex).toBe(0); | ||
|
||
|
@@ -744,6 +754,7 @@ describe('Key managers', () => { | |
const items = itemList.toArray(); | ||
items[1].skipItem = true; | ||
itemList.reset(items); | ||
itemList.notifyOnChanges(); | ||
|
||
expect(keyManager.activeItemIndex).toBe(0); | ||
|
||
|
@@ -839,6 +850,7 @@ describe('Key managers', () => { | |
new FakeFocusable('две'), | ||
new FakeFocusable('три'), | ||
]); | ||
itemList.notifyOnChanges(); | ||
|
||
const keyboardEvent = createKeyboardEvent('keydown', 68, 'д'); | ||
|
||
|
@@ -854,6 +866,7 @@ describe('Key managers', () => { | |
new FakeFocusable('321'), | ||
new FakeFocusable('`!?'), | ||
]); | ||
itemList.notifyOnChanges(); | ||
|
||
keyManager.onKeydown(createKeyboardEvent('keydown', 192, '`')); // types "`" | ||
tick(debounceInterval); | ||
|
@@ -874,6 +887,7 @@ describe('Key managers', () => { | |
const items = itemList.toArray(); | ||
items[0].disabled = true; | ||
itemList.reset(items); | ||
itemList.notifyOnChanges(); | ||
|
||
keyManager.onKeydown(createKeyboardEvent('keydown', 79, 'o')); // types "o" | ||
tick(debounceInterval); | ||
|
@@ -889,6 +903,7 @@ describe('Key managers', () => { | |
new FakeFocusable('Boromir'), | ||
new FakeFocusable('Aragorn'), | ||
]); | ||
itemList.notifyOnChanges(); | ||
|
||
keyManager.setActiveItem(1); | ||
keyManager.onKeydown(createKeyboardEvent('keydown', 66, 'b')); | ||
|
@@ -905,6 +920,7 @@ describe('Key managers', () => { | |
new FakeFocusable('Boromir'), | ||
new FakeFocusable('Aragorn'), | ||
]); | ||
itemList.notifyOnChanges(); | ||
|
||
keyManager.setActiveItem(3); | ||
keyManager.onKeydown(createKeyboardEvent('keydown', 66, 'b')); | ||
|
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?