Convert toolbar.tpl.php to HTML5

The big question here is do we keep the list element or not.

<nav>
   <ul>
      <li><a><span class="icon"></span>...<span class="element-invisible">...</span></a></li>
      <li><a><span class="icon"></span>...<span class="element-invisible">...</span></a></li>
      <li><a><span class="icon"></span>...<span class="element-invisible">...</span></a></li>
      <li><a><span class="icon"></span>...<span class="element-invisible">...</span></a></li>
   </ul>
</nav>

We have an opportunity here to keep semantics but tidy up the mark up as:

<nav>
      <a><span class="icon"></span>...<span class="element-invisible">...</span></a>
      <a><span class="icon"></span>...<span class="element-invisible">...</span></a>
      <a><span class="icon"></span>...<span class="element-invisible">...</span></a>
      <a><span class="icon"></span>...<span class="element-invisible">...</span></a>
</nav>

The menu tag is a very good shout Jeff. It would be nice to distinguish the toolbar from the standard page nav.

Ok, that seems like a pretty strong way to go. <menu type="toolbar"> is the correct semantic meaning and it requires list items.

I'm happy to give this patch a go. I need to start getting more friendly with the toolbar anyway.

I did a quick test of the accessibility support for menu.

<menu type="toolbar">
<li>Item 1</li>
<li>Item 2</li>
<li>Item 3</li>
</menu>

Testing with FF5, and JAWS 12 / NVDA 2011.1 no semantics were available. Recognizing that D8 release is quite some time away, and that UAs will more thoroughly implement the spec in this time, I still have some reservations with us making changes to Core that reduce the semantics available to screen-reader users.

Nonetheless, I do agree that menu type = toolbar is the correct semantic to be applied to this widget. My question is what do we do to ensure the best experience for all users. I'm not asserting a position here as much as begging the question, perhaps there should be a new discussion for this?

The situation for this specific widget improves if we add the WAI-ARIA toolbar role.

<menu type="toolbar" role="toolbar">
...
</menu>

The implied ARIA role for menu is toolbar, but since UAs might not map this we can set the role explicitly. This would also improve accessibility of menu if used by UAs that support ARIA semantics, but not html5 semantics. We are still removing semantics for users of UAs that do not support html5 / ARIA semantics, and this is what we need to consider.

@Jeff

I agree that accessibility comes in layers, this is part of the reason that WCAG 2.0 specifies levels for each success criteria.

When assessing the accessibility of a page, or of part of a page, the four questions (WCAG 2.0 guidelines) are: is the content perceivable, is the content understandable, is the content operable, is the content robust?

In this specific case we have a toolbar, which at its essence is a list of links with a special purpose. I am not incredibly concerned that when using menu type = toolbar -> li some users will not perceive the toolbariness of the content, but I am concerned that they will lose the semantics of his being a related list of links.

As an aside, @SteveFaulkner wrote on Twitter:

stevefaulkner: @ezufelt ... HTML5 menu is not implemented at all yet and who knows when it will be and what the result will be so suggest avoiding.

@Jeff

To be clear, at this time, I am not able to be supportive of using <menu> in Core. It reduces the semantics available to screen-reader users. As far as I know it is not implemented in any UA, which means we have no way of predicting how it will be implemented.

Should there be some sign that UAs are implementing menu type = toolbar in a way that provides equivalent, or greater, semantics to screen-readers I would remove my objection. I would also be happy to remove my objection with further persuasive comments.

@Jeff

Using elements that are not supported by UAs is not going to work. It doesn't matter if you, or I, would like for them to be supported: they are not.

Currently, with ul - li, JAWS and NVDA lets the user know that there is a list. With menu - li they do not. If this is do to lack of support in JAWS / NVDA, or w/ Firefox, or both, I do not know.

The processing model of a page is thus:

1. UA (firefox) parses the page into the DOM.
2. UA (firefox) parses the DOM into the accessibility API (IA2)
3. UA (JAWS) uses the DOM and accessibility API in order to present information (content and semantics) to the user.

Ok, it looks like we have three paths to go down.

1. Patch in our ideal mark up now and then reassess the situation closer to Drupal 8's release. I see this as menu with the tool bar attributes.
2. Add in as much semantic value as we can now, without removing existing semantics for legacy software and then reassess the situation closer to Drupal 8's release. This would be a combination of manu, nav and ul.

There's always a realistic cut off point for support. We can't guess at what that will be in a few years time.

So we're not going to use the ...

<nav>

.. tag?

Let's test this out,

 
<nav role="menubar">
  <menu role="toolbar" type="toolbar">
      <ul> 
         <li><a href="/">menu item one</a></li>
         <li><a href="/">menu item two</a></li>
         <li><a href="/">menu item three</a></li>
      </ul>  
  </menu>
</nav>

I'm curious to see if there are issues with this catch all solution. Nested lists and combined <menu> and <nav>. Semantically you could argue for all these elements and it would degrade nicely enough/

Here's a first pass at a patch. I also posted a patch for #14.

I ended up needed to clean up a lot of overly-specific CSS selectors like div#toolbar. Just more reason not to type selectors if it isn't semantically necessary. I imagine we're going to run into this a lot as we change the markup to use HTML5 elements.

Updating this patch to use the list type array key #list after an update to http://drupal.org/node/1203908#comment-4690726. The contextual links use the array key #type for their own purpose, so co-opting the key name for the theme_links function caused the contextual links to render improperly.

Tagging.

The patch here no longer applies. It fails on:

error: patch failed: modules/toolbar/toolbar.tpl.php:34
error: patch failed: modules/shortcut/shortcut-rtl.css:1

Can we please post the CSS changes in #1217048: Clean up the CSS for Toolbar module though? There's already a patch there. In the future, if we want to do the templates and CSS together (which is fine), just mark the CSS issue as a dupe and update the template issue title to reflect the CSS work too. We just need to make sure we are not duplicating efforts unnecessarily.

Also, I am completely fine with the proposed theme_links() changes, and I agree that <menu> could make sense here, but I am a little leery of using it until there's at least one browser implementation. When the browser implementation hits, and it may break ours. I think I'd rather go with <nav> until then, and revisit </menu> to be safe, but I'll support whatever you guys to decide.

+1 to holding off on <menu> until there is at least one browser implementation.

Part of the shortcut cleanup is needed because of the template changes.

I left out the toolbar css changes.
I also left out the menu stuff for now as it isn't supported yet by any browser.

Can we please add role="navigation" to the <nav>? I'm not sure about the nav wrapping the entire toolbar, as the menu itself is primary, and a lot of the rest seems superfluous, but I could go either way on that.

Like this?

Yes.

I also notice that the role="toolbar" is added to each list of items. I'm not sure that is going to be best practice. Toolbar http://www.w3.org/TR/wai-aria/roles#toolbar is meant to be used for things that are toolbars, as in a desktop application. I'd recommend removing the toolbar roles, and just leaving the navigation role. Note: this is the opposite of what I suggested in #6, apologies.

what about type=>toolbar?

Didn't notice that. @type is a state for the <menu> element, which we aren't using here. Again, I believe that the intent of <menu> is to be an interactive element, much like a menu in a desktop application, think of the Google Docs menu (File, View, etc.)

More info:

http://stackoverflow.com/questions/4969801/nav-or-menu-html5

nav is used for groups of internal links (a elements). Generally this means the links should travel to separate pages, or change content in the case of an AJAX page. Expect some sort of content change when clicking on a nav item.

menu is used for groups of controls (a, input, button). Generally this means the inputs should perform a function within the page. Expect some sort of javascript interaction when clicking on a menu item.

nav: the navigation for the site.

menu: the menu for a web application.

So nav it is :)

Working on a new and BETTER approach :p with richthegeek

This is what myself and aspilicious came up with - using the nav element is a more semantic approach, and reduces the tag soup in this one small area.

Requires review for:
- ARIA / accessibility (we removed the header stuff from theme_links for theme_nav)
- RTL (no rtl changes made yet at all)
- correctness and all other shiny things.

Attached the discussion that drove us to this approach

(ignore this epic failure on my part)

Ok, fixed prior bug and added attempt at RTL css.

Fixed RTL issues entirely (it looks identical to me vanilla d8 vs d7).

I won't comment on the approach. But, please make sure that $attributes['role'] = 'navigation' is set somewhere by default.

Added the role=>navigation by default

Ok new patch without windows endings ;)

Screenshots to show IE7 in rtl (and it works :D )and one to show what this means (semanticly).

Ok my editor settings are officially broken...

I don't love the theme_nav() idea. Seems like needless duplication, and also a job for theme_links(), which should ideally have a configurable wrapper element. I believe there is already an issue for that. I also think that some of this needs to be output in a list, not just links.

We definitely need more feedback here on the approach. Anyone?

Also, tagging.

Here's the issue I was referring to regarding theme_links(): #1203908: Add a type parameter to theme_links

After a discussion with the html5 team we decided to go for simplicity here. That means we are going to add the navaigation role and replace the outer div with a nav elemebt. (like in patch #30)

Reuploading now.

This patch is way easier to review, everyone can do it!

Looks great. Works as well. Thank you for removing the tag-inflected selectors.

I'm marking this as RTBC. I don't see any risk in committing this and it improves the semantics of the HTML and the clarity of the CSS around the toolbar.

Committed to 8.x. Thanks all. Glad to see more HTML5-ification. :)