Toolbar tabs should have ID attributes based on hook_toolbar() array keys rather than sequentially numbering

Much, much of Drupal is positioned in a specific order/place by using #weight. I don't see the problem of using #weight.

However, I do agree it's utterly nonsensical to have toolbar tab IDs have values like toolbar-item--3 (IDs based on order) instead of toolbar-item--shortcuts (IDs based on the key used in the items returned by hook_toolbar().

And in that sense, it's indeed problematic. Especially because it means that if you currently want to style a certain toolbar tab differently, you MUST use something like #toolbar-item--3 rather than #toolbar-item--shortcuts… which means that if you install another module, that the numbers might change and hence the IDs might change, hence the CSS wouldn't work anymore!

I'll have a look.

I noticed that the toolbar links (underneath the toolbar tabs) use the route name to generate their ids.

We could do the same for the toolbar tabs, for consistency's sake. It's also a little bit easier, because we can easily get the route name from the Url object inside the ToolbarItem element. The key of the hook_toolbar array however, is not that easy to get in Drupal\toolbar\Element\ToolbarItem::preRenderToolbarItem().

The downside would be that you could have two tabs with the same id, if they have the same route name but different route parameters (or query parameters?). However this is also possible with toolbar links.

I propose to move the code that generates the "unique" ids for the toolbar links to a re-usable function but keep the method to generate the ids the same for now. We can then use this function to generate the ids for the toolbar tabs as well, based on the route name.

While writing a patch for my suggested approach, I discovered that the "Edit" and "Tour" (?) buttons in the toolbar do not have a url (and so no Url object), which makes it impossible to generate an id for them based on the proposed solution.

Providing a fallback for such kind of toolbar tabs will probably make the code too inconsistent, so I agree now that it's best to use the keys defined in hook_toolbar().

I found a way to generate the id based on the key. However it was not very clean IMO.
Either we store the key in the element itself before we call drupal_render_children() in Toolbar::preRenderToolbar() (by looping over all the children), or we have to force everyone to add the key (id) to the element themselves when implementing hook_toolbar().

When trying out the first approach, I noticed that the "Edit" button never gets an actual id attribute in the HTML, even if one is specified in ToolbarItem::preRenderToolbarItem().

So technically it would still be possible to base the ids on the Url object for the tabs that have one, as the "Edit" button (which doesn't have a Url object) doesn't have an id attribute anyway.

I created a patch to generate the ids based on the route name.

Even if we agree to do it this way, we might want to look into using \Drupal\Core\Html::getId() instead of using the str_replace that was used before. But maybe this is best taken up in another issue, since this will probably change the ids of the toolbar links (not to be confused with the toolbar tabs)?

if you have different permissions for different roles this is even worse. So yeah we need to get rid of the weight.

Thanks for sharing your thought process, bertramakers, that is tremendously helpful :)

I'd agree with you thanks to your explanation, but unfortunately I can't because it assumes that any toolbar tab that is not a link cannot and shouldn't have a toolbar tray. I think this is an unnecessary limitation. And I think it might be the one aspect you didn't consider while weighing your options :)

I found a way to generate the id based on the key. However it was not very clean IMO.
Either we store the key in the element itself before we call drupal_render_children() in Toolbar::preRenderToolbar() (by looping over all the children),

This is basically a case of "array keys contain the identifiers, the corresponding values are the associated values". What is so terrible/ugly about:

foreach (array_keys($items) as $key) {
  // Set the hook_toolbar() key as the ID attribute for that toolbar item (tab).
  $items[$key]['#id'] = $key;
}

This is basically a case of "array keys contain the identifiers, the corresponding values are the associated values". What is so terrible/ugly about:

It's not terrible/ugly, I was just hoping to solve it with the data that we already have in the array, and in a way that's more consistent with the toolbar links underneath the tabs. But you're right, I didn't take the trays into account and they should work as well for toolbar tabs that don't have a url.

I created a new patch that generates the ids based on the array key in hook_toolbar().

I didn't create an interdiff because the patch is completely different from the first one.

1. +++ b/core/modules/toolbar/src/Element/Toolbar.php
   @@ -97,6 +98,13 @@ public static function preRenderToolbar($element) {
   +    $children = Element::children($element);
   +    foreach ($children as $key) {
   

   Why not do this in a single line?

2. +++ b/core/modules/toolbar/src/Element/ToolbarItem.php
   @@ -51,7 +51,7 @@ public function getInfo() {
   -    $id = Html::getUniqueId('toolbar-item');
   +    $id = Html::getId('toolbar-item-' . $element['#id']);
   

   Hrm, this means #id does NOT end up being the ID attribute. That's misleading.

   We can move the generating of an ID attribute entirely out of ToolbarItem::preRenderToolbarItem() and into Toolbar::preRenderToolbar(), which then gets us easier to understand code.

Okay, created a new patch based on the review remarks.

Great! Just one more nitpick before this is RTBC :)

+++ b/core/modules/toolbar/src/Element/ToolbarItem.php
@@ -50,8 +49,7 @@ public function getInfo() {
+    $id = $element['#id'];

This looks rather silly :)

Why not do it directly in the code block below it:

    // Provide attributes for a toolbar item.
    $attributes = array(
      'id' => $id,
    );

?

I left it like that because it's also used multiple times further down in the code.

    if (!empty($element['tray'])) {
      // Provide attributes necessary for trays.
      $attributes += array(
        'data-toolbar-tray' => $id . '-tray',
        'aria-owns' => $id,
        // ...
      );

      // ...

      // Provide attributes for the tray theme wrapper.
      $attributes = array(
        'id' => $id . '-tray',
        'data-toolbar-tray' => $id . '-tray',
        'aria-owned-by' => $id,
      );
}

(So 5 times + the one in the attributes array that you mentioned.)

We could replace all those with $element['#id'] of course, your call.

Oh, oops! My bad! Sorry :) In that case, this is good to go — thank you very much!

Nice and clean solution, +1 from me, thanks!

I think due to the fact that module install could change the numbering and therefore style things incorrectly we are dealing with a bug here. This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed b733559 and pushed to 8.0.x. Thanks!