feat(Tree): integrate ARIA HTML design pattern #1488
Conversation
silviuaavram
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1488 +/- ##
==========================================
- Coverage 73.43% 73.22% -0.21%
==========================================
Files 822 822
Lines 6192 6193 +1
Branches 1778 1800 +22
==========================================
- Hits 4547 4535 -12
- Misses 1640 1653 +13
Partials 5 5
Continue to review full report at Codecov.
|
silviuaavram
added
accessibility
packages/react/src/lib/accessibility/Behaviors/Tree/treeBehavior.ts
Outdated
Show resolved
Hide resolved
packages/react/src/lib/accessibility/Behaviors/Tree/treeTitleBehavior.ts
Outdated
Show resolved
Hide resolved
| } | ||
| }, | ||
| setFocusToFirstChild: e => { | ||
| expandOrPassFocus: e => { |
There was a problem hiding this comment.
nit: expandOrPassFocusToSubtree - wouldn't be more meanigful?
There was a problem hiding this comment.
same sort of comment - as a client that is consuming this API, I don't understand what passFocus means. Would suggest shorter form of what @sophieH29 has suggested: expandOrFocusSubtree
|
|
||
| _.invoke(this.props, 'onTitleClick', e, this.props) | ||
| }, | ||
| collapseOrReceiveFocus: e => { |
There was a problem hiding this comment.
may be even shorter: collapseOrFocus
There was a problem hiding this comment.
general thoughts on current actionHandlers API. As a client, frankly, it is completely unobvious for me how I should respond on collapseOrFocus action - specifically, what drives my confusion is this or part. What is the principle based on which I should select one or the other? It turns out that it is based on open prop value - but how I would know that?
Ok, and then the question for me, as a client, is - I have provided all the props to accessibility behavior, so I might expect that it has access to all of them. Then, why it doesn't just expose collapse or focus actions, because I expect that inner implementation might just check open prop I've provided to it - and then I, as a client, won't be required to solve this puzzle on my side.
Really think that we should adjust implementation of action handlers a bit, to make this decision for the client (not this PR - just general thoughts). This will result in much more convenient API
There was a problem hiding this comment.
@jurokapsiar @sophieH29 @kolaps33 We can follow up.
There was a problem hiding this comment.
agree, action handlers can be extended to reflect this kind of situation - let's discuss how the API would look like
| @@ -0,0 +1,49 @@ | |||
| import { treeItemBehavior } from 'src/lib/accessibility' | |||
|
|
|||
| describe('TreeItemBehavior.ts', () => { | |||
There was a problem hiding this comment.
lets omit extension from the test name
| @@ -0,0 +1,49 @@ | |||
| import { treeItemBehavior } from 'src/lib/accessibility' | |||
There was a problem hiding this comment.
suggest to introduce e2e tests for Tree's focus behavior - will be glad to assist if necessary.
There was a problem hiding this comment.
already created task for that, thanks!
https://www.w3.org/TR/wai-aria-practices-1.1/examples/treeview/treeview-2/treeview-2b.html
Added ARIA attributes to tree, treeitem and titles depending on their role. Items that are leaves will get
treeitemrole on the<a>(and rolenoneon<li>) tag while expandable items will gettreeitemrole on the parent<li>.Focus is handled differently: if leaf, focus will be on the
<a>. If not, focus will be on the parent<li>.Adapted UTs to match new specification. Also behavior UTs.
Not possible: to add focus tests for Left-Right behavior. I cannot focus anything on the tree with Enzyme.
WIP: adjust the behavior prop types.