The Menu-Toolbar area should be refactored so that Tray items are nested in the same container as their associated parent Toolbar item.

Could we modify the twig file in core/modules/toolbar/template/?

We would delete the second for loop and its contents to the first for loop. Check if the current array index exists in the second array, if it does print out that div.

this may also break some javascript or css so that would need to be modified

It's fine to break stuff, get the patch as far as you can and jump in on IRC #drupal-contribute to get help to get the patch even further :)

This was an interesting patch, I expect it to have a few issues, but it’s good to get the ball rolling. I have tried the keyboard navigation out with the patch applied and I can definitely see the advantages.

A few things to note:

• This turns the mobile menu into an accordion. I think this actually works pretty well.
• The semantic structure of the toolbar is now greatly improved. H3’s follow H2’s etc.

My biggest annoyance was dealing with "toolbar-lining” and its :before element. It was appearing over the parent toolbar items, and top could not be set dynamically (as it’s not part of the DOM). If we could hard-code the height of the bar, it wouldn't be an issue, but this isn’t how the rest of the toolbar handles it. Can someone confirm we still need this lining? It doesn't seem to break anything in chrome by simply removing it. I got around this by bringing the top level menu items above it, but I'm not totally satisfied with this.

If you are providing a review, please test under different orientations and screen sizes.

That's a pretty important patch to allow us to simplify the JS behind toolbar #2145365: Make toolbar rendering fast.

Not sure about the acordion-ification of the menu though. I personally do not use the dropdown menu so I don't really care either way.

Just wanted to point out:

#1800614: Improve the responsive toolbar accessibility

and especially comment#6 of that issue...

Patch looks really good but the a11y implications are not clear to me. Seems like your comment in the other issue suggest another solution than keeping tabs and trays separated might be possible.

The patch from February 15 still applies.

The accessibility challenge is that when someone is just using a keyboard to navigate a site, if they hit the Menu toolbar it expands nicely, but the next tab doesn't go to Content, which is the first item in the expanded menu, but rather you have to tab to Shortcuts and then over to admin before you get to the Content tab.

The logical assumption is that if you open the Menu toolbar you might want to use it.

The patch addresses this issue nicely. With the patch it allows keyboard only users to more quickly get to the information they need.

I think this is RTBC.

It's in my queue to review.

Would be great to get this reviewed in TwinCities DC.

I re-rolled the patch and it looks like it works, could someone review?

Often useful to provide an interdiff.

Found this still in the CSS

+<<<<<<< HEAD
   [dir="rtl"] .toolbar .toolbar-bar .toolbar-icon:before {
+=======
+  .no-svg .toolbar .toolbar-bar .toolbar-tab > .toolbar-icon:before {
+    background-size: auto auto;
+  }
+  [dir="rtl"] .toolbar .toolbar-bar .toolbar-tab > .toolbar-icon:before {
+>>>>>>> Applying patch from issue 2173527 comment 8487843

Should be an easy thing to remove.

That doesn't break the installation process - http://sd12046bf51ce36a.s2.simplytest.me

This causes other CSS issues.

Re-rolled, without git markers. The interdiff fails, but the patch applies cleanly. Removed an unmatched < /div > as well

There seems to be a missing <div> tag when I look at the source.

One of the last two </div>'s is reported as being extra..

        <div id="toolbar-item--6-tray" data-toolbar-tray="toolbar-item--6-tray" aria-owned-by="toolbar-item--6" class="toolbar-tray"><div class="toolbar-lining clearfix"><h3 class="toolbar-tray-name visually-hidden">User account actions</h3><ul class="menu"><li class="account"><a href="/user" title="User account">View profile</a></li><li class="account-edit"><a href="/user/1/edit" title="Edit user account">Edit profile</a></li><li class="logout"><a href="/user/logout">Log out</a></li></ul></div></div>      </div>
      
</div>

  </div>

EDIT: Other than that it is great. Think it's very close to RTBC.

The patch applies cleanly. It doesn't need a reroll anymore, but perhaps just the removal of an extraneous element. I will check that now.

I don't know where that extra div is coming from. Please check toolbar.html.twig after applying the patch. I can't see anything wrong with it, but I'm new to twig.

No longer applies.

+++ b/core/modules/toolbar/templates/toolbar.html.twig
@@ -25,23 +25,21 @@
+        {% endspaceless %}
+      </div>
     {% endfor %}
-  </nav>
-  {% for tray in trays %}
-    {% spaceless %}

The patch #20 removed the tag </nav> for that maybe is the issue of the <div>.

Also I fix a typo in core/modules/toolbar/css/toolbar.module.css

.toolar .toolbar-tray > .toolbar-lining {
  position: relative;
}

to:

.toolbar .toolbar-tray > .toolbar-lining {
  position: relative;
}

Here the reroll.

+++ b/core/modules/toolbar/templates/toolbar.html.twig
@@ -25,23 +25,22 @@
+      <div{{ tab.attributes.addClass('toolbar-tab') }}>
+	      {{ tab.link }}
+	      {% spaceless %}
+	        <div{{ tray.attributes }}>
+		        <div class="toolbar-lining clearfix">
+			        {% if tray.label %}
+			          <h3 class="toolbar-tray-name visually-hidden">{{ tray.label }}</h3>
+			        {% endif %}
+			        {{ tray.links }}
+	          </div>
+          </div>
+	      {% endspaceless %}

Please use spaces instead of tabs.

There are testbot problems ATM, these are the actual failures:

Drupal\shortcut\Tests\ShortcutTranslationUITest              143 passes   3 fails

Just a re-roll. I don't know enough about how the CSS file has changed to do much more than copy the added text over.

It doesn't look like we are any closer to having nested elements in the same container as their parent toolbar item...

Here is a new patch which also patch the toolbar.html.twig defined in the classy theme. This should fix the issued described in #40.

This one fix the new failing test.

I'm ready to mark this RTBC. I've tested it in Chrome, FF, IE. Also reviewed it on my Android phone.

The code looks good, there are tests and it would be great to get this in. Screenshots are attached showing the nested code.

1. +++ b/core/modules/toolbar/css/toolbar.icons.theme.css
   @@ -150,7 +150,7 @@
   -  .toolbar .toolbar-bar .toolbar-icon {
   +  .toolbar .toolbar-bar .toolbar-tab > .toolbar-icon {
   

   .toolbar-tab .toolbar-tab

   This looks extremely weird.

2. +++ b/core/modules/toolbar/css/toolbar.module.css
   @@ -170,6 +170,10 @@ body.toolbar-tray-open.toolbar-fixed.toolbar-vertical .toolbar-oriented {
   +  z-index:502;
   

   Please document why the specific value of 502 was chosen. Also, needs space after the colon.

3. +++ b/core/modules/toolbar/templates/toolbar.html.twig
   @@ -25,23 +25,22 @@
   +    {% for key, tab in tabs %}
   +      {% set tray = trays[key] %}
   

   Nice :)

4. +++ b/core/modules/toolbar/templates/toolbar.html.twig
   @@ -25,23 +25,22 @@
   -      <nav class="toolbar-lining" role="navigation">
   

   We lost this role="navigation". I'm assuming this was intentional, but I'd like explicit confirmation.

   And why did this <nav> become a <div>?

1) That can't be correct... That slipped by me. EDIT: But it is .toolbar-bar .toolbar-tab

2) Is there a standard for what the z-index should be?

3) :)

4) role="navigation" & <nav> should remain. I should have seen that... Too distracted these days. Sorry.

1. Oops! Then my next question: why is the > necessary?

2. Not really, but 502 seems very... random. And hence feels/seems like a hack.

Here is a new patch which should fix the issues discussed in #45 #46 #47.

1. I think the > is required now because this target the icons on the "first level" toolbar (the icons on the black bar). And now that the tray items are nested in it they can also have .toolbar-icon inside them.

2. The 502 might have been chosen because the .toolbar .toolbar-tray has a z-index of 501. With something lower the links become unclickable. This is also consistent with the .toolbar-oriented .toolbar-bar which has a z-index of 502 too and this comment: "Layer the bar just above the trays and above contextual link triggers."

1. That makes sense, thanks.

2. Thanks for adding a comment to the CSS for that.

I'm now fine with this being committed, I'll let @mgifford mark it as RTBC again. (Wanna make sure that the markup changes indeed don't adversely affect the accessibility.)

I forgot to change the test again.

This looks good now. I reviewed the interdiff. Thanks for adding back in the role="navigation" & <nav>

I've tested it in FF & Chrome. I found another issue with the toolbar by doing the review, but I'll post that separately. It's unrelated to the patch.

Accessibility is a prioritized change during beta. Committed 184d4b4 and pushed to 8.0.x. Thanks!

Why did you delete aria-label="{{ tray.label }}" from core/modules/toolbar/templates/toolbar.html.twig?

It's from the original patch and hasn't been put in question since.

Reading #1800614-6: Improve the responsive toolbar accessibility makes it clear it should be kept so it needs to be added back. Opened new issue for it (since a new patch after it's been committed gets in the way of the workflow).

Please review #2548027: Follow-up add back aria-label to toolbar tray for the fix.

And another regression: the orientation toggling arrows: #2546196: Toolbar's orientation-toggling arrows broken by #2173527.