Markup for menus

<strong>{{ item.link.content }}</strong> - not good. If you don't want it bold, you'll have to go to defaults. Better:
<span>{{ item.link.content }}</span>.
Also need different classed. Maybe something like this?

{% if menu_level = 1 %}
<nav class="nav menu-{{ menu-name}}">
{% endif %}
<ul class="menu level-{{ menu_level }}">
  {% for item in items %}
    <li class="menu__list-item">
    {% if item.link.active_trail %}
      <span class="menu__active">{{ item.link.content }}</span>
    {% else %}
      {{ item.link }}
    {% endif %}

    {{ item.children }}
    </li>
  {% endfor %}
</ul>
{% if menu_level = 1 %}
</nav>
{% endif %}

It's a minor point, but I honestly don't think you need the span at all. Could you not do something like:

{% if menu_level = 1 %}
<nav class="menu menuname-{{ menu-name}}">
{% endif %}
<ul class="menu-level-{{ menu_level }}">
  {% for item in items %}
    {% if item.link.active_trail %}
      <li class="active">{{ item.link.content }}
    {% else %}
      <li>{{ item.link }}
    {% endif %}
    {{ item.children }}
    </li>
  {% endfor %}
</ul>
{% if menu_level = 1 %}
</nav>
{% endif %}

I agree that .active is better than .menu_active - I think the latter might introduce confusion more than anything.

The CSS standards recommend that we don't make classes too broad. We should use menu-item--active

I have a couple of different takes on this. I’ve omitted any Twig to focus on the markup.

<nav role="navigation">
  <ul class="menu menu--level-1">
    <li class="menu__item is-active">
      <a class="menu__link">Current page</a>
    </li>
    <li class="menu__item">
      <a class="menu__link">Another page</a>
    </li>
    <li class="menu__item">
      <a class="menu__link">A section with subnav</a>

      <ul class="menu menu--level-2">
        <li class="menu__item">
          <a class="menu__link">Submenu item</a>
        </li>
      </ul>

    </li>
  </ul>
</nav>

- Added an ARIA role to the nav
- Removed default classes from the <nav> and moved .menu to the <ul>. The list is what repeats, so the 'menu' class is more useful there. I’ve not added new classes to the nav in the spirit of 'start with nothing'.
- Each item in the menu tree has a class following the new CSS standards. However, there is a case to be made for .menu > li instead of .menu__item.
- States follow the new standards too, with .is-active for the active item. (State classes are the only place we should see qualified selector in core, i.e. .menu__item.is-active. The extra specificity is useful for states but is a detriment in most other cases.)
- Menu levels are indicated by a class modifier, since they are variants of the menu component.

If we wanted to embrace listless navigation, we could even consider this:

<nav class="nav nav--level-1" role="navigation">
  <span class="nav__item is-active">
    <a class="nav__link">Current page</a>
  </span>
  <span class="nav__item">
    <a class="nav__link">Another page</a>
  </span>
  <span class="nav__item">
    <a class="nav__link">A section with subnav</a>

    <span class="nav nav--level-2">
      <span class="nav__item">
        <a class="nav__link">Subnav item</a>
      </span>
    </span>
    
  </span> 
</nav>

I really like this idea of listless navigation. Having listening to a screen reader speak outloud the endless "list" declarations, especially when tabbing through an extensive navigation structure (e.g. many, many lists within lists, many sublevels), it does make sense to reconsider this output. After all, we are putting these in lists in the first place partly for accessibility, right? And for semantics, the <nav> element does address that concern.

You guys may already be way ahead of me on this, but I was interested in this topic of using HTML lists for menus (or not). And it seems to be a big part of all this code we're writing for twig templates.

So I asked some accessibility folks, and started a post here, on the Accessibility Group page

The consensus seems to be to stick with HTML Lists.

Again, you all may already know this, but on the chance you do not... now you do.

Moving to core queue.

Actually markup is a better component.

Let's stay with the ul > li variant now.
We should better focus on classes and semantics.
@ry5n - how an item without a link will look like?

In D7 in all my projects I add something like that:

$original_link = $element['#original_link'];
    $depth = 'depth-' . $original_link['depth'];
    $menu_name = 'item-' . $original_link['menu_name'];
  
    $variables['element']['#attributes']['class'][] = $depth;
    $variables['element']['#attributes']['class'][] = $menu_name;

So I usually add the depth to each item and not the parent. Also I add the menu name to the item - that makes theming the menus really great. Adding depth on the actual li can reduce the selector target to 1 item:

.nav--level-2 > .nav__item {}
.nav__item.nav--depth-2 {}

Any thoughts on this?

we only wanna add "active" class to the stark theme right ? & then add all the other stuff to bartik

The end result should be its easy for whomever to create exactly the markup they wanna do for a given menu

Yes, and yes. Especially important for giving us all the ability to mark things up how we want is to ensure that the data structure containing the actual navigation tree is easy to retrieve and manipulate. (But I suppose that’s another story.)

Just active/active trail classes sound good, and no CSS please, or silly triangles on .collapsed items, but you might get some push back removing collapsed classes. leaf, first, last all can go imo.

I'm happy if we have no line endings \n or white space between li elements, I don't wanna be floating li's for another 5 years.

Can someone pleas update the summary with the current requested syntax so someone can start working on this?

Meta issue added.

Done, if someone can confirm the suggested markup in the issue summary I'm happy to work on a patch right-away.

Updates issue summary

Updated Bartik markup

Ok, I've updated the mark up in the summaries to remove white space between the list items based on this article.

Cool, thanks. I'll work on it.

Seems that we need a major API change for this:

Tasks

- Remove theme_menu_tree
- Remove theme_menu_link
- Update template_preprocess_menu_tree to add extra twig variables (menu_level, items).
- Add twig file (/core/modules/menu/templates/menu-tree.html.twig)

Also, why do we use a theme_menu_tree and a theme_links? And why do we use theme_links for the main_menu? Shouldn't we fix this as well?

Why does my Tools menu is an ul.menu and my Main menu an ul.links?

Updated Stark mark up

@BarisW any ETA on the patch?

@mgifford. I've unassigned myself from the issue. This goed way above my head ;)
Some work is done is this issue: #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template, so we might want to close this one here to keep the focus in one place?

#890362: Links should not be indicated by color only is waiting on a resolution of this issue.

Looks like there's been no direct means to address #1777332 which also stalled in October.

this might not finalise until #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template which is waiting on #1898478: menu.inc - Convert theme_ functions to Twig