-
Notifications
You must be signed in to change notification settings - Fork 376
feat(TreeView): add support for disabled TreeViewListItems #12140
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
base: main
Are you sure you want to change the base?
Conversation
|
Preview: https://pf-react-pr-12140.surge.sh A11y report: https://pf-react-pr-12140-a11y.surge.sh |
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.
Some general comments after testing and tinkering in the preview:
- For the "Single selectable" tree view (where the entire node toggles expansion rather than selection, if it expands), the disabled state isn't immediately announced. I have to 1) go into the tree view, 2) navigate to the tree item, then 3) go further into that tree item (rather than navigating forward/backward) to get to the button inside, which then announes a disabled state. I'm not sure if users would typically do that.
- For this, what we could do instead of putting
disabledon the button, putaria-disabled="true"on therole="treeitem"element
- For this, what we could do instead of putting
- For the separate expand/select buttons from "With separate selection and expansion" example, sort of the same issue as above, except the other issue is that the navigation as noted above is basically required with VO for this implementation. I have to go deeper into a tree item in order to know that there are separate expand/collapse and select buttons (which, idk if that's really a great implementation in the first place). Via plain keyboard it works okay as I can go to the separate expand and select buttons, but for VO it's not as straight forward.
I'm not sure what the best approach would be off the top of my head. For the more basic tree view, applying aria-disabeld to the treeitem would be better than putting disabled on the button. For an implementation where expansion and selection are separate actions within a tree item, that may not work, but at the same time it may not be clear that you have to go further into the tree item for additional/separate actions. I might want to say that allowing this mixed state of "one action is disabled, but not the other" isn't advisable - either the entire tree item is disabled or it isn't. That would mean we could just apply aria-disabled to the treeitem and have it exposed as disabled far easier for users.
| /** Flag indicating if the tree view item is disabled. */ | ||
| isDisabled?: boolean; | ||
| /** Flag indicating if the tree view item toggle is disabled. */ | ||
| isToggleDisabled?: boolean; |
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.
Would we want isDisabled to disable everything (selection and expansion)? Might just be me, just that's what I expected to happen when passing isDisabled, but that only disabled selection for the example where the selection is a separate action; for the default behavior isDisabled works as expected since there's only one button.
Or would we want to have isToggleDisabled and isSelectionDisabled props instead?
|
TLDR of this comment: how we currently have our TreeView component setup, the way this PR goes about disabling stuff might be one of the better ways to handle it. The longer story: So I tried testing out with NVDA a bit as well and looking a little more, and at least based on whatever settings I have set in both NVDA and VO, the interaction experiences are fairly different. In NVDA I can use arrow keys alone to navigate the tree view items, i.e. arrow keys will essentially focus the button elements within each tree item (whether it be the entire button that expands, or separately the expand button and selectable button). VoiceOver from what I can tell I need to use VO keyboard shortcuts to navigate since it's being announced as a table, in which case my previous comment of it being difficult to tell that there are separate expand/select actions still seems to stand (and in that case, the disabled state may not get announced since VO focus i on the tree item, not the button element within the tree item. Though NVDA can get a bit complicated as well when navigating via their shortcuts; if I use NVDA navigation within the treeview and try to move browser/system focus to the treeitem that has NVDA focus, I still have to press the Tab key to place focus on the actual button element to interact properly. I can't be sure if this is how a user would actually interact/navigate in a treeview, though. As an addendum to one of my previous comments, making the FWIW, WCAG has this navigation tree view example that has a separate selection/expansion sort of implementation. Essentially, the caret for expansion isn't in a semantic element, it looks to rely solely on mouse clicking the icon to expand it without selection while keyboard users need to use left/right arrow keys to expand/collapse. Also their expected keyboard interactions can e found on their tree view pattern page. |
What: Closes #11893
Still needs some testing and definitely requires a styling update. Functionally the disabled props will work, but they will not look any different.