[ListItem] Make semantically correct

[mbrookes]

This is a discussion draft. I have only changed one demo so far, and it's still not perfect. The changes are:

• Significantly simplify ListItem internals.

• Use semantically correct lists to allow screen readers to enumerate the items.

• Use role="navigation" on the nav:
  https://www.w3.org/WAI/tutorials/page-structure/regions/#accessupport

• Use <li role="separator"> for the Divider. (Divider already adds this when component="li". Updated demos to be compliant.)

Also, closes #15482

[oliviertassinari]

@mbrookes For context, I have fought hard in the past with the list page to make https://validator.w3.org pass. I'm happy if we can simplify the logic.

[mbrookes]

Sorry, I meant to make this a draft PR.

[mbrookes]

It seems it doesn't like role="button" on <li>, which I can sort of understand, but then it also doesn't like role="navigation" on <nav>, which is recommended here: w3.org/WAI/tutorials/page-structure/regions/#accessupport, so who knows!

We could resolve the former by making ButtonBase a child of <li>. Only w3.org can resolve the latter.

[oliviertassinari]

Use semantically correct lists to allow screen readers to enumerate the items.

Should they be listed as items? We are displaying buttons or links in the example.

However, to maximize compatibility with web browsers and assistive technologies that support WAI-ARIA but do not yet support HTML5

As far as I know, the browsers we support have full support for HTML5. Can we skip the usage of <nav role="navigation">? https://www.w3.org/WAI/tutorials/page-structure/regions/#accessupport

[eps1lon]

I'd be so glad to get rid of the implicit Button. Just create a ListItemButton instead that nests li > button properly.

<div role="listitem button" /> would work as well. <li role="button" /> is probably bad because it removes the listitem role?

[mbrookes]

@oliviertassinari

However, to maximize compatibility with web browsers and assistive technologies

I'm kicking myself that I didn't bookmark it, but one article I referred to described how each of the major screen readers interprets <nav>. It too recommended <nav role="navigation">

@eps1lon

I'd be so glad to get rid of the implicit Button.

💯

Just create a ListItemButton instead that nests li > button properly

Separate ListItemButton component, or correctly nests ButtonBase for the button prop?

<div role="listitem button" /> would work as well.

li > button seems to be the preferred option from what I've been reading. The suggestion seems to be that role="listitem" is for "fixing" components that are semantically incorrect, however we are in the position to be correct.

<li role="button" /> is probably bad because it removes the listitem role?

Makes sense.

Me:

the divider isn't really an item in the list

The answer is: <li role="separator"> <- corrected.

[eps1lon]

Just create a ListItemButton instead that nests li > button properly

Separate ListItemButton component, or correctly nests ButtonBase for the button prop?

Create the separate ListItemButton.

• ListItem is already bloated and every time I'll look into it I need 5 minutes to understand what's actually rendered
• complicated API: one has to be very careful about valid DOM nesting (or what prop controls what) depending on the prop combination. Reacts composition model already has a good solution. People will still complain "Why cant this just be a prop? I don't want to write that much." but we want to optimize for change not for the initial write.
• ListItem is poorly analyzed statically
  • If you never need the button you always end up pulling in ButtonBase (fairly large).
    This is probably not an issue since ButtonBase will end up in your bundle anyway but it still looks bad to create such a big bundle for just a list
  • types of that component require a lot of time to get right
• It's consistent with the other ListItem helpers (e.g. ListItemText instead of <ListItem text />

I'm kicking myself that I didn't bookmark it, but one article I referred to described how each of the major screen readers interprets

. It too recommended

Well I don't mind adding the redundancy here. It's just an attribute and not multiple lines of code. But if this is an actual issue we should investigate the other html5 elements we use.

[oliviertassinari]

Create the separate ListItemButton.

Would it make thing simpler? Looking at the source, I believe that most of the complexity is in the secondary action logic. It's kind of a mess. We should optimize for the user experience. I don't think that the button property is that much an issue on its own. Alternatively, we could have people do <ListItem component={ButtonBase} /> or <ListItem><ButtonBase /></ListItem>.

For the secondary action problem, what do you think of this different API?

    <List className={classes.root}>
      {[0, 1, 2, 3].map(value => (
        <ListItemSecondaryAction action={
          <IconButton edge="end" aria-label="Comments">
            <CommentIcon />
          </IconButton>
        }>
          <ListItem key={value} role={undefined} dense button onClick={handleToggle(value)}>
            <ListItemIcon>
              <Checkbox
                edge="start"
                checked={checked.indexOf(value) !== -1}
                tabIndex={-1}
                disableRipple
              />
            </ListItemIcon>
            <ListItemText primary={`Line item ${value + 1}`} />
          </ListItem>
        </ListItemSecondaryAction>
      ))}
    </List>

[oliviertassinari]

Funny enough, we are using the ListItem > Button strategy for the left side list menu of the documentation.

[eps1lon]

Create the separate ListItemButton.

Would it make thing simpler?****

I haven't looked into it. But even if it's just a thin wrapper I rather let people use <ListItemButton /> instead of <ButtonBase />. The less we expose base classes (erm components) the better.

[mbrookes]

I created a simple ListItemButton, which results in:

However nesting a ListItemButton inside ListItem of course results in this when hovered:

...since the ListItem padding is set up for children such as ListItemIcon, ListItemText.

[eps1lon]

If we nest the button can we also use the native button instead of div + manual aria attributes?

[mbrookes]

@eps1lon Done.

I have also cleared out much of the cruft that had accumulated in ListItem. There was code to catch issues caused by code to... (you get the idea).

I suspect many other components may have similar bloat...

Anyway, while there is a slight tradeoff when providing props to ButtonBase (see SimpleList.js demo), the API is simpler overall, and the ListItem code much simpler.

I have not attempted to align the tests or type definitions yet, pending approval of the general approach.

[mbrookes]

@eps1lon

If we nest the button can we also use the native button instead of div + manual aria attributes?

The issue is that you can't have block elements as the children of button. <ButtonBase component="div"> -> <div role="button"> seems the simplest solution.

[eps1lon]

The issue is that you can't have block elements as the children of button.

I meant li > button instead if li > div[role="button"]

[mbrookes]

I meant li > button instead if li > div[role="button"]

That's what it is currently, as of this commit c3e1986 per your comment here: #15333 (comment)

However as I said here: #15333 (comment), the issue with li > button is that you can't have block elements as the children of button: li > button > div.

<ButtonBase component="div"> (<div role="button">) seems the simplest solution, rather than trying to coerce all the children to be inline.

[eps1lon]

However as I said here: #15333 (comment), the issue with li > button is that you can't have block elements as the children of button

That was my question: Why do you need block content? But I guess we need it for the other ListItem* components. Which leads to the question: Should we use rather use inline elements for the ListItem* components?

[mbrookes]

That was my question: Why do you need block content?

When did you ask that question? What you said was (not a question): "I meant li > button instead if (sic) li > div[role="button"]" #15333 (comment).

Not only was this already implemented (in response to your prior suggestion), it's the very reason block content becomes a problem. This is what I was pointing out (twice).

Why do you need block content?

Because some children (Avatar, MuiListItemSecondaryAction) are not inline.

Should we use rather use inline elements for the ListItem* components?

Possibly. Some of them already are inline, but not sure how we'd deal with Avatar, short of creating a wrapper component. This is why I said (also twice): "<ButtonBase component="div"> (<div role="button">) seems the simplest solution, rather than trying to coerce all the children to be inline.". We also can't predict what developers might include as ListItem children, but perhaps that's an acceptable trade-off.

[oliviertassinari]

@mbrookes I would propose that we come back to this problem when we start working on v5. From what I understand, it requires a breaking change.