[List] Improve ListItemSecondaryAction handling #13597

tregusti · 2018-11-14T14:37:13Z

This is not a v0.x issue.
I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

ListItem should consistently either render a <li> or not render a <li>

Current Behavior

When adding a ListItemSecondaryAction into the ListItem a <li> is automatically added. If I render a list where some items should have an icon and some do not, this element structure becomes inconsistent.

Steps to Reproduce

Link: https://codesandbox.io/s/20w105myyy

Open Browser DevTools and inspect the DOM structure or the children to the <ul>s.

Context

I'm trying to make a generic ListItemLink helper that wraps the content inside a ListItem and a NavLink like this:

class ListItemLink extends Component {
  render() {
    const { classes, to, children } = this.props;

    return (
      <ListItem button component={NavLink} to={to} activeClassName={classes.active}>
        {children}
      </ListItem>
    );
  }
}

But since I use component property I override the default use of <li>, however, when adding the ListItemSecondaryAction inside of it a <li> is added. I assume that this is somehow related to #10452, but cannot understand how to fix it. Is this a bug? If not, how can I work aroun this issue when I do not know beforehand if the content will contain a ListItemSecondaryAction?

Your Environment

Tech	Version
Material-UI	v3.4.0
React	v16.6
React Router	4.3.1

The text was updated successfully, but these errors were encountered:

oliviertassinari · 2018-11-16T07:38:12Z

@tregusti Your HTML5 semantic isn't valid in your example. It should be ul > li > a.

        </ListItem>
+       <li>
          <ListItem button component={NavLink} to="/dfgafdg">
            <ListItemText primary="I do not have a li" />
          </ListItem>
+       </li>
      </List>

We are using the following logic on Material-UI side to determine the existance of a secondary action:

const children = React.Children.toArray(childrenProp);
const hasSecondaryAction =
  children.length &&
  isMuiElement(children[children.length - 1], ['ListItemSecondaryAction']);

You can do the same.

tregusti · 2018-11-16T08:06:06Z

That is exactly my problem, hence the reason I opened the issue...

If I loop a bunch of items that will contain the information that renders the links. Sometimes I should manually add the <li> and sometimes not, and that is my problem.

As I see it, ListItem should either always add a <li> or never do it. "Sometimes" it not really something that is easy to handle as a consumer of an API.

It the following code, the generated markup will become non-semantic, since <li> will sometimes be present and sometimes not.

<nav>
	<List>
		{ items.map(item => (
			<ListItem ...>
				{/*
					content based of `item`.
					sometimes with and sometimes without secondary action
				*/}
			</ListItem>
		))}
	</List>
</nav>

If what you suggest is how to do it, then I should take into account how internal code in material-ui looks/works instead of assuming it is consistent. So if this is not a bug, then I would either like a property available telling me if a <li> will be automatically added or not so that I can add it myself. I should NOT check for the presence of ListItemSecondaryAction because that is how the internal code in the lib currently works.

So please reconsider closing this. Or at least let some mor eyes see it before closing it.

eps1lon · 2018-11-16T08:44:58Z

TL;DR: I agree the code is bloated but that markup is weird in the first place. It equates to buttons inside buttons.

I had the same issue when porting the context. It his very hard to reason about a component if that component changes the markup depending on the immediate children. It is very close to breaking component encapsulation and doesn't even handle cases where one would wrap ListItemSecondaryAction in a headless component.

That being said I find the component itself an example where we trade reasonable code for writing shorthand code that does not make sense semantically in the end.

What should've happened is a separate ListItemButton that doesn't allow anything else than text as a child. It doesn't make much sense to have a <ListItem button /> that also has a secondary action. Buttons inside Buttons seems like a very bad idea. That is also closer to the W3 spec that states that elements should not change their roles over time (which we basically enable with a button prop).

PS: I'm totally aware that I might miss many use cases here and look at this from a very simplified perspective but I believe that I rather have a many one or two prop components than a 10 prop component that allows for nonsensical combinations of those.

oliviertassinari · 2018-11-16T10:59:31Z

That is exactly my problem, hence the reason I opened the issue...

I'm raising the issue because your demo states that ul > li > a is wrong and that ul > a is correct. It's the oppossite.

Sometimes I should manually add the
and sometimes not, and that is my problem.

There is an intrinsic complexity with nesting buttons and links. Of course. We could make the API more explicit as @eps1lon is suggesting but it won't solve your problem, you still have to code branch the two cases. The upside of @eps1lon's approach is that it's explicit. No surprises, this is great. I have spend hours into try to get the current button nesting logic "right". I understand your frustration, it's a pain to get it right!

So, going forward:

You can add a property to your custom list item to know when to add a li and when not to.
We can look into increasing the API surface of the list item, to make it closer to the underlying DOM node. I has a great potential. However, at this point, I don't think that it will better solve your problem, you have already been hit by the need of code branch.

tregusti · 2018-11-16T12:41:16Z

First off, I really do appreciate all the time you put into the library!

So, maybe my demo isn't as clear as I wanted it. I'm not advocating ul > a. At least that was not my intent.

So, my real problem is when using <ListItem component={NavLink}> I assume that I tell ListItem to NOT render a <li> since I explicitly want a NavLink instead. And then of course I have to manually add the <li> myself, that it fine. What is not fine is that I get another nested <li> when adding the ListItemSecondaryAction into my content of the ListItem.

I updated the demo with a new list in the end, clarifying the problem (I hope).

https://codesandbox.io/s/20w105myyy

For now however, I will go forward as you suggested in your comment.

Note to self, do not copy/paste a demo for something else, start from scratch =)

ryancogswell · 2018-12-29T20:20:59Z

@tregusti When a ListItemSecondaryAction is in the ListItem, the component gets wrapped by the ContainerComponent. This defaults to li, but it is another property that you can control. So, for instance, you could specify ContainerComponent to be div instead of li (e.g. <ListItem component={NavLink} ContainerComponent="div">) and then know that you can safely wrap all your list items in li without resulting in nested lis.

You can specify this ContainerComponent unconditionally (without needing to know whether or not your ListItem contains a ListItemSecondaryAction) because it will be ignored except in the case where a ListItemSecondaryAction is detected.

tregusti · 2018-12-29T20:46:48Z

Thank you so much @ryancogswell. This was what I was after (next to a more consistent behaviour =)

I will try this out!

ryancogswell · 2018-12-29T22:09:23Z

@tregusti Another option to get more consistent markup across your list items would be to always include ListItemSecondaryAction and just have it be empty when it isn't applicable. This will have the effect of leaving a space at the end where an icon would be (due to a paddingRight: 32 that is added when the ListItem has a ListItemSecondaryAction) which, depending on what is in the list items, may be a desirable look.

Here's an example of this (via modifying a CodeSandbox from another ListItem issue):

ilovett · 2019-08-14T17:26:57Z

For me I was just trying to use the list item styling without being in a List

The following will correctly render div as I want with component="div"

<ListItem component="div" disableGutters>
  <ListItemAvatar>
    <Avatar>{ avatarContent }</Avatar>
  </ListItemAvatar>
  <ListItemText
    primary="ListItem is a div"
  />
</ListItem>

As soon as I have a ListItemSecondaryAction child, it renders as li and ignored component="div"

<ListItem component="div" disableGutters>
  <ListItemAvatar>
    <Avatar>1</Avatar>
  </ListItemAvatar>
  <ListItemText
    primary="ListItem is a li"
  />
  <ListItemSecondaryAction>
    <IconButton edge="end">
      <OpenInNewIcon />
    </IconButton>
  </ListItemSecondaryAction>
</ListItem>

eps1lon · 2020-06-11T09:10:12Z

I think we need to revisit this for v5. The current situation with regards to navs is pretty bad (our "simple" demo is not how you'd mark up a navigation UI). It's too easy to get wrong e.g.

<nav>
  <List>
    <ListItem button component={Link} href="/">
      Foo
    </ListItem>
  </List>
</nav>

creates invalid and wrong markup.

<nav>
  <List>
+   <li>
      <ListItem button component={Link} href="/">
        Foo
      </ListItem>
+   </li>
  </List>
</nav>

is unintuitive (li is a shorthand for listitem; do I have a button or a link?).

It's probably best always add an li and then nest a dedicated ListItemLink e.g.

<nav>
  <List>
    <ListItem>
      <ListItemButton component={NavLink}>
        Foo
      </ListItemButton>
      <ListItemSecondaryAction>
        Bar
      </ListItemSecondaryAction>
    </ListItem>
  </List>
</nav>

This still has the button vs link problem but at least that's (for better or worse) well established in Material-UI. The upside is that this naturally creates the correct UI (nav > ul > li > a) and nested buttons are discouraged.

oliviertassinari · 2020-06-11T10:35:31Z

Agree, I think that abstracting two DOM nodes in the initial design was too ambitious, in this context.

m4theushw · 2021-05-08T02:42:30Z

I think ListItem should always render a li, changing the component based on the child may cause surprises. Based on #13597 (comment) I can think of two solutions:

Option 1

Add a new ListItemContent component, that should be the only child of ListItem.
Move the button prop to ListItemContent.
If the user wants to render a link, the component prop of ListItemContent can be used.

<ListItem>
  <ListItemContent button component={NavLink}>
    Foo
    <ListItemSecondaryAction>
      Bar
    </ListItemSecondaryAction>
  </ListItemContent>
</ListItem>

Option 2

If button is true, we render a Button as the child of ListItem.
The initial children of ListItem becomes the children of Button.
Add a ButtonComponent prop to allow to change the root node of Button.
If the user wants to render a link, the ButtonComponent prop can be used.

<ListItem button ButtonComponent={NavLink}>
  Foo
  <ListItemSecondaryAction>
    Bar
  </ListItemSecondaryAction>
</ListItem>

eps1lon · 2021-05-08T08:36:10Z

<ListItem>
  <ListItemContent button component={NavLink}>
    Foo
    <ListItemSecondaryAction>
      Bar
    </ListItemSecondaryAction>
  </ListItemContent>
</ListItem>

This is highly problematic UI since you nest a button within a button. With a new API you shouldn't be able to create this kind of markup or rather it should be harder to do.

What happens with <ListItem ButtonComponent={NavLink}>?

m4theushw · 2021-05-08T14:31:08Z

This is highly problematic UI since you nest a button within a button. With a new API you shouldn't be able to create this kind of markup or rather it should be harder to do.

In terms of accessibility, it's completely wrong. However, Material Design allows it. How can we make this UI harder to do and what to say to users migrating from v4?

What happens with <ListItem ButtonComponent={NavLink}>?

Since button is false, the custom component would not be rendered. In my examples, I forgot to add a ButtonProps prop to pass props to the Button component.

eps1lon · 2021-05-10T09:37:08Z

Since button is false, the custom component would not be rendered.

That's the current behavior. Do you think that this is expected behavior by library users?

eps1lon · 2021-05-10T09:40:05Z

In terms of accessibility, it's completely wrong. However, Material Design allows it.

The linked page does not include the secondary action in tab order. They probably expect that the author provides other affordances to activate secondary actions. Our demos nests buttons that are both in tab order expecting screen readers to patch broken markup.

oliviertassinari · 2021-05-16T10:04:32Z

@m4theushw None of these two options seems to be ones we can follow, based on what Sebastian raised.
I would extend our options to these new ones:

Option 3: we do nothing. One could argue that this issue got little to no traction in 3 years, that the button + secondary action is a hedge case.
Option 4: we create a new ListItemButton component, we use it two keep the component flat (one React component always equals one DOM element), it covers 3 use cases:

nav > button

<List component="nav">
  <ListItem button>
    <ListItemText primary="Inbox" />
  </ListItem>
</List>

ul > li

<List>
  <ListItem>
    <ListItemText primary="Photos" secondary="Jan 9, 2014" />
  </ListItem>
</List>

ul > li > a + button

<List>
  <ListItem>
    <ListItemButton href="/a">
      Foo
    </ListItemButton>
    <ListItemSecondaryAction>
      Bar
    </ListItemSecondaryAction>
  </ListItem>
</List>

m4theushw · 2021-05-18T23:07:50Z

@oliviertassinari I was thinking about not allowing nav > button. The ListItem should always render a li. It makes more sense given the name of the component. If the user wants to change the root element, the component prop can be used, not button.

With the button prop, we have to check if someone tries to do the following:

<List>
  <ListItem button>
    <ListItemButton href="/a">
      Foo
    </ListItemButton>
    <ListItemSecondaryAction>
      Bar
    </ListItemSecondaryAction>
  </ListItem>
</List>

oliviertassinari · 2021-05-22T11:22:33Z

I was thinking about not allowing nav > button

@m4theushw Yeah, ok, sounds good. It seems that a more idiomatic structure would be: nav > ul> li > button. In this case,
I would propose a new Option 5:

List always render ul/ol, ul by default.
ListItem always render il
A list of buttons/links: (nav > ul > li > button > span)

<nav>
  <List>
    <ListItem>
      <ListItemButton>
        <ListItemText primary="Inbox" />
      </ListItemButton>
    </ListItem>
  </List>
</nav>

A simple list: (ul > li > span)

<List>
  <ListItem>
    <ListItemText primary="Inbox" />
  </ListItem>
</List>

A nested list of buttons/links (ul > li > a + button):

<List>
  <ListItem>
    <ListItemButton href="/a">
      Foo
    </ListItemButton>
    <ListItemSecondaryAction>
      Bar
    </ListItemSecondaryAction>
  </ListItem>
</List>

We add a variant prop to the List component, to support multiple use cases:
1. An unordered list, variant="unordered"
1. Anordered list, variant="ordered"
1. A basic list, variant="base", behave like Stack
1. A standard list, variant="standard", behave like in MD
2. A navigation list, variant="nav, behave like in our documentation side nav, or https://material.io/components/navigation-drawer#anatomy. But it might be too ambitious/bloated and it should have a dedicated component: Implement DrawerMenuItem #15107. Related to [Menu] Remove MenuItem dependency on ListItem #21587.

@eps1lon what do you think?

eps1lon · 2021-05-24T10:23:18Z

This looks good except two things:

variants should be different components not props
why do we need this nesting and not just <List><ListItemButton /><ListItemText /></List>?

oliviertassinari · 2021-05-24T10:32:39Z

variants should be different components not props

@eps1lon Do you mean, something in this order? Sounds fair

<List variant="ordered"> -> <OrderedList>
<List variant="unordered"> -> <UnorderedList>
<List>
<List variant="nav"> -> <NavList>

why do we need this nesting and not just <List><ListItemButton /><ListItemText /></List>?

What would be the underlying DOM structure of these components? Would it be List: ul and ListItemButton: li > a? (ListItemButton can host links)

oliviertassinari added the component: list This is the name of the generic UI component, not the React module! label Nov 16, 2018

oliviertassinari closed this as completed Nov 16, 2018

oliviertassinari added the discussion label Nov 16, 2018

oliviertassinari reopened this Nov 16, 2018

oliviertassinari added new feature New feature or request breaking change and removed discussion labels Nov 16, 2018

fastfedora mentioned this issue Dec 15, 2018

Improve documentation for ListItem and ListItemSecondaryAction #13916

Closed

2 tasks

oliviertassinari changed the title ~~Having a ListItemSecondaryAction inside the ListItem adds a <li>~~ [List] Improve ListItemSecondaryAction handling Aug 20, 2019

oliviertassinari mentioned this issue Aug 20, 2019

[ListItem] ListItemSecondaryAction onClick event has bubbling effect #17071

Closed

This was referenced Oct 31, 2019

Is there a reason ListItemSecondaryAction is absolutley positioned? #13495

Closed

CSS optimization MuiListItem MuiListSecondaryAction #18244

Closed

eps1lon mentioned this issue Jun 23, 2020

Bump eslint-plugin-jsx-a11y from 6.2.3 to 6.3.1 #21525

Merged

oliviertassinari mentioned this issue Jul 8, 2020

[ListItem] Using button prop forces an invalid nesting of list items #21712

Closed

2 tasks

oliviertassinari added this to the v5 milestone Jul 8, 2020

oliviertassinari mentioned this issue Jul 9, 2020

[ListItem] Secondary action is positioned with gutter when gutters are disabled #21722

Closed

2 tasks

oliviertassinari mentioned this issue Nov 2, 2020

[core] Use boolean type for ListItem prop instead of 'false' #23338

Closed

1 task

oliviertassinari mentioned this issue Jan 27, 2021

[RFC] Material-UI v5 🚀 #20012

Closed

mnajdova mentioned this issue Apr 15, 2021

[List] Add support for list item container style overrides #25777

Merged

This was referenced May 25, 2021

[RFC] ListItem improvement #26442

Closed

[List] extract button from ListItem to ListItemButton #26446

Merged

[List] extract button from ListItem to ListItemButton #26480

Closed

eps1lon closed this as completed in #26446 Jun 7, 2021

dctalbot mentioned this issue Jun 14, 2021

ui/users: fix secondary action rendering in safari target/goalert#1624

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[List] Improve ListItemSecondaryAction handling #13597

[List] Improve ListItemSecondaryAction handling #13597

tregusti commented Nov 14, 2018

oliviertassinari commented Nov 16, 2018 •

edited

Loading

tregusti commented Nov 16, 2018

eps1lon commented Nov 16, 2018

oliviertassinari commented Nov 16, 2018

tregusti commented Nov 16, 2018 •

edited

Loading

ryancogswell commented Dec 29, 2018

tregusti commented Dec 29, 2018

ryancogswell commented Dec 29, 2018

ilovett commented Aug 14, 2019

eps1lon commented Jun 11, 2020

oliviertassinari commented Jun 11, 2020

m4theushw commented May 8, 2021 •

edited by oliviertassinari

Loading

eps1lon commented May 8, 2021 •

edited

Loading

m4theushw commented May 8, 2021

eps1lon commented May 10, 2021

eps1lon commented May 10, 2021

oliviertassinari commented May 16, 2021 •

edited

Loading

m4theushw commented May 18, 2021

oliviertassinari commented May 22, 2021 •

edited

Loading

eps1lon commented May 24, 2021

oliviertassinari commented May 24, 2021 •

edited

Loading

[List] Improve ListItemSecondaryAction handling #13597

[List] Improve ListItemSecondaryAction handling #13597

Comments

tregusti commented Nov 14, 2018

Expected Behavior

Current Behavior

Steps to Reproduce

Context

Your Environment

oliviertassinari commented Nov 16, 2018 • edited Loading

tregusti commented Nov 16, 2018

eps1lon commented Nov 16, 2018

oliviertassinari commented Nov 16, 2018

tregusti commented Nov 16, 2018 • edited Loading

ryancogswell commented Dec 29, 2018

tregusti commented Dec 29, 2018

ryancogswell commented Dec 29, 2018

ilovett commented Aug 14, 2019

eps1lon commented Jun 11, 2020

oliviertassinari commented Jun 11, 2020

m4theushw commented May 8, 2021 • edited by oliviertassinari Loading

Option 1

Option 2

eps1lon commented May 8, 2021 • edited Loading

m4theushw commented May 8, 2021

eps1lon commented May 10, 2021

eps1lon commented May 10, 2021

oliviertassinari commented May 16, 2021 • edited Loading

m4theushw commented May 18, 2021

oliviertassinari commented May 22, 2021 • edited Loading

eps1lon commented May 24, 2021

oliviertassinari commented May 24, 2021 • edited Loading

oliviertassinari commented Nov 16, 2018 •

edited

Loading

tregusti commented Nov 16, 2018 •

edited

Loading

m4theushw commented May 8, 2021 •

edited by oliviertassinari

Loading

eps1lon commented May 8, 2021 •

edited

Loading

oliviertassinari commented May 16, 2021 •

edited

Loading

oliviertassinari commented May 22, 2021 •

edited

Loading

oliviertassinari commented May 24, 2021 •

edited

Loading