mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.02k stars 1.24k forks source link

[TreeView] HTML is not valid #12285

Open vibl opened 3 years ago

vibl commented 3 years ago

The HTML generated by the TreeView component (such as can be seen in the doc examples: https://material-ui.com/components/tree-view/) is not valid:

Capture d’écran 2021-01-06 à 12 40 53

https://validator.w3.org/nu/?doc=https%3A%2F%2Fmaterial-ui.com%2Fcomponents%2Ftree-view%2F

Search keywords:

eps1lon commented 3 years ago

@joshwooding I think we can just use div instead of ul and li. We're overriding their semantics anyway and can potentially remove some CSS resets (e.g. list-style: none;).

joshwooding commented 3 years ago

@eps1lon Yeah that is true, we can probably just use div. @vibl Did you want to give it a go?

oliviertassinari commented 3 years ago

I have been tracking this problem in https://trello.com/c/R8CQU43K/779-better-collapse-implementation since mui/material-ui#14827. Back then, I noticed that https://validator.w3.org/ was reporting this issue.

The extra divs are coming from the Collapse component:

https://github.com/mui-org/material-ui/blob/1b24a942f93a07a7f2073942c6dec91bdd688d75/packages/material-ui/src/Collapse/Collapse.js#L244-L254

Do we really need 3 divs for implementing a collapse?

I have been benchmarking a bunch and this doesn't seem to be really needed:

By solving the Collapse, we should get a valid HTML structure with the TreeView for free.

vibl commented 3 years ago

Are you sure there isn't a way to do it with ul and li? Semantic markup is better for accessibility. And reducing the number of DDM elements might be better for performance, especially in a nested tree where the number of elements displayed can get huge.

As for giving it a go, I'm too busy at the moment, sorry.

eps1lon commented 3 years ago

Are you sure there isn't a way to do it with ul and li? Semantic markup is better for accessibility.

Not necessarily. Especially since we remove the semantics anyway in this instance.

eps1lon commented 3 years ago

By solving the Collapse, we should get a valid HTML structure with the TreeView for free.

But not guaranteed for custom TreeItem components, no?

vibl commented 3 years ago

Are you sure there isn't a way to do it with ul and li? Semantic markup is better for accessibility.

Not necessarily. Especially since we remove the semantics anyway in this instance.

We're not using the word "semantic" in the same sense here. By "semantic markup", I meant "using the HTML tags that mean what we want to express". Here, ul and li precisely mean what the TreeView wants to express (a tree of nested lists). Whether or not implementation details (such as bullets) are disabled is irrelevant in this regard.

eps1lon commented 3 years ago

Here, ul and li precisely mean what the TreeView wants to express (a tree of nested lists).

But a treeitem is not a listitem. I don't think we're using the word "precisely" in the same sense here.

Why is a <li role="treeitem" /> here better for a11y?

vibl commented 3 years ago

But a treeitem is not a listitem. I don't think we're using the word "precisely" in the same sense here.

I don't understand. How could a treeitem not also be a listitem? Isn't the TreeView a component that displays nested lists?

Why is a <li role="treeitem" /> here better for a11y?

Because it specifies that the element is a list item that is part of a tree.

Cf. https://www.w3.org/TR/wai-aria-practices-1.1/examples/treeview/treeview-2/treeview-2a.html

Maybe I'm missing something about what you want to say or do with the TreeView. If so, please tell me.

eps1lon commented 3 years ago

Because it specifies that the element is a list item that is part of a tree.

But assistive technology will omit that there's a list and instead treat it as a tree. As I said: The semantics are overridden. Just to be clear: This is all as far as I know which is why I'm asking you to provide a concrete behavior that <li role="treeitem" /> implements that <div role="treeitem" /> doesn't. Not what is "more semantic" (what ever that is supposed to mean) in some ontology.

How could a treeitem not also be a listitem? Isn't the TreeView a component that displays nested lists?

What I was talking about is what users with assistive technology get announced. You're correct that in the class hierarchy a treeitem is a listitem. But that doesn't change whether we do <li role="treeitem" /> or <div role="treeitem" /> since treeitem has listitem as its superclass.

vibl commented 3 years ago

But assistive technology will omit that there's a list and instead treat it as a tree. What I was talking about is what users with assistive technology get announced.

Ok. That makes sense.

My concern is that, because Material-UI can be used in many different contexts, we might not know how assistive technologies (which can be visual as well as auditory, and might not all follow the correct/best standards) will render/translate it. For all I know, there might exist apps that would just make ul/li more visually obvious for visually impaired people, without interpreting role="treeitem". But I don't know much about this in practice.

Ideally, an a11y expert in that field would review Material-UI components, and this would give it even more value from the endorsement and perception of "entreprise-readiness". But it probably wouldn't be free, unless some a11y advocates see this as a popular enough FOSS project to make it worthwhile to volunteer. There might be non-profits that could advise on who to reach for.

oliviertassinari commented 3 years ago

@vibl Regarding the a11y expert, we will likely come to it, maybe late 2021. What would be good to do before is: get v5 out, hire 2 more developers to have enough bandwidth, and get the second theme out. Even if we get quality reports, the bottleneck is likely on the implementation.

vibl commented 3 years ago

I didn't know you were hiring. I'm not available at the moment but I might be soon. I'll have a look at the job description.