Meta issue: #1980004: [meta] Creating Dream Markup

The markup for Menu's should be improved. We suggest the following markup for Stark and Bartik:

Suggested markup in Stark:

<nav role="navigation">
  <ul class="menu">
    <li class="is-active">
      <a>Current page</a>
    </li><li>
      <a>Another page</a>
    </li><li>
      <a>A section with subnav</a>
      <ul>
        <li>
          <a>Submenu item</a>
        </li>
      </ul>
    </li>
  </ul>
</nav>

Suggested markup in Bartik:

<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>

Comments

oresh’s picture

<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 %}
mortendk’s picture

sounds reasonable but i would like to kill somethings:

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

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

the .menu__list-item we can easyli target with a .menu > ul li {}

i would like if we used the same name for active elements all over so instead maybe us .active instead of menu__active

chippper’s picture

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.

LewisNyman’s picture

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

ry5n’s picture

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>
minneapolisdan’s picture

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.

minneapolisdan’s picture

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.

Cottser’s picture

Title: menu markup idea » Markup for menus
Project: » Drupal core
Version: » 8.x-dev
Component: Twig templates conversion (front-end branch) » theme system
Category: feature » task

Moving to core queue.

Cottser’s picture

Component: theme system » markup

Actually markup is a better component.

oresh’s picture

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?

mortendk’s picture

remember that we wanna keep markup as clean as possible outta core
imho that means that 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, without beeing forced into whatever desissions we are taking right now

ry5n’s picture

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.)

Jeff Burnz’s picture

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.

jenlampton’s picture

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

jenlampton’s picture

Issue summary: View changes

Meta issue added.

BarisW’s picture

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

BarisW’s picture

Issue summary: View changes

Updates issue summary

LewisNyman’s picture

Issue summary: View changes

Updated Bartik markup

LewisNyman’s picture

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

BarisW’s picture

Assigned: Unassigned » BarisW

Cool, thanks. I'll work on it.

BarisW’s picture

Assigned: BarisW » Unassigned

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?

BarisW’s picture

Issue summary: View changes

Updated Stark mark up

mgifford’s picture

@BarisW any ETA on the patch?

BarisW’s picture

@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?

mgifford’s picture

#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.

Jeff Burnz’s picture

LewisNyman’s picture

Issue tags: +frontend, +Twig
mortendk’s picture

Status: Active » Closed (works as designed)

a little cleanup here:
After the updates to the menu.html.twig this issue seems to not be relevant anymore.