[TreeView] Add disabled prop #20133

netochaves · 2020-03-15T19:56:43Z

Closes #19975
This PR adds a new disabled prop to TreeItem

I have followed (at least) the PR section of the contributing guide.

mui-pr-bot · 2020-03-15T20:06:26Z

@material-ui/lab: parsed: +0.35% , gzip: +0.33%

Details of bundle changes

Generated by 🚫 dangerJS against 8cec22f

oliviertassinari

Thanks for working on this problem. I believe we can extend the reflection of #19967 to this case.

packages/material-ui-lab/src/TreeItem/TreeItem.js

netochaves · 2020-03-16T22:20:19Z

@oliviertassinari Sure, you want to set this as the default behavior or create a new prop as it was done in #19967? I believe that in this second case, it should be done in a separated PR

tonyhallett · 2020-03-19T08:33:44Z

You have not applied the aria-disabled attribute.

aria-disabled (state)§
Indicates that the element is perceivable but disabled, so it is not editable or otherwise operable. See related aria-hidden and aria-readonly.

For example, irrelevant options in a radio group may be disabled. Disabled elements might not receive focus from the tab order. For some disabled elements, applications might choose not to support navigation to descendants. In addition to setting the aria-disabled attribute, authors SHOULD change the appearance (grayed out, etc.) to indicate that the item has been disabled.

The state of being disabled applies to the current element and all focusable descendant elements of the element on which the aria-disabled attribute is applied.

You can still select with the keyboard.

Perhaps use context to pass disabled through for when disabling a parent node ?

eps1lon · 2020-03-27T18:25:47Z

The state of being disabled applies to the current element and all focusable descendant elements of the element on which the aria-disabled attribute is applied.

That's an important point that is currently not tested in this PR. @netochaves Would be nice if you could test behavior of child treeitems of a disabled treeitems.

You can still select with the keyboard.

This seems rather restrictive. Is this quoted from somewhere? We currently disable navigation in all our components. I think we can rethink this later similar to https://github.com/mui-org/material-ui/pull/19967/files

tonyhallett · 2020-03-27T18:51:45Z

You can still select with the keyboard.

This seems rather restrictive. Is this quoted from somewhere? We currently disable navigation in all our components. I think we can rethink this later similar to https://github.com/mui-org/material-ui/pull/19967/files

I was referring to https://github.com/mui-org/material-ui/blob/f1a2ec4ebab84d1ffe7b71e9e31904a44efc26c1/packages/material-ui-lab/src/TreeItem/TreeItem.js#L403

<div
        className={classes.content}
        onClick={!disabled ? handleClick : null}

This is the only place where behaviour changes due to the disabled prop being true ( all other changes are styling). This prevents focus by click, toggling expansion and selection. Yet, with the keyboard, you can focus, toggle expansion and select.

netochaves · 2020-03-28T16:06:33Z

That's an important point that is currently not tested in this PR. @netochaves Would be nice if you could test behavior of child treeitems of a disabled treeitems.

Should we disable the expansion of a parent treeItem when it is disabled?

eps1lon · 2020-03-28T20:39:53Z

Should we disable the expansion of a parent treeItem when it is disabled?

I think so. disabled usually disables interaction. Since expansion is triggered by interaction it should not be possible.

netochaves · 2020-04-25T17:37:11Z

Hey guys, there's still work to be done here? please let me know.

joshwooding · 2020-07-26T00:18:01Z

@netochaves Thanks for the head start. I've updated the PR with some more use cases and added some tests. Still need to add a demo with some wording and make sure the styling is working.

packages/material-ui-lab/src/TreeView/TreeView.js

joshwooding · 2020-07-26T00:19:59Z

packages/material-ui-lab/src/TreeView/TreeView.js

@@ -459,14 +479,12 @@ const TreeView = React.forwardRef(function TreeView(props, ref) {

  const selectRange = (event, nodes, stacked = false) => {
    const { start = lastSelectedNode.current, end, current } = nodes;
-    if (start != null && end != null) {


Was preventing arrow down + shift from working without an already selected node.

packages/material-ui-lab/src/TreeItem/TreeItem.js

joshwooding · 2020-07-26T00:46:23Z

Keeping disabled nodes focusable was easier (and more correct in my opinion)

oliviertassinari

I think that we should demonstrate this in the documentation. I'm not sure about using the first basic demo as it's not a frequent use case. Maybe we could have a new demo, just before the last a11y section?

joshwooding · 2020-07-26T10:54:53Z

@oliviertassinari I agree. I don’t think the first demo should be re-used either.

eps1lon · 2020-07-26T11:00:14Z

Keeping disabled nodes focusable was easier (and more correct in my opinion)

It's definitely an option but we never did this before. Just like selection follows focus we should make this configurable since you don't know if it makes more sense to keep a disabled item in tab order. The authoring practices encourage a consistent set of rules and this would go against it.

Authors are encouraged to adopt a consistent set of conventions for the focusability of disabled elements.

-- https://www.w3.org/TR/wai-aria-practices-1.1/#kbd_focus_activedescendant

We can definitely rethink our current set of rules but breaking them because it's easier for us is not really a helpful rationale.

If it's hard to get right in a first PR we should definitely add a warning for now. But if we don't have a viable plan to make it work later we should rethink this feature. It seems to me that the path of least resistance is chosen and justified after the fact.

packages/material-ui-lab/src/TreeItem/TreeItem.test.js

…less specific

stale

eps1lon

editorial suggestions
did you take a look at [TreeView] Add disabled prop #20133 (comment)?
Reverted the changes that introduced createStyles where it is not necessary

docs/src/pages/components/tree-view/DisabledTreeItems.js

docs/src/pages/components/tree-view/DisabledTreeItems.tsx

docs/src/pages/components/tree-view/DisabledTreeItems.js

Great catches! Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>

stale

joshwooding · 2020-08-01T16:01:42Z

NVDA logs for when you can focus disabled items.

joshwooding · 2020-08-01T16:02:37Z

Thanks for the head start @netochaves

github-actions bot mentioned this pull request Mar 15, 2020

Update snapshots for pr/20133 eps1lon/mui-scripts-incubator#68

Closed

oliviertassinari added the component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! label Mar 15, 2020

oliviertassinari reviewed Mar 15, 2020

View reviewed changes

packages/material-ui-lab/src/TreeItem/TreeItem.js Outdated Show resolved Hide resolved

tonyhallett mentioned this pull request Mar 22, 2020

[TreeItem] correct single-select aria-selected #20102

Merged

1 task

eps1lon added the new feature New feature or request label Mar 27, 2020

eps1lon changed the title ~~[TreeView] disabled-prop~~ [TreeView] Add disabled prop Mar 27, 2020

netochaves force-pushed the TreeView/disabled-prop branch 2 times, most recently from a39f870 to a648641 Compare April 5, 2020 19:41

joshwooding force-pushed the TreeView/disabled-prop branch from a648641 to 7ea7461 Compare July 26, 2020 00:13

netochaves requested a review from dimitropoulos as a code owner July 26, 2020 00:13

joshwooding removed the request for review from dimitropoulos July 26, 2020 00:14

joshwooding changed the base branch from master to next July 26, 2020 00:16

joshwooding reviewed Jul 26, 2020

View reviewed changes

packages/material-ui-lab/src/TreeView/TreeView.js Show resolved Hide resolved

joshwooding reviewed Jul 26, 2020

View reviewed changes

oliviertassinari reviewed Jul 26, 2020

View reviewed changes

packages/material-ui-lab/src/TreeItem/TreeItem.js Outdated Show resolved Hide resolved

oliviertassinari approved these changes Jul 26, 2020

View reviewed changes

github-actions bot mentioned this pull request Jul 26, 2020

Update snapshots for pr/20133 eps1lon/mui-scripts-incubator#617

Closed

joshwooding requested a review from eps1lon July 26, 2020 15:44

eps1lon reviewed Jul 28, 2020

View reviewed changes

packages/material-ui-lab/src/TreeItem/TreeItem.test.js Outdated Show resolved Hide resolved

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 29, 2020

netochaves and others added 13 commits July 29, 2020 21:45

feat(TreeView):disabled-prop

8bb5d35

feat: add aria-disabled and logic to disable childs

16c9c5a

Update branch, add more use cases and add tests

dacb665

api docs

4886db6

fireEvent in act was causing test to fail

d1da63d

polish

9802e48

Fix items being focusable by type-ahead, add disabledItemsFocusable

cbe8a15

Remove hover styling from disabled items

089d760

Add disabled tree item demo

775cf1c

prettier

4e8c366

Prevent programmatic focus, add more tests and make disabled styling …

6e41bd5

…less specific

Seb feedback

6b0394a

focusIn -> focus

4749555

joshwooding force-pushed the TreeView/disabled-prop branch from 7c92e97 to 4749555 Compare July 29, 2020 20:46

joshwooding requested a review from eps1lon July 29, 2020 20:46

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 29, 2020

Remove unnecessary createStyles

8c636fa

eps1lon previously requested changes Jul 30, 2020

View reviewed changes

Apply suggestions from Sebastian

ffee36b

Great catches! Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>

oliviertassinari approved these changes Jul 30, 2020

View reviewed changes

Remove mutation

8cec22f

joshwooding requested a review from eps1lon July 30, 2020 20:22

joshwooding merged commit 08d7334 into mui:next Aug 1, 2020

kodai3 pushed a commit to kodai3/material-ui that referenced this pull request Aug 3, 2020

[TreeView] Add disabled prop (mui#20133)

8b2458b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TreeView] Add disabled prop #20133

[TreeView] Add disabled prop #20133

netochaves commented Mar 15, 2020

mui-pr-bot commented Mar 15, 2020 •

edited

Loading

oliviertassinari left a comment

netochaves commented Mar 16, 2020

tonyhallett commented Mar 19, 2020

eps1lon commented Mar 27, 2020

tonyhallett commented Mar 27, 2020

netochaves commented Mar 28, 2020

eps1lon commented Mar 28, 2020

netochaves commented Apr 25, 2020

joshwooding commented Jul 26, 2020

joshwooding Jul 26, 2020

joshwooding commented Jul 26, 2020

oliviertassinari left a comment

joshwooding commented Jul 26, 2020

eps1lon commented Jul 26, 2020

eps1lon left a comment •

edited

Loading

joshwooding commented Aug 1, 2020

joshwooding commented Aug 1, 2020 •

edited

Loading

[TreeView] Add disabled prop #20133

[TreeView] Add disabled prop #20133

Conversation

netochaves commented Mar 15, 2020

mui-pr-bot commented Mar 15, 2020 • edited Loading

oliviertassinari left a comment

Choose a reason for hiding this comment

netochaves commented Mar 16, 2020

tonyhallett commented Mar 19, 2020

eps1lon commented Mar 27, 2020

tonyhallett commented Mar 27, 2020

netochaves commented Mar 28, 2020

eps1lon commented Mar 28, 2020

netochaves commented Apr 25, 2020

joshwooding commented Jul 26, 2020

joshwooding Jul 26, 2020

Choose a reason for hiding this comment

joshwooding commented Jul 26, 2020

oliviertassinari left a comment

Choose a reason for hiding this comment

joshwooding commented Jul 26, 2020

eps1lon commented Jul 26, 2020

eps1lon left a comment • edited Loading

Choose a reason for hiding this comment

joshwooding commented Aug 1, 2020

joshwooding commented Aug 1, 2020 • edited Loading

mui-pr-bot commented Mar 15, 2020 •

edited

Loading

eps1lon left a comment •

edited

Loading

joshwooding commented Aug 1, 2020 •

edited

Loading