[TreeItem] correct single-select aria-selected #20102
Conversation
There was a problem hiding this comment.
Have you looked at setting selected to false if the selection is disabled? It smells a bit that the internal selected state can be true if selection is disabled. Would be nice to have a consistent state so that we can have a 1:1 mapping between selected and aria-selected.
|
Agreed that is incorrect but that is another issue unrelated to aria-selected. Do you want me to correct that one in this pull request or create another ? |
I'm interested in the semantics of |
I would say that disableSelection means it ignores interactions.
If we agree that disableSelection only disables interactions : If selected then the class name should be present otherwise not - no change here. aria-selected from https://www.w3.org/TR/wai-aria-practices/#TreeView
above has no mention of selectability - seems clear that below is correct ( this is not the case with this pull request which was to ensure that aria-selected = false for single select was changed ). The selected class matches aria-selected. single select selected - aria-selected = true Your thoughts on interpretation of 'selectable' below
if we disable selection interactions does that mean that a node is not selectable even when it is selected ? Does c) trump a) in that case or do we look no further than a ? I would go for if selected then aria-selected true and if not selected then aria-selected = false regardless of disableSelection. This would agree with the selected class and the aria-selected logic needs to change. |
|
So |
eps1lon
added
the
component: tree view
|
Having looked again at aria practices and in particular https://www.w3.org/TR/wai-aria-1.1/#aria-disabled as wel as pull - disabled-prop I am beginning to think that disableSelection is a global 'this tree does not support selection'. ( There probably should be a prop on the TreeItem as well ) Perhaps @joshwooding can answer ? @netochaves @oliviertassinari your thoughts ? |
I think This means we make all the select handlers no-ops when |
Details of bundle changes.Comparing: 8b32809...7a735fe Details of page changes
|
|
@tonyhallett Much appreciated, thanks |
|
Although it does not affect this pull request I need to ask:
In what way is it a subset ? If we look at the definition of disabled https://www.w3.org/TR/wai-aria-1.1/#aria-disabled
By disabling the interactions that select / deselect - are we not making the tree item not editable ( cannot edit the selected state ) ? Shouldn't we be indicating that that interactions are disabled both visually and with aria ? The pull - [TreeView] disabled-prop does agree with your thinking that it is a subset as it prevents expansion in addition to selection ( but this is not consistent, as the keyboard can be used to toggle expansion. It does not apply aria-disabled either ) |
|
|
@eps1lon thought as much. So no need to apply aria-disabled if just selection disabled ? Different css ? Should it be possible to be more granular and disable selection on specific nodes ? |
By controlling The solution for const useTreeItemStyles = makeStyles({
// disabled selection
selected: {
background: 'red'
},
disableSelection: {}
})
function TreeItem(props) {
const classes = useTreeItemStyles(props)
return <MuiTreeItem {...props} classes={{...classes, selected: classes.selected}} />
} |
Ok, but given that the control provides this functionality if disabling all, shouldn't it do the same for individual tree items ? After all, disableSelection could have been managed with selected / onNodeSelect ? As per the styling do you want me to add disableSelection ? Without that we have to do https://codesandbox.io/s/wizardly-cache-cowsh What about aria though ? If disableSelection is a prop that can change how do you signal this change ? |
Mentioned in issue 20099