Problem/Motivation

The initial commit of the responsive toolbar patch included a basic definition of the array structure to be returned by modules that implement hook_toolbar.

$items[$group_name] = array(
  'tab' => array(
    'title' => t(''),
    'href' => '',
    'attributes' => array(
      'class' => array('icon', 'icon-menu'),
    ),
  ),
  'tray' => array(
    '#heading' => t(''),
    'content' => array(/* renderable content */),
  'weight' => -15,
);

The structure returned by modules implementing hook_toolbar is needed to produce:

  1. A top-level tab.
  2. An optional tray of links or control structures. The content of the tray is arbitrary.

screenshot of the edit module tab and tray in the toolbar.

Tabs are returned as link structures and merge into an array passed to theme_links. theme_links passes each link to the l() function or directly renders the item with a span. This results in the tab items not running through the full drupal_render process. Pre and post render functions cannot be associated with the tabs for example. The edit module, in particular, needs to determine if its tab should be rendered in a post render function.

We need to determine an array structure to be returned from hook_toolbar that is flexible and simple to implement.

Use Cases

Countcount module

The Countcount module wants to have a drop-down list in the Tab area: Zero, One, Many. So far, so good: my patch allows any render element to be used for a tab. Next, when the user clicks on "Many", the corresponding tray should open, so that the user can select the actual number.

User module

Show the avatar image of an authenticated user in the User tab.

Screen shot of the user module toolbar tab with a small image of the users avatar

ShoppingCart module

Show the number of items a user has in their shopping cart in a toolbar tab. Clicking the tab brings the user to the shopping cart management page. The ShoppingCart module maintainers also want to provide a "drop button" like menu with the option "Check out" on the toolbar tab. This should not be a toolbar tray.

MessageStream module

A hypothetical MessageStream module wants to place a button in the toolbar that opens a sidebar with a list of system messages. The placement and styling of the button should not be like a toolbar tab. In fact, all styling and behavior will be custom. In this case, the MessageStream module is using the toolbar as a simple container.

A hypothetical Message Stream module showing a sidebar tray opened with a list of system messages

Search module

A screenshot of a search box in the toolbar.

Proposed resolution

The purpose of hook_toolbar is to provide module developers a means to add components to the toolbar. The toolbar is meant to be a container for top-level site actions and configuration. See the use cases above for examples of what a module might add to the toolbar.

In toolbar also provides a construct known as a "tray". A tray is a container that will display vertically on narrow screens and can be display horizontally on wide screens. A module developer can specify renderable content that should be placed in a tray. The tray is controlled by a trigger, normally an HTML link element.

jessebeach and benjifisher have been working through an approach to improve the processing of hook_toolbar so that developers can leverage the toolbar tray system, but also be able to pass any renderable element as a component of the toolbar.

API changes

hook_toolbar()
An implementation of hook_toolbar() should return a keyed array of render elements, each element of type toolbar_item.
New render elements
We define a new type of render element, toolbar_item, with properties '#tab' (required), '#tray', and '#weight'. The tab and tray can be any render elements, and the weight is an integer. This type is given a default pre-render function, toolbar_pre_render_item(), theme function, theme_toolbar_item(), and theme wrapper, theme_toolbar_tab_wrapper(). We also define a new toolbar type, which is used internally. If the tray is present and javascript is enabled, then the tab will toggle the tray's visibility.
Theme functions
  • theme_toolbar(): Responsible for theming the toolbar and its child components.
  • theme_toolbar_item(): Responsible for theming the toolbar_item element. This renders only the tab, leaving the tray for theme_toolbar().
  • theme_toolbar_tab_wrapper(): Wraps the #children of an element in a div with the class tab. Provide common styling for "tab" elements in the toolbar.
  • theme_tray_heading_wrapper(): Appends an <h3> heading with the contents of a #heading property to a tray. Used for accessibility landmarking.
  • theme_toolbar_tray_wrapper(): Wraps the #children of a rendered content of a toolbar component's tray property in markup that styles and provides behavior attachment for a toolbar tray.

Some of the follow-up issues may add default properties of the toolbar_item type and/or change the list of theme functions, but we have settled on the '#tab', '#tray', and '#weight' properties of the toolbar_item type.

Example

Under this patch, a module might return the following renderable in hook_toolbar(). This code would create a tab in the toolbar with the label "Shopping cart" and an associated tray with actions associated with the cart.

$items['commerce'] = array(
  '#type' => 'toolbar_item',
  '#tab' => array(
    '#type' => 'link',
    '#title' => t('Shopping cart'),
    '#href' => '/cart',
    '#options' => array(
      'attributes' => array(
	'title' => t('Shopping cart'),
      ),
    ),
  ),
  '#tray' => array(
    '#heading' => t('Shopping cart actions'),
    'shopping_cart' => array(
      '#theme' => 'item_list',
      '#items' => array( /* An item list renderable array */ ),
    ),
  ),
  '#weight' => 150,
);

Remaining tasks

  • Gather feedback and opinions on the proposed patch in #58.
  • Manual testing.

User interface changes

None most likely.

Original report by jessebeach

This issue is a followup to: #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop

Related issues.

#1869638: Make the menu shown in the administration menu tray configurable
#1877468: Finalize usage of hook_toolbar_alter()
#1877482: Toolbar tabs should have ID attributes based on hook_toolbar() array keys rather than sequentially numbering
#1877486: Clean up code in toolbar.module

CommentFileSizeAuthor
#74 toolbar-refactor-1847198-74.patch6.59 KBeffulgentsia
#71 toolbar-refactor-1847198-71.patch36.98 KBeffulgentsia
#71 interdiff.txt654 byteseffulgentsia
#70 toolbar-refactor-1847198-69.patch38.29 KBjessebeach
#70 interdiff_68-to-69.txt10.81 KBjessebeach
#69 toolbar-refactor-1847198-69.patch36.84 KBeffulgentsia
#69 interdiff.txt9.91 KBeffulgentsia
#68 toolbar-refactor-1847198-68.patch37.34 KBeffulgentsia
#68 interdiff.txt7.92 KBeffulgentsia
#67 toolbar-refactor-1847198-67.patch46.25 KBjessebeach
#67 interdiff_58-to-67.txt1.07 KBjessebeach
#58 toolbar-refactor-1847198-58.patch44.99 KBbenjifisher
#51 toolbar-refactor-1847198-51.patch43.64 KBbenjifisher
#51 interdiff-50-51.txt8.53 KBbenjifisher
#50 toolbar-refactor-1847198-50-do-not-test.patch43.03 KBbenjifisher
#50 interdiff-38-50.txt10.16 KBbenjifisher
#40 toolbar-ABCDE.png15.15 KBbenjifisher
#38 toolbar-refactor-1847198-38.patch43.8 KBjessebeach
#38 toolbar-refactor-1847198-27-to-38-do-not-test.patch36.33 KBjessebeach
#38 countcount-module-before-removing-hook_toolbar_alter.patch3.9 KBjessebeach
#38 countcount-module-after-removing-hook_toolbar_alter.patch5.31 KBjessebeach
#36 toolbar-refactor-1847198-36-do-not-test.patch27.51 KBbenjifisher
#36 interdiff-27-36.txt2.2 KBbenjifisher
#27 1847198_toolbar-dx_27.patch28.57 KBjessebeach
#27 interdiff.txt12.02 KBjessebeach
#26 1847198_toolbar-dx_25.patch21.94 KBjessebeach
#26 interdiff.txt9.7 KBjessebeach
#26 interdiff-to-preprocessors.txt3.5 KBjessebeach
#26 interdiff-preprocessors-to-25.txt7.49 KBjessebeach
#25 1847198_toolbar-dx-preprocess-fn_21_do-not-test.patch4.03 KBjessebeach
#25 1847198_toolbar-dx_25.patch8.1 KBjessebeach
#25 1847198_toolbar-dx-search_25_do-not-test.patch1.79 KBjessebeach
#21 1847198_toolbar-dx_21.patch19.95 KBjessebeach
#21 interdiff.txt (17 to 21)20.74 KBjessebeach
#20 countcount-horizontal.png14.51 KBbenjifisher
#17 toolbar-refactor-1847198-17.patch11.78 KBbenjifisher
#16 toolbar-refactor-1847198-16.patch11.08 KBbenjifisher
#14 toolbar-refactor-1847198-14.patch7.12 KBbenjifisher
#12 user-module-avatar.png141.46 KBjessebeach
#12 MessageStream-module-toolbar.png119.19 KBjessebeach
#12 toolbar-search-module.png15.61 KBjessebeach
#8 toolbar-refactor-1847198-8.patch6.73 KBbenjifisher
#7 toolbar-refactor-1847198-7.patch5.8 KBbenjifisher
#1 spark-dev-tray-tab.png53.68 KBjessebeach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jessebeach’s picture

FileSize
53.68 KB

adding an image

jessebeach’s picture

Issue summary: View changes

added more info

webchick’s picture

The thing I like about toolbar_theme_tabs() and toolbar_theme_trays() is that it ideally helps address sun's feedback in the original toolbar issue about being able to put things other than links in this place. The Northstar designs even have a Search module widget in the "tabs" section, and it's also the ideal place to put contextual controls like the "Save / Publish" button when inline editing, or the mobile preview functionality from Spark.

Shyamala’s picture

Issue tags: -toolbar
benjifisher’s picture

Shouldn't it be theme_toolbar_tabs() (or _tab()) and theme_toolbar_tray() instead of toolbar_theme_tabs() and toolbar_theme_tray()?

On second thought, I do not think we want to use theme functions for this. That would be useful if we wanted to let themers give a custom design to all the tabs, but I think what we really need is to allow the module that defines the tab/tray combination to specify how it will be rendered.

Perhaps the simplest thing is to let each tab and each tray be an arbitrary render element. The module that defines them can specify any standard or custom #type. Or maybe #theme. By default, we will use '#type' => 'link', for the tab elements, so they will be rendered by theme('link'). This will require minor changes to the existing implementations, because theme_link() and theme_links() take different keys. (argh!)

If people like this approach, I think I can implement it. All I need is some time. I would like to know what @sun thinks.

@webchick, can you give a pointer to the Northstar designs? I am clueless.

jessebeach’s picture

benjifisher, adding #types toolbar_tab and toolbar_tray sounds promising. If you could post a simple initial patch we can hash through it.

benjifisher’s picture

@jessebeach:

Can do, but not today. (I am not sure we will need the types you suggest, but I will have to think about it.)

I spent all morning adding issues (several screen shots, two tiny patches). It is a relief to get all of these questions out of the back of my mind and onto the issue queue, but it used up a lot of my available time.

benjifisher’s picture

Status: Active » Needs work
FileSize
5.8 KB

I have only had an hour or two to work on this. The attached patch is a work in progress. In other words, it totally breaks the toolbar. I am not sure that I have the right keys ('text' or '#text'?) for my render elements, for example. If I am lucky, a rewrite of template_preprocess_toolbar() will get it close to working again.

Once the code is working, we will have to rewrite the tests and toolbar.api.php.

benjifisher’s picture

Upon further investigation, I see that an element with '#type' => 'link' is processed by drupal_pre_render_link() and not theme_link(). So instead of '#text' and '#path' I should use the keys '#title' and '#href'.

The attached patch is still a WIP, but it is a lot closer. I am getting errors from drupal_process_attached(), and I am getting unstyled lists of links, but at least I have the attributes about right. Now I think it is time to look at template_preprocess_toolbar().

benjifisher’s picture

I have a simple use case in mind. The Countcount module wants to have a drop-down list in the Tab area: Zero, One, Many. So far, so good: my patch allows any render element to be used for a tab. Next, when the user clicks on "Many", the corresponding tray should open, so that the user can select the actual number.

First: is this the sort of use case we want to support?

Second: if so, then how do we structure it? What should hook_toolbar() return to indicate the correspondence between links and trays, keeping in mind that we want to default to a single tab owning its own tray.

benjifisher’s picture

I have one possible answer to the question I asked in #9. (Please, stop me from talking to myself!)

As long as toolbar_view() builds a render array, the Countcount module can pass the Many link in countcount_toolbar() and then add the Zero and One links in countcount_toolbar_alter().

I think this approach makes good use of the structure we already have, and it keeps the structure simple for modules that add a single link to the Tabs section.

jessebeach’s picture

The association between the tray and tab has to be soft (i.e. not expressed through DOM relationships) because of styling and placement. Also, I'd like the Toolbar to have some kind of common handling for opening a tray when a tab is pressed.

I'll look over the patch in #8 toady.

Regarding the use case, I think it's interesting. Let's put it and a few more in the description of the issue and see what type of behavior we might want to support.

jessebeach’s picture

Issue summary: View changes

adding more info

jessebeach’s picture

Adding some images for the issue summary.

jessebeach’s picture

Issue summary: View changes

adding use cases

benjifisher’s picture

@jessebeach:

Thanks, you saved me from talking to myself!

Unless I am missing something, my suggested use of hook_toolbar_alter() in #10 is consistent with your point about "soft associations" in #11. We would have to rearrange the code in toolbar_view() so that the sequence is

  1. Invoke hook_toolbar()
  2. Process the render arrays, adding the tab/tray associations.
  3. Invoke hook_toolbar_alter()

In the Countcount use case, this means that the contrib module does not have to know what is going on in Step 2. Because the Many link has already been processed, it has the necessary "soft association" to its tray. Now the contrib module can alter the render array, using the existing Many element and adding the Zero and One links.

I will have to think about the other use cases. Thanks for providing them.

benjifisher’s picture

Finally, a patch that is not completely broken! The status is still NR, for a variety of reasons: need for clean-up, need to update tests and .api.php, maybe others. Oh, yes, I have to restore the <h2 class="element-invisible">Toolbar</h2>.

It turns out that the main problem with the patch in #8 is that I tried to "clean up" the line

  $build['#attached']['library'][] = array('toolbar', 'toolbar');

by removing the "extra" 'toolbar'. Live and learn!


I think the code is good enough that I want to continue the discussion in #9—#13 before I do any more.

benjifisher’s picture

Back to talking to myself ...

I think I like the approach I outlined in #13. I will have to rearrange the code a bit, deferring the sort until after invoking hook_toolbar_alter(), because I want to make it easy for other modules to change the weights.

Overall, there is nothing in Drupal that cannot be represented by a renderable array, so I think that making the tabs an item_list of render arrays is flexible enough for all the use cases that Jesse added to the issue summary.

I want to get into specifics of toolbar_view().


    $media_queries['toolbar']['breakpoints'] = array_map(
      function($object) {return $object->mediaQuery;},
      $breakpoints->breakpoints);

    $build['#attached']['js'][] = array(
      'data' => $media_queries,
      'type' => 'setting',
    );

I added the array_map() line above, replacing the ones below, which seem to have been added back. I am pretty sure they have the same effect.

    foreach ($breakpoints->breakpoints as $key => $breakpoint) {
      $build['#attached']['js'][0]['data']['toolbar']['breakpoints'][$key] = $breakpoint->mediaQuery;
    }

I thought that array_map() might be more efficient that foreach(), and PHP 5.3 allows us to use anonymous functions for the callback. If the second form is easier to understand, or I guessed wrong about efficiency, then we can stick with the foreach()


I think I am responsible for these lines:

  foreach ($toolbar_groups as $group => $items) {
    // ...
      if (array_key_exists($group, $build['trays'])) {
        array_merge($build['trays'][$group], $items['tray']);
      }
      else {
        // Assign the tray to the build.
        $build['trays'][$group] = $items['tray'];
      }

It looks like an overabundance of caution. Is there any way the condition can be satisfied?

benjifisher’s picture

The attached patch works well enough that I have started to clean up the code and add comments. It would be interesting at this point to add a module to the Examples project that implements one of the use cases we have described. See whether this patch actually does what I think it does!

One little annoyance. I was not sure of the right way to get the second line of this markup:

  <!-- Tabs -->
  <h2 class="element-invisible">Toolbar</h2>

The problem is that theme_links() handles a heading, but theme_item_list() hard-codes its title as an <h3>. I solved this with a few lines in toolbar_pre_render() and the template file. It makes sense to me, but maybe there is a better way.

edit: D'oh! I left in a dsm() line.

benjifisher’s picture

Status: Needs work » Needs review
FileSize
11.78 KB

I modified toolbar_test.module to be consistent with the new hook_toolbar(). The test passes locally, and I am setting the issue to NR so that the test bot can have its say.

penyaskito’s picture

Some nitpicking:

+++ b/core/modules/toolbar/tests/modules/toolbar_test/toolbar_test.moduleundefined
@@ -6,21 +6,23 @@
+    l(t('link 1'), '<front>'),
+    l(t('link 2'), '<front>'),
+    l(t('link 3'), '<front>'),
...
+      '#title' => t('Test tab'),

Do we really need t() here? I'm not sure about it.

+++ b/core/modules/toolbar/toolbar.moduleundefined
@@ -269,82 +278,106 @@ function toolbar_toolbar() {
+  // @todo Maybe define a new type with these defaults using
+  // hook_element_info().
...
+    // @todo Maybe merge in $tab_defaults['options'] separately. Or use
+    // array_merge_recursive().

There shouldn't be a @todo in the code, but a follow-up issue or a fix.

+++ b/core/modules/toolbar/toolbar.moduleundefined
@@ -269,82 +278,106 @@ function toolbar_toolbar() {
+      // Provide an id, a data attribute linking each tab to the corresponding
+      // tray and aria information. If another module defines these in
+      // hook_toolbar(), it had better know what it is doing.

Should be ARIA instead of aria?

+++ b/core/modules/toolbar/toolbar.moduleundefined
@@ -269,82 +278,106 @@ function toolbar_toolbar() {
+    // Load the breakpoints for toolbar.
+    // foreach ($breakpoints->breakpoints as $key => $breakpoint) {
+    //   $build['#attached']['js'][0]['data']['toolbar']['breakpoints'][$key] = $breakpoint->mediaQuery;
+    // }

Commented code should be removed.

Very great job BTW. And I am really learning from your "self-talks", thanks for taking time for documenting your way to understanding too :-)

benjifisher’s picture

Status: Needs review » Needs work

@penyaskito:

Thanks for the review! Maybe it is too early to set the status to NR; I will set it back to NW. I certainly want to make further changes before the code is committed.

I consider "talking to myself" to be part of the documentation as well as an invitation for feedback. I am glad you find it helpful.

  1. DrupalCS suggested t(). This is text that will be displayed on the site.
  2. I will resolve the todo's before suggesting that the code be committed.
  3. I have almost no idea what this aria/ARIA stuff is. Can you give me a link to a quick summary? That comment is already in 8.x, and I think that @jessebeach added it.
  4. Absolutely, I should remove the commented-out stuff before the patch is committed. I left them in for now because I am hoping for some feedback on my questions in #15 about using array_map() or foreach().

Before the code is committed, we need a module that tests it. I am working on that, now. That should serve as a basis for testing this patch, updating toolbar.api.php, and updating the tests. When all that is done, I will try to add the module to the Examples project.

benjifisher’s picture

FileSize
14.51 KB

I have set up a sandbox project for a testing/example module for the toolbar hooks: http://drupal.org/sandbox/benji/1862108.

I would like to have help on the sandbox project from someone who knows CSS, icons, and javascript better than I do. For example, I implemented the Countcount example I described in #9 above. I am happy to report that it works as I hoped it would, but I do not plan to refine it. It could use some love in these three areas. See the screen shot: the "0, 1, 2, Many" should be a drop-down list, and the form could be prettier.

One thing I noticed is that the PHP sorting functions are not stable. If several tabs have the same weight (or none, which is equivalent to having weight 0) then they are sorted unpredictably. I think this is a problem. What is the best solution? My initial thought is to sort by link title (case insensitive).


toolbar with "How many?" tray exposed.

jessebeach’s picture

Sorry for the delay in responding. Let me go through the work you've done and how I extended it.

I added the array_map() line above, replacing the ones below, which seem to have been added back. I am pretty sure they have the same effect.

Using array_map is much cleaner. I removed the foreach code.

<!-- Tabs -->
<h2 class="element-invisible">Toolbar</h2>

I'm not sure that we need this h2 from an accessibility standpoint. Not having it didn't come up as an issue when we did a11y reviews in Toronto. I've removed it for now.

Now, to the changes I've introduced. I really wanted to get away from called theme_links on the tabs. If we're going to put content in the bar like a search bar or a right-aligned tab, we need to give developers a way to opt out of creating a visually-styled tab and also a tab that controls a tray. Also, the tray cannot be necessary. To that end, I introduced a theme function for the tray toggle (a linke that controls a tray) and two theme wrapper functions to create a visual tab and a tray. None of these theme functions are necessary.

function toolbar_theme($existing, $type, $theme, $path) {
  $items['toolbar'] = array(
    'render element' => 'elements',
    'template' => 'toolbar',
  );
  $items['toolbar_tab_wrapper'] = array(
    'render element' => 'element',
  );
  $items['toolbar_tray_toggle'] = array(
    'variables' => array('text' => NULL, 'path' => NULL, 'options' => array(), 'toolbar_identifier' => NULL),
  );
  $items['toolbar_tray_wrapper'] = array(
    'render element' => 'element',
  );
  return $items;
}

This is how the User module implements the theme functions.

$items['user'] = array(
  "#theme_wrappers" => array('toolbar_tab_wrapper'),
  '#theme' => 'toolbar_tray_toggle',
  '#text' => user_format_name($user),
  '#path' => 'user',
  '#options' => array(
    'html' => FALSE,
    'attributes' => array(
      'title' => t('My account'),
      'class' => array('icon', 'icon-user'),
    ),
  ),
  '#tray' => array(
    '#theme_wrappers' => array('toolbar_tray_wrapper'),
    '#heading' => t('User account actions'),
    'user_links' => array(
      '#theme' => 'links__toolbar_user',
      '#links' => $links,
      '#attributes' => array(
        'class' => array('menu',),
      ),
    ),
  ),
  '#weight' => 100,
);

When we preserve the renderable nature of the content, we can leverage the theme layer to sort the tabs, thus I changed the weight property to #weight and removed the ua sort from toolbar_view. This function becomes much less complicated as well.

The bulk of the tray and tab processing moves to template_preprocess_toolbar where users can alter the output with more ease.

The tray toggle link attributes move to template_preprocess_toolbar_tray_toggle, where again, they can be altered before render.

I tested the changes with Edit module #1824500: In-place editing for Fields in a followup issue that will be blocked on this one #1860436: Remove toolbar-specific code from the Edit module. The changes proposed in this patch work splendidly.

I also added a search box with a #weight of 150 to the bar and it came through without a problem. So the search module could theoretically implement hook_toolbar and put a global search on the page.

benjifisher, are these changes at all in line with your Countcount use case? Hopefully the changes to Shortcut and User provide sufficient example.

benjifisher’s picture

Wow, that is a lot to respond to. I will start with the easy stuff.

I am glad we agree on array_map() vs. foreach().

In addition to removing the <h2> from the template file, you have removed all the <h3> elements. I am not sure whether they belong in the template file or elsewhere, but I like the idea of keeping all the h2's and h3's in the same place. I defer to others on a11y issues.

Looking through the interdiff, I noticed this line:

    if (in_array('#tray', array_keys($item)) && isset($item['#tray'])) {

I think it is simpler, and probably more efficient, to use

    if (array_key_exists('#tray', $item) && isset($item['#tray'])) {

Either way, I am not sure we need the second condition. Is there a chance that the key is there, but $item['#tray'] is set to NULL?


So much for the easy stuff.


The interdiff is larger than the regular patch. I think we have two very different approaches.

I really wanted to get away from called theme_links on the tabs.

I completely agree. My first patch on this issue includes

-    '#theme' => 'links',
-    '#links' => array(),
+    '#theme' => 'item_list',
+    '#type' => 'ul',
+    '#items' => array(),

As I have said before, I think that allowing a tab to be any render element gives modules complete flexibility. I may think differently after studying the patch in #21, but my initial impression is that, using it, module writers would have to learn the toolbar-specific elements and theme functions instead of using generic render elements.


The main work in going from the patch in #14 to the one in #16 was to implement the idea outlined in #13: do some processing after invoking hook_toolbar() and before invoking hook_toolbar_alter(). The patch in #21 goes back to

  // Get toolbar items from all modules that implement hook_toolbar().
  $toolbar_items = module_invoke_all('toolbar');
  // Allow for altering of hook_toolbar().
  drupal_alter('toolbar', $toolbar_items);

I do not see how I can get the Countcount example to work using this structure.


Let's compare how a module can add a search field to the toolbar using the two patches. Based on the patch in #17, the toolbar_example module (screen shot in #20, code in my sandbox http://drupal.org/sandbox/benji/1862108) includes these lines:

  // The tray will display a simple form.
  $items['toolbar_example_count']['tray'] = array(
    '#type' => 'form',
    'element' => drupal_get_form('toolbar_example_count_form'),
  );

  // Functionally, this is completely redundant, but for sake of example, add a
  // form directly to the tabs section.
  $items['toolbar_example_form']['tab']
    = $items['toolbar_example_count']['tray'];

(Of course, those lines also add a form to the tray for a different tab.) @jessebeach, can you post the code you mentioned in #21 for comparison?


I do not think we want to defer sorting to the theme layer: see my remark in #20. OTOH, the decision to use 'weight' or '#weight' is an important one, not just whether to use drupal_sort_weight() or element_sort(). It is really a conceptual question of whether our array is a render element (with '#weight' as one of its keys) or a more generic array. I think we should use render elements at some level, but I have been wrestling with the question of whether we should have one big element with keys '#tab', '#tray', and '#weight' or whether 'tab' and 'tray' should be children. This decision affects how default values (defined in hook_element_info()) are merged in and the theme functions that we are exected to provide. I think I need a render guru.

benjifisher’s picture

Title: Refactor toolbar_view and the definition of the array returned by hook_toolbar » Refactor toolbar_view and the definition of the array returned by hook_toolbar

Get rid of the trailing space in the title ...

I think that render arrays should be modified in preprocess functions, not in theme functions, as in this (from the patch in #21)

function theme_toolbar_tray_wrapper(&$variables) {
  $element = $variables['element'];
  if (!empty($element['#children'])) {
    if (!empty($element['#toolbar_identifier'])) {
      $group = $element['#toolbar_identifier'];
      if (!isset($element['#attributes'])) {
        $element['#attributes'] = array();
      }
      $element['#attributes'] += array(
        'id' => 'toolbar-tray-' . $group,
        'data-toolbar-tray' => $group,
        'aria-owned-by' => 'toolbar-tab-' . $group,
      );
      $element['#attributes']['class'][] = 'tray';
      $element['#attributes']['class'][] = 'tray-' . $group;
      $element['#attributes']['class'][] = 'overlay-displace-top';
    }
    // Print a heading in the tray if one exists.
    $heading = (isset($element['#heading'])) ? '<h2 class="element-invisible">' . $element['#heading'] . '</h2>' : '';
    return '<div' . new Attribute($element['#attributes']) . '><div class="lining clearfix">' . $heading . $variables['element']['#children'] . '</div></div>';
  }
}

This is too late in the process for the Countcount strategy that I outlined in #13. I guess my module would have to add a preprocess function instead of implementing hook_toolbar_alter() if we use the approach in #21.


In general, adding theme functions for better separation of markup and processing is a good idea. We can easily add that structure on top of the patch in #17. But I do not like the idea of requiring the modules implementing hook_toolbar() to add these theme functions via the '#theme' key. That should happen in toolbar_view() (before invoking hook_toolbar_alter()) or in the preprocess function. Cf. my remarks in #22 about render elements.

One peculiarity of the approach used in #17 is how one module can add a tray to another's tab (or vice versa). This is probably an edge case anyway. It can be done using hook_toolbar() (since module_invoke_all() combined the results from different modules using array_merge_recursive()) but not in hook_toolbar_alter() (since that is after the processing that adds the connection between tab and tray).

benjifisher’s picture

Follow-up to my last point in #22:

After sleeping on it, I have decided that the tab and tray components of the array returned by hook_toolbar() should be properties, not children. The point is that we are looking for specific keys, and children can have any keys the module developer chooses.

So I think we should define one new element type: 'toolbar', or maybe 'toolbar_item' or maybe something involving 'tab' or 'bar'. A module that implements hook_toolbar() should return a keyed array of these elements. The general structure should be

function mymodule_toolbar() {
  $tab = array(/* any render element */);
  $tray = array(/* any render element */);
  $items['mymodule'] = array(
    '#type' => 'toolbar',
    '#tab' => $tab,
    '#tray' => $tray,
    '#weight' => 0,
    '#options' => array(),
  );

  return $items;
}

We can also add some other options in '#options'; I am not sure what the use cases are. My initial thought is that extra '#theme' or '#theme_wrappers' can be added to the child elements, but maybe this is a better place to specify them.


The patch in #21 give the module developer the responsibility for adding the appropriate theme wrapper if a tab is meant to activate a tray (rather than be a simple link). The patch in #17 makes that decision based on whether a tray is defined as part of the same element, and a module can override that decision using hook_toolbar_alter(). I prefer the approach that makes the module developer's job easier.

I have also been thinking that we might go back to @jessebeach's earlier design of allowing each tab/tray combination have its own flag (or set of breakpoints) for switching between horizontal and vertical. At narrow screen widths, I find that the long admin menu is best viewed vertically, but trays with only one or two items leave a lot of white space. This could be one use of '#options'.

jessebeach’s picture

Great thoughts in #23 and #24. First, I moved much of the logic in theme_toolbar_tray_wrapper to a preprocess function. See 1847198_toolbar-dx-preprocess-fn_21_do-not-test.patch. Good call.

I changed the tray headings back to <h3>.

The big change in this patch is moving tab render arrays under the #tab key. I'm intrigued at the notion, although I don't think I've gotten it right yet. Mostly, it ends up creating an awkward processing block in toolbar_preprocess_toolbar

foreach($variables['elements']['#items'] as $key => $item) {
  $id = drupal_html_class($key);
  if (array_key_exists('#tab', $item) && isset($item['#tab'])) {
    if (!isset($item['#tab']['#theme_wrappers'])) {
      $item['#tab']['#theme_wrappers'] = array();
    }
    array_push($item['#tab']['#theme_wrappers'], 'toolbar_tab_wrapper');
    $item['#tab']['#theme'] = 'toolbar_tray_toggle';
    $variables['bar']['items'][$key] = $item['#tab'];
    $variables['bar']['items'][$key]['#toolbar_identifier'] = $id;
    unset($item['#tab']);
  }
  if (array_key_exists('#tray', $item) && isset($item['#tray'])) {
    if (!isset($item['#tray']['#theme_wrappers'])) {
       $item['#tray']['#theme_wrappers'] = array();
    }
    array_push($item['#tray']['#theme_wrappers'], 'toolbar_tray_wrapper');
    $variables['trays'][$key] = $item['#tray'];
    $variables['trays'][$key]['#toolbar_identifier'] = $id;
    unset($item['#tray']);
  }
  // Render any content that is not explicitly identified as a #tab or #tray
  // to the bar area.
  $variables['bar']['items'][] = $item;
}

Where a developer could include renderable content in the returned item and in #tab and both would end up making tabs. The use-case I'm trying to solve is represented in the included search module patch, which sits on top of the proposed patch in this issue.

I think what needs to happen is the user needs to pass '#theme' => 'toolbar_tab' with a #tab property and optional #tray property. Otherwise, we just pass the contents of the item passed to hook_toolbar to the whims of the render layer. I'm going to see what that looks like in a followup.

jessebeach’s picture

I totally horked up those patches in #25! Redoing and properly basing on 8.x this time.

jessebeach’s picture

FileSize
12.02 KB
28.57 KB

I played around with the idea of a theme_toolbar_item function and I ultimately ran into this problem: If we pass the processing of #tray and #tab content to a theme function below theme_toolbar, then we can conglomerate the "tabs" into the administration bar and the "trays" outside of that bar. For positioning reasons, the trays need to live outside the administration bar (the .bar element) in the DOM. The triage of elements in the bar (tabs) and trays needs to take place in toolbar_preprocess_toolbar.

Given that, I kinda like where I ended up in #26. It allows us to create the following type of content.

// Add a search field to the toolbar. The search field employs no toolbar
// module themeing functions.
$items['global_search'] = array(
  '#type' => 'search',
  '#attributes' => array(
    'placeholder' => t('Search the site'),
    'class' => array('search-global'),
  ),
  '#weight' => 200,
  // Custom CSS, JS or a library can be associated with the toolbar item.
  '#attached' => array(
    'css' => array(
      drupal_get_path('module', 'search') . '/css/search.base.css',
    ),
  ),
);

/**
 * The 'Home' tab is a simple link, that is wrapped in markup associated
 * with a visual tab styling.
 */
$items['home'] = array(
  '#theme_wrappers' => array('toolbar_tab_wrapper'),
  '#theme' => 'link',
  '#text' => t('Home'),
  '#path' => '<front>',
  '#options' => array(
    'html' => FALSE,
    'attributes' => array(
      'title' => t('Home page'),
      'class' => array('icon', 'icon-home'),
    ),
  ),
  '#weight' => -20,
);

/**
 * A tray may be associated with a tab.
 *
 * When the tab is activated, the tray will become visible, either in a
 * horizontal or vertical orientation on the screen.
 *
 * The tray should contain a renderable array. An option #heading property
 * can be passed. The text is written to a heading tag in the tray as a
 * landmark for accessibility.
 */
$items['commerce'] = array(
  "#theme_wrappers" => array('toolbar_tab_wrapper'),
  '#theme' => 'toolbar_tray_toggle',
  '#text' => t('Shopping cart'),
  '#path' => '/cart',
  '#options' => array(
    'html' => FALSE,
    'attributes' => array(
      'title' => t('Shopping cart'),
    ),
  ),
  '#tray' => array(
    '#theme_wrappers' => array('toolbar_tray_wrapper'),
    '#heading' => t('Shopping cart actions'),
    'shopping_cart' => array(
      '#theme' => 'item_list',
      '#items' => array( /* An item list renderable array */ ),
    ),
  ),
  '#weight' => 150,
);

/**
 * The #tray can be used to render arbritrary content.
 *
 * A renderable array passed to the #tray property will be rendered outside
 * the administration bar but within the containing toolbar element.
 *
 * If the default behavior and styling of a toolbar tray is not desired, one
 * can render content to the toolbar element and apply custom themeing and
 * behaviors.
 */
$items['user_messages'] = array(
  // Include the toolbar_tab_wrapper to style the link like a toolbar tab.
  // Exclude the theme wrapper if custom styling is desired.
  "#theme_wrappers" => array('toolbar_tab_wrapper'),
  '#text' => t('Messages'),
  '#path' => '/user/messages',
  '#options' => array(
    'html' => FALSE,
    'attributes' => array(
      'title' => t('Messages'),
    ),
  ),
  '#tray' => array(/* renderable content */),
  '#weight' => 125,
);

This patch is based on #21, ignoring #25.

benjifisher’s picture

@jessebeach:

Thanks for the extensive examples in #27. I think there are still two major differences between your approach and mine. The differences depend on each other.

First, my idea is to make both the tab and the tray renderable arrays. So I would modify your last example as

$items['user_messages'] = array(
  // Include the toolbar_tab_wrapper to style the link like a toolbar tab.
  // Exclude the theme wrapper if custom styling is desired.
  '#theme_wrappers' => array('toolbar_tab_wrapper'),
  '#tab' => array(
    '#type' => 'link',
    '#text' => t('Messages'),
    '#path' => '/user/messages',
    '#options' => array(
      'html' => FALSE,
      'attributes' => array(
        'title' => t('Messages'),
      ),
    ),
  ),
  '#tray' => array(/* renderable content */),
  '#weight' => 125,
);

Maybe the '#theme_wrappers' property should also belong to the '#tab' element; I am not sure.


Which is better? If we use your method, then we cannot use '#options' for anything toolbar-related, since too many other elements want to process this property. I guess my real objection to your approach is that you are mixing levels. The tab element should logically be nested (as I see it) with its own element type (link, form, search). It should also have its own properties, separated from those of the parent element. Off hand, I cannot think of any more functional distinction between the two structures.

The second, related difference in approach is that my patch in #17 does some processing between hook_toolbar() and hook_toolbar_alter(). That allows me to implement the Countcount example (screen shot in #20) with a few lines in hook_toolbar_alter();

 // Replace the "How many?" tab with three links; the third will open the tray.
 // The path toolbar_example/count/% has not yet been implemented.
 $many = $toolbar_groups['toolbar_example_count']['tab'];
 $many['#title'] = t('Many');
 $choices = array(
   l(0, 'toolbar_example/count/0'),
   l(1, 'toolbar_example/count/1'),
   l(2, 'toolbar_example/count/2'),
   $many,
 );
 $toolbar_groups['toolbar_example_count']['tab'] = array(
   '#theme' => 'item_list',
   '#items' => $choices,
 );

(This builds on the simple definitions in hook_toolbar(). The full code is at http://drupalcode.org/sandbox/benji/1862108.git/blob/refs/heads/8.x-1.x:....) Using your approach, I think that the Countcount module would have to work much harder to make its fourth link the one that opens the tray.

jessebeach’s picture

The content of the user_messages array is a renderable array, right? Just making sure I'm not off base here. We don't need to bury it in a #tab key.

$items['user_messages'] = array(
  "#theme_wrappers" => array('toolbar_tab_wrapper'),
/* This is renderable content */
  '#text' => t('Messages'),
  '#path' => '/user/messages',
  '#options' => array(
    'html' => FALSE,
    'attributes' => array(
      'title' => t('Messages'),
    ),
  ),
/* end top-level renderable content */
  '#tray' => array(/* renderable content */),
  '#weight' => 125,
);

The #tray is supplemental to the main renderable content of user_messages in this example. If we bury renderable content under #tab then we get ourselves into a situation where we might have two tabs for every item passed back to hook_toolbar: (1) the renderable content in the item array and (2) the renderable content inside of #tab. Does that make sense? I should be able to pass the following back to hook_toolbar and have it render.

$items['global_search'] = array(
  '#type' => 'search',
  '#attributes' => array(
    'placeholder' => t('Search the site'),
    'class' => array('search-global'),
  ),
  '#weight' => 200,
  // Custom CSS, JS or a library can be associated with the toolbar item.
  '#attached' => array(
    'css' => array(
      drupal_get_path('module', 'search') . '/css/search.base.css',
    ),
  ),
);
benjifisher’s picture

Maybe this is a better way to put my position: in your example $items['user_messages'], you have assigned properties '#text', '#path', and '#options'. You have not assigned '#type', but you want those three properties to be interpreted as if '#type' => 'link', so it looks to me as if you are using a link element. Yes, you can assign '#theme' and '#theme_wrapper' properties, but it seems to me that people will understand the code better if we use recognizable element types. (Maybe the existence of a '#type' property is what distinguishes "an element" from "renderable content"?)

OTOH, if you explicitly assign '#type' => 'link' (or assign it by default during processing), then you are in the unusual situation of using a link element with the additional property of '#tray', which is ignored by theme_link. Not to mention '#weight'.

You refer to my suggestion as "burying" the renderable content. I don't know, is there any rule against making a render element the value of an element property? Is there precedent?

To repeat a couple of things I said before:

  1. I think I need a render guru.
  2. ...I cannot think of any more functional distinction between the two structures.
jessebeach’s picture

Aha! I think we're on the same page now!

Ok, so in my example of $items['user_messages'], I'M TOTALLY MISSING '#theme' => 'link'. MY BAD. Oy, very sorry for the confusion, it should be

$items['user_messages'] = array(
  "#theme_wrappers" => array('toolbar_tab_wrapper'),
/* This is renderable content */
  '#theme' => 'link',
  '#text' => t('Messages'),
  '#path' => '/user/messages',
  '#options' => array(
    'html' => FALSE,
    'attributes' => array(
      'title' => t('Messages'),
    ),
  ),
/* end top-level renderable content */
  '#tray' => array(/* renderable content */),
  '#weight' => 125,
);

Then, on to

OTOH, if you explicitly assign '#type' => 'link' (or assign it by default during processing), then you are in the unusual situation of using a link element with the additional property of '#tray', which is ignored by theme_link. Not to mention '#weight'.

I see your point here. Let me see if I can rustle up some opinions on this issue here at work.

jessebeach’s picture

benjifisher, I chatted with moshe about using hook_element_info, which you've been alluding to. I'm going to explore that option and we can hack on that a bit as an alternative?

benjifisher’s picture

jessebeach:

I really wish @sun would chime in. He is the one who seems to care most about being able to use the structure here to modify or extend or apply the toolbar.

I think you are more likely to find a render guru among your coworkers than I am.


IIUC, the point of using hook_element_info() would be to "register" a new element type. (As I said before, it also affects how default values are merged in and makes us responsible for defining a theme function.)


There is also the issue of the Countcount example. I admit it is contrived, but I have a hunch that someone will come up with a compelling use case that follows the same pattern. I want a "complex" render array (in this example, a list of four links) and somewhere inside it is a link that activates the tray. In the patch for #27, the '#toolbar_identifier' property is added in template_preprocess_toolbar(). I think the author of the Countcount module would have to add a preprocess function to move that property around, and this would require knowing the internals of the Toolbar module. Contrast that with my code snippet in #28, using hook_toolbar_alter().

I think this is a harder thing to change than whether the tab element is nested inside a '#tab' property.

benjifisher’s picture

I have been wondering about the comment in the documentation block

/**
 * Provides pre_render function for the toolbar.
 *
 * Since building the toolbar takes some time, it is done just prior to
 * rendering to ensure that it is built only if it will be displayed.
 *
 * @see toolbar_page_build().
 */
function toolbar_pre_render($toolbar) {
  $toolbar = array_merge($toolbar, toolbar_view());
  return $toolbar;
}

I finally checked the commit message and the corresponding issue: #612974: Optimize toolbar to only build if it will be displayed. I think we should add a comment to toolbar.api.php to the effect that the toolbar is built after hook_page_alter() is invoked. If a module wants to add elements to the toolbar or change its '#access' property, the module can use hook_page_alter(), but this hook cannot be used to modify the completed toolbar.


I still do not see why we need a separate function toolbar_view() (used to be toolbar_build()). Why not move that code into toolbar_pre_render()?

jessebeach’s picture

That code predates the current Toolbar update. It it makes sense now to change it, it's there for us to propose.

benjifisher’s picture

@jessebeach:

Right, the issue I referenced in #34 was from 2009.

The attached patch is (almost) the minimal change to #27 that I think will make the Countcount module possible. It moves a few lines from template_preprocess_toolbar() to toolbar_view(). I need some sleep, so I have not done any testing at all.

If you accept this patch, then I think we still disagree on two points. Both of these affect the API that the module provides, but neither makes much difference (if any) to the functionality. (edit) Make that three.

  1. I think that toolbar_view() should insert the '#theme_wrappers' property where needed, so that modules implementing hook_toolbar() are not responsible for it.
  2. I think that '#tab' and '#tray' should both be nested inside the element returned by hook_toolbar().
  3. Sorting

The first point makes it easier for one module to define a tab and another to hijack that tab to open its own tray. That is an edge case, and an even edgier case is an API module that defines a tray in case some other module wants to attach a tab to it.

I will try again to explain why I think the second point is the right way to go. If '#tray' is a property of the render element that defines the tab, then we allow any render element to be used for a tab unless the element type uses a '#tab' property to mean something else. It may never come up, but I would feel silly documenting such an exception.

For sorting, I do not have time to run an example and generate screen shots—and I will not have any time until Friday afternoon—but to see what I mean by "unpredictable sort order," try

$link = array(
  '#theme_wrappers' => array('toolbar_tab_wrapper'),
  '#theme' => 'link',
  '#path' => '<front>',
  '#options' => array(
    'html' => FALSE,
    'attributes' => array(
      'title' => t('alphabet'),
    ),
  ),
);
$items['a'] = $link + array('#text' => t('A'));
$items['b'] = $link + array('#text' => t('B'));
$items['c'] = $link + array('#text' => t('C'));
$items['d'] = $link + array('#text' => t('D'));
$items['e'] = $link + array('#text' => t('E'));

Edit: Added sorting to the ordered list and discussion at the end.

jessebeach’s picture

If '#tray' is a property of the render element that defines the tab, then we allow any render element to be used for a tab *unless* the element type uses a '#tab' property to mean something else. It may never come up, but I would feel silly documenting such an exception.

Right, right, right. I'm working on implementing a 'toolbar_tray_combo" element through hook_element_info. I think that the end result of this work will be the ability to define a tray for any render element. So the second element in a list such as in your Countcount example. I'm 25% done (about) with this approach. I hope to have a patch posted today given my meeting load for the day doesn't impede too much.

jessebeach’s picture

Major Changes

It took me a little longer to finesse the details, but I've got a patch I think we can work with. I ran these changes past Moshe at several points in the evolution of the patch before I finally landed on what's here.

What this patch does is remove the highly idiosyncratic processing of the hook_toolbar return values. We keep the structure of the renderable array passed back from the modules. But, because the trays need to be physically outside the .bar element in the DOM so their position context isn't a fixed element, we move them in JavaScript instead.

...
$trays = $toolbar.find('.tray');
$trays
  // Move the trays outside of the bar.
   .insertAfter($toolbar.find('.bar'))
  ...

The trays are display:none by default, so we don't incur a repaint when they're moved.

The root of the changes in this patch is the toolbar_item element defined in toolbar_element_info.

// A toolbar item is wrapped in markup for common styling.  The 'tray'
// property contains a renderable array. theme_toolbar_tab() is a light
// wrapper around the l() function. The contents of tray are rendered in
// theme_toolbar_tab().
$elements['toolbar_item'] = array(
  '#pre_render' => array('toolbar_pre_render_item'),
  // A light wrapper for the l() function.
  '#theme' => 'toolbar_tab',
  '#theme_wrappers' => array('toolbar_tab_wrapper'),
  '#text' => NULL,
  '#path' => '',
  // Standardize element attributes on the #attributes property.
  '#attributes' => array(),
  // l() takes an #options parameter.
  '#options' => array(
    'html' => FALSE,
    'attributes' => array(),
  ),
  'tray' => array(
    '#heading' => '',
  ),
);

Modules can declare this type when registering components into the toolbar in hook_toolbar.

A toolbar item without a tray

/**
 * The 'Home' tab is a simple link, that is wrapped in markup associated
 * with a visual tab styling.
 */
$items['home'] = array(
  '#type' => 'toolbar_item',
  '#text' => t('Home'),
  '#path' => '<front>',
  '#options' => array(
    'html' => FALSE,
    'attributes' => array(
      'title' => t('Home page'),
      'class' => array('icon', 'icon-home'),
    ),
  ),
  '#weight' => -20,
);

A toolbar item with a tray

/**
 * A tray may be associated with a tab.
 *
 * When the tab is activated, the tray will become visible, either in a
 * horizontal or vertical orientation on the screen.
 *
 * The tray should contain a renderable array. An option #heading property
 * can be passed. The text is written to a heading tag in the tray as a
 * landmark for accessibility.
 */
$items['commerce'] = array(
  "#type" => 'toolbar_item',
  '#text' => t('Shopping cart'),
  '#path' => '/cart',
  '#options' => array(
    'html' => FALSE,
    'attributes' => array(
      'title' => t('Shopping cart'),
    ),
  ),
  'tray' => array(
    '#heading' => t('Shopping cart actions'),
    'shopping_cart' => array(
      '#theme' => 'item_list',
      '#items' => array( /* An item list renderable array */ ),
    ),
  ),
  '#weight' => 150,
);

A toolbar item with a tray and custom styling

/**
 * The #tray can be used to render arbritrary content.
 *
 * A renderable array passed to the 'tray' property will be rendered outside
 * the administration bar but within the containing toolbar element.
 *
 * If the default behavior and styling of a toolbar tray is not desired, one
 * can render content to the toolbar element and apply custom themeing and
 * behaviors.
 */
$items['user_messages'] = array(
  // Include the toolbar_tab_wrapper to style the link like a toolbar tab.
  // Exclude the theme wrapper if custom styling is desired.
  "#type" => 'toolbar_item',
  '#theme' => 'user_message_toolbar_tab',
  '#theme_wrappers' => array(),
  '#text' => t('Messages'),
  '#path' => '/user/messages',
  '#options' => array(
    'html' => FALSE,
    'attributes' => array(
      'title' => t('Messages'),
    ),
  ),
  'tray' => array(
    '#heading' => t('User messages'),
    'messages' => array(/* renderable content */),
  ),
  '#weight' => 125,
);

The code also let's you implement your Countcount example without resorting to hook_toolbar_alter. I've provided two patches to your module in this issue that updates Countcount to use the new element defintion. One just updates the code to use the new element definition; the other removes the hook_toolbar_alter code to get your example of a item list in the toolbar where the fourth item of the list opens a tray. This can be done right from hook_toolbar now.

Summary

To get a toolbar tab with an associated tray, a developer will need to do 3 things.

  1. Declare the #type of a render array as toolbar_item.
  2. Structure this renderable array with keys that correspond to l() (unless a custom #theme function is provided).
  3. Provide a renderable array in a key labeled tray.

Other small changes

  • Removed the toolbar_view function as mentioned in #34.
  • Introduced an element toolbar in toolbar_element_info that contains the defaults for a renderable toolbar element.
  • Updated toolbar.api.php
  • Updated the test.
  • Refactored toolbar.module so that similar functions are grouped. It had gotten quite jumbled.
  • (edit) toolbar.twig is removed in favor of theme_toolbar

Patches

toolbar-refactor-1847198-38.patch
The proposed patch.
toolbar-refactor-1847198-27-to-38-do-not-test.patch
The significant changes between #27 and #38. Does not include the refactoring of toolbar.module to group functions.
countcount-module-before-removing-hook_toolbar_alter.patch
Patch to the Countcount module to bring it in line with the changes proposed in this patch.
countcount-module-after-removing-hook_toolbar_alter.patch
Patch to the Countcount module to bring it in line with the changes proposed in this patch. The code to shift a tray around in hook_toolbar_alter has been removed. Apply this patch after the previous Countcount patch.

Status: Needs review » Needs work

The last submitted patch, countcount-module-removed-hook_toolbar_alter-jessebeach.patch, failed testing.

benjifisher’s picture

FileSize
15.15 KB

I will make a bunch of minor observations as I test. I will follow up with another comment after I have a chance to digest these changes (maybe tomorrow).

Minor point: I tried to apply the incremental patch on top of the one from #27. The patch failed.


I applied the two patches to the toolbar_example module. I will commit them to my sandbox: http://drupal.org/sandbox/benji/1862108.

I note that the "Click me!" link is a bare <a> element: since the module does not specify '#type' => 'toolbar_item' (whether by design or by accident) it is not wrapped in the same <div class="tab"> as the other items, and it lacks the attributes they get.

Originally, the "0, 1, 2, How many" links displayed horizontally, but now they are vertical. I do not really care: the example module is just an example, and in real life the module would be responsible for styling something this unusual.

The id attributes of the tabs and trays are now distinguished numerically, instead of by the keys returned by hook_toolbar(). I do not like this: how is a module supposed to provide custom styling for the elements it defines?

The new version of the example module does not show how to add a form to the toolbar. I tried restoring the deleted lines, and it did not work. (Edit: I tried a little harder, and it worked.)


I added the alphabet example from #36 to the example module (and updated to use '#type' instead of '#theme_wrappers'). It is about as bad as I expected. I think we need to sort the toolbar ourselves.
screenshot of toolbar with example module and ABCDE links


I think that using a '#type' property instead of '#theme_wrappers' is a big improvement.

I do not like the idea of using javascript to separate tabs from trays. The only practical effect I can think of is very small: if I understand the caching strategy correctly, then we are asking the browser to do a little extra work when the page is reloaded.

If a tray is opened and the page reloads, I guess the server returns HTML with all trays in the default, display="none", state; with the patch from #38, they are rearranged through JS; and then (with or without the patch) the tray is opened. If I have it right, then I understand your claim that this patch does not cause a repaint.


I am not sure whether the toolbar_item type should use the key 'tray' (a child element) or '#tray' (a property or a variable for the theme function).

benjifisher’s picture

@jessebeach:

From #38:

What this patch does is remove the highly idiosyncratic processing of the hook_toolbar return values. ... we move them in JavaScript instead.

An alternative is to use render() or drupal_render() (depending on where in the process we do it) on the tray elements. This will mark them as "printed", so we can then render the overall array and they will not be output a second time.

A similar approach would be to use hide() and show() on the trays. This might make it simpler to render the result in the order we want.

Back-end issues like this can be left for follow-up issues if we like. The part of this issue that is blocking others is agreeing on the structure for toolbar_item. I am still mulling it over.

benjifisher’s picture

Sorting:

As I said in #36 and #40, I think we have to have a custom sort. My initial idea was to sort by title (after the primary sort by weight). That has some advantages and some disadvantages. I just thought of another disadvantage, which I think is decisive: if the site is translated, then the '#title' properties may change, leading to a different sort order. For predictable results, I think we should sort by the keys returned by hook_toolbar(). This will also be simpler to implement than my original idea.

benjifisher’s picture

OK, on to the main design decision. What is the structure that should be returned by hook_toolbar()?

First of all, I think the patch in #38 meets the standard of "I can live with it." It is flexible enough to handle all the use cases we have come up with.

However, I think the standard for this issue should be "Do we get it right?" rather than "Can I live with it?" We are designing an API to be used by other modules.

@jessebeach, I hope that one of us can win over the other.


I agree that defining the toolbar_element type is the right way to go. We agree that the tray should be a nested element. I am not sure whether it should be a child element or a property ('tray' or '#tray').

The question is whether the tab should also be nested, as in #17, or if it should be the main content of the element, as in #38 and others. I think it should be nested.


One way to decide is to make it work like other hooks. I think that hook_block_view() is a good analogy. Both 'title' and 'content' are children elements. It is not a perfect analogy: there is no '#type' => 'block' that I know of.


One thing you cannot do with the structure in #38 is assign a type to make your tab element render as a link, button, form, or other element. There can be only one type, and we have to set it to 'toolbar_item' (at least if we want to associate it to a tray).

OK, you get around the problem of not being able to specify a '#type' by specifying a '#theme'. Or maybe a '#theme_wrappers'. At some level, what specifying a type does for you is that it provides default values of these properties, via hook_element_info().

This is a maintainability issue. If I write a contrib module and specify '#type' => 'magic_pony', then the immediate benefit is that I let the Magic Pony module figure out what theme properties should be specified. Or process or preprocess properties. The long-term benefit is that the Magic Pony module can change its implementation, and I automatically get the the right theme wrappers applied. If I cannot specify '#type' => 'magic_pony', then I will have to update my module when this happens.

jessebeach’s picture

The id attributes of the tabs and trays are now distinguished numerically, instead of by the keys returned by hook_toolbar(). I do not like this: how is a module supposed to provide custom styling for the elements it defines?

A module needs to pass in attributes to identify its renderables. theme_item_list doesn't add id information to the list about the module that called it for example. If a renderable with an associated tray is found deep within the array that a module returns to hook_toolbar then we can't use the render system as-is to produce the output. We would need to parse the renderable and find instances of some special array key and add information to that array about its controlling module. This is not ideal and I struggled for hours to come to terms with this issue, ultimately deciding that toolbar can't be responsible for marking up content passed to it -- the calling module must do this.

Why not just sort by weight and let the sort functions operate as they do? If fine-grained sorting is needed, then a module needs to pass in a #weight attribute.

I think a module that implements hook_toolbar and passes a renderable with '#type' => 'magic_pony' can still do so. If that type needs a tray, then it will need to provide the correct HTML classing and data-* attributes to associate a tray trigger with the tray element. The trays work off of HTML attributes -- nothing that a module couldn't provide on its own. The toolbar_item has the advantage of making these connections automatically. But if a module really needs to go off the reservation, I don't think it's unreasonable that core doesn't support such an edge case.

It's a bit late and I'm not completely following on the render() or drupal_render() issue you mention in #41, but it sounds like you're alerting to an issue with #children not being set by drupal_render_children?

Using hide() and show() is an interesting approach. I'd thought of it but I just couldn't make it work, since a tray might found deep in the structure of a renderable, such as in the Countcount module. You need a predictable structure to use hide() and show() and we can't rely on or enforce that and also allow for arbitrary content to be passed back to hook_toolbar. I'm happy to be proved wrong or shown a better way here!

The reason I think that #tab should not be a called-out thing (with an array key) is because I adding content to the toolbar should be possible just by passing a renderable array to hook_toolbar. If I can pass a renderable that also contains #tab, I run the risk of defined two toolbar items with one renderable. Does that make sense?

benjifisher’s picture

@jessebeach:

This time I will start with the key point. We are not forced to render arbitrary content defined by hook_toolbar(), because theme('toolbar_item') has the final say. See the description of the '#theme' key on http://drupal.org/node/930760 (near the end of the main content, about half-way down the page counting the current comments). As I said, hook_block_view() is analogous: a module can return any sort of renderable content, but only the 'title' and 'content' children get rendered.

I will respond to the rest of your points in order. Several decisions depend on the key decision of how to structure the toolbar_item type.


Yes, a module can define its own CSS classes and style them, but there may be cases where styling has to be applied to the wrapping elements.

Looking at the markup again, I think the current patch applies an id attribute to the tab element, overriding any that might be set by the module. I would like to see that id (such as toolbar-tab-toolbar-tray--11) applied to the wrapping div with class="tab". OTOH, the tray has the id attribute attached to the wrapping div.

If the tab and tray are both top-level children of the toolbar_item, then it should be easy to give them (that is, the wrapping div's) id attributes based on the key.


Individual modules should not be responsible for fine-grained sorting. That should be left to the site builder. One alternative is to offer an admin page where the site builder can rearrange the tabs. (This is definitely a follow-up issue!) That would be a good way for the Toolbar module to implement hook_toolbar_alter().

I feel strongly that we should encourage most modules to stick with the default (no '#weight' property, which is treated the same as '#weight' => 0) AND that the sort order should be predictable.


I agree that the Countcount example is an edge case, and that a module following this pattern should be expected to do extra work. I just think that the approach I suggested, using hook_toolbar_alter(), is simpler than the approach you suggest. The Toolbar module should be the only one that needs to know the details of the data-* attributes.

The same point I made about maintainability at the end of #43 applies here. If Toolbar changes its implementation, then I have to update my module. We avoid this problem by using '#type' => 'toolbar_item' in the top-level element and '#type' => 'magic_pony' in the 'tab' child element.

Secondary issue: as long as the sort is unpredictable, it is impossible for the module to set the data-* attributes itself.


No, I am not aware of a current problem with '#children' not being set. I was just suggesting alternative ways (render() or drupal_render() or hide() and show()) to separate the tray markup from the tab markup. I agree that this is impractical using the current structure, but if each toolbar_item element has 'tab' and 'tray' children, then it should be easy.

benjifisher’s picture

Issue summary: View changes

added images to the use cases

jessebeach’s picture

Yes, a module can define its own CSS classes and style them, but there may be cases where styling has to be applied to the wrapping elements.

Looking at the markup again, I think the current patch applies an id attribute to the tab element, overriding any that might be set by the module.

Correct, I tried to provide for this. As a renderable passes through the various stages of pre rendering, user-provided values are preferred to theme-provided values. So if you set an id or a class on a renderable, that id will be used instead of the one produced by the default functions.

I also provided a key #wrapper_attributes that a developer can use to pass attributes to the wrapping functions.

Both these issues of overrideability came up as I was testing the use cases we defined in the summary.

On the sorting issue, I'm happy to see something better than what's there, but personally I'm fine with the standard sorting behavior since any secondary sorting, as far as I can predict, will be based on assumptions that may not be true in all cases i.e. that items of equal weight should be sorted by alpha on title.

Secondary issue: as long as the sort is unpredictable, it is impossible for the module to set the data-* attributes itself.

Sorting will have nothing to do with the data-* attributes. As long as the data-toolbar-tray attribute is the same on the tray and its trigger, the JavaScript will associate them, regardless of their placement in the DOM.

benjifisher’s picture

So if you set an id or a class on a renderable, that id will be used instead of the one produced by the default functions.

In that case, I retract the objection that modules cannot set the data-* attributes. When I wrote that, I was just looking at the markup, not the code, and what I saw there is that the data-* attributes involved the numerical sort order.

benjifisher’s picture

Following up on one of my points from #45, I added #1869638: Make the menu shown in the administration menu tray configurable.

As I contemplate building a configuration page for the toolbar, it occurs to me that hook_toolbar() should define '#title' (or '#name') and '#description' properties for each item it returns. These may often be the same as the corresponding properties for the tab, but I do not think we can rely on that. This is one more reason that IMO the tab should be a child element of the toolbar_item.

jessebeach’s picture

benjifisher, if we place the "tab" content under a #tab key, what would we do in this case?

$items['global_search'] = array(
  '#type' => 'search',
  '#attributes' => array(
    'placeholder' => t('Search the site'),
    'class' => array('search-global'),
  ),
  '#tab' => array(
    '#type' => 'toolbar_item',
    '#text' => t('Home'),
    '#path' => '<front>',
    '#options' => array(
      'html' => FALSE,
      'attributes' => array(
        'title' => t('Home page'),
        'class' => array('icon', 'icon-home'),
      ),
    ),
    '#weight' => -20,
  ),
  '#weight' => 200,
  // Custom CSS, JS or a library can be associated with the toolbar item.
  '#attached' => array(
    'css' => array(
      drupal_get_path('module', 'search') . '/css/search.base.css',
    ),
  ),
);

Do we render both the search box and the home link "tab?" So each module can place two items in the toolbar with each keyed item returned to hook_toolbar?

This is the case I'm stuck on for the reason I don't think we can bury the content that should be rendered to the toolbar in a keyed array inside the array that we pass back to hook_toolbar. The contents of the array passed back to hook_toolbar should be the thing rendered to the toolbar. The fact that it has a tray is completely ancillary.

That all being said, your use case in #48 for configuration makes sense. If we make hook_toolbar more like hook_menu then it makes sense that the array passed back to hook_toolbar has a more rigid array key definition. I can back that. I'm reluctant to over-optimize the code in this issue, though. I'd rather leave configuration to its own issue and adjust the signature of the return array in followup issue when the focus is specifically configuration.

The patch in this issue makes hook_toolbar more of a pass-through for renderables and tries not to make any presumptions about the structure of the renderable except for the tray key. We're still in feature completion, we can continue to refine the code as use cases mount. What do you think?

benjifisher’s picture

@jessebeach:

The example that worries you is inside out. As I see it, $items['global_search'] should have '#type' => 'toolbar_item'. If you want a search box to display in the toolbar, then $items['global_search']['#tab']['#type'] = 'search';; If you want a link, then $items['global_search']['#tab']['#type'] = 'link';.

As I said in #46 (first point) once we require hook_toolbar() to return items of type toolbar_item, we get to decide what gets rendered because we get to choose the theme function. The attached patch shows how to do this: I have removed theme_toolbar_tab() and added theme_toolbar_item(). This function does nothing but render the '#tab' property, leaving the '#tray' for later.


I agree that we should leave configuration for later. That is why I added #1869638: Make the menu shown in the administration menu tray configurable as a separate issue. I am not sure what you mean by "adjust[ing] the signature of the return array", but I think I disagree. As I see it, the point of this issue is to finalize the API. I think the configuration page is just another use case, and we should design the API with it in mind.


The patch in this issue makes hook_toolbar more of a pass-through for renderables and tries not to make any presumptions about the structure of the renderable except for the tray key.

I think we agree on the goal. It seems to me that your theme_toolbar_tab() does make a presumption that the tab is a link element.


I am still not sure whether '#tab' and '#tray' should be properties or children. If we are going to define defaults for either one in hook_element_info(), then I think they should be properties. I looked at all the core implementations of hook_element_info(), and only field_ui_element_info() defines a default value for a child. That looks to me like a mistake rather than a precedent.


The attached patch does not do much more than necessary to make the tab a nested property inside the toolbar item. There are a lot of other things I would like to change, but first let's agree on the API. There are certainly comments and tests to be updated, plus the .api.php file. Let me mention just one more thing for now: the line

  $element['#children'] = drupal_render_children($element);

inside toolbar_pre_render(). It seems to me that rendering inside a pre-render function short-circuits the usual rendering process.

benjifisher’s picture

Status: Needs work » Needs review
FileSize
8.53 KB
43.64 KB

I am setting this to NR to see what the testbot thinks. I still think the patch needs work.

I have also attached an incremental patch (interdiff).

The attached patch does two things. First, it updates the test module and the API documentation to be consistent with the patch in #50 (see below). Second, it uses PHP instead of javascript to separate the tabs from the trays, undoing oart of the change made in #38.

@jessebeach, I tested briefly with the ".tray" div's inside the "#toolbar-bar" div, and it seemed to work properly. At what point do you see a problem?


I guess I have not been clear about the structure I am suggesting, with a toolbar-item render element having '#tab' and '#tray' properties. To be explicit, here is a snippet from my version of user.module:

  $items['user'] = array(
    '#type' => 'toolbar_item',
    '#tab' => array(
      '#type' => 'link',
      '#title' => user_format_name($user),
      '#href' => 'user',
      '#options' => array(
        'attributes' => array(
          'title' => t('My account'),
          'class' => array('icon', 'icon-user'),
        ),
      ),
    ),
    '#tray' => array(
      '#heading' => t('User account actions'),
      'user_links' => array(
        '#theme' => 'links__toolbar_user',
        '#links' => $links,
        '#attributes' => array(
          'class' => array('menu'),
        ),
      ),
    ),
    '#weight' => 100,
  );

I hope this clarifies what I meant when I called the snippet in #49 "inside out."

Looking at this again, I wonder whether the '#heading' property should be part of the '#tray' element or perhaps the parent element (type toolbar_item) should have a property '#tray_heading'.

Status: Needs review » Needs work
Issue tags: -toolbar-followup

The last submitted patch, toolbar-refactor-1847198-51.patch, failed testing.

jessebeach’s picture

Status: Needs work » Needs review
Issue tags: +toolbar-followup

#51: toolbar-refactor-1847198-51.patch queued for re-testing.

jessebeach’s picture

Tests are running fine locally. I requeued #51.

I'm ok with the patch in #51. Let's leave this as Needs Review for a week to let others chime in. @benjifisher, I think we've reached a point of consensus that allows us to move forward. Getting this code in and working with it will let us know if further tweaks are needed. Sound good?

Status: Needs review » Needs work

The last submitted patch, toolbar-refactor-1847198-51.patch, failed testing.

Bojhan’s picture

I see a whole bunch of CSS changes, does this have any UI changes?

benjifisher’s picture

Status: Needs work » Needs review

#38: toolbar-refactor-1847198-38.patch queued for re-testing.

I am getting the same failures locally as the testbot. Technically, not failures but exceptions. It is the breadcrumb test: I was not expecting a problem there.

I am now racing with the testbot: can I figure out what is going wrong before it finishes this test?

benjifisher’s picture

The problem is the Edit module, which added hook_toolbar() in #1824500: In-place editing for Fields. The attached patch is the same as #51 except that I have updated edit_toolbar().

benjifisher’s picture

@jessebeach:

Can you answer @Bojhan's question in #56? (and mine in #51, just before the <hr>)

There are a few changes I still want to make, though not necessarily before closing this issue:

  1. There are some changes I want to make in toolbar_pre_render(). I may have over-stated the problem at the end of #50, but I still want to change that, among other things.
  2. I still have not looked at the two countcount/hook_toolbar_alter() patches from #38.
  3. I want to re-opent the discussion of CSS classes from #45–47. A module writer can provide an id attribute and corresponding CSS, but what about a themer? Instead of just CSS, a themer has to implement hook_toolbar_alter(). (Can that be done from template.php?) In real life, most will just look at the markup and use the id there for CSS rules; some time later, another module adds an element to the toolbar and the numeric parts of the CSS classes change, and the themer wonders why things break. So I still like the idea of using the keys from hook_toolbar() as part of the CSS classes.

OK, #1 does not affect the API, so it can certainly wait for a follow-up issue. #2 and #3 both affect the API, so I would prefer to address them here. I am willing to go along with putting them in follow-up issues in the interests of helping the Edit module, especially if it seems likely that no one will be using hook_toolbar_alter() nor targeting the id tags with their CSS just yet.

benjifisher’s picture

Assigned: Unassigned » benjifisher

I am working on the issue summary.

jessebeach’s picture

RE #56, The CSS changes replace li with .tab. There are no visible effects of these changes.

I tested briefly with the ".tray" div's inside the "#toolbar-bar" div, and it seemed to work properly. At what point do you see a problem?

To position the trays reliably and consistently, we need the positioning context of the tray elements to be absolute. The #toolbar element that contains both the bar and the trays is set to absolute and this does not vary. The bar on the other hand, switches positioning from absolute to fixed depending on the media query. When the position of the bar is fixed, then so are the trays in effect; they won't scroll with the content and we need scrollbars inside the trays to deal with an overflow of vertical content.

I want to re-opent the discussion of CSS classes from #45–47. A module writer can provide an id attribute and corresponding CSS, but what about a themer? Instead of just CSS, a themer has to implement hook_toolbar_alter(). (Can that be done from template.php?) In real life, most will just look at the markup and use the id there for CSS rules; some time later, another module adds an element to the toolbar and the numeric parts of the CSS classes change, and the themer wonders why things break. So I still like the idea of using the keys from hook_toolbar() as part of the CSS classes.

I don't want to over-optimize the toolbar for themers without use cases for how and why they would need easy access to manipulate aspects of the Toolbar's structure and styling. Module developers might add a top-level item, but even this should be done only in cases when a top-level item is really warranted. Otherwise administrative links should be located in the administrative menu. Themers shouldn't be restyling the Drupal administrative toolbar under normal circumstances.

sun’s picture

Title: Refactor toolbar_view and the definition of the array returned by hook_toolbar » Refactor toolbar_view() and structure of hook_toolbar()

Thanks for working on this, and also for the elaborate discussion.

There are various goals and tasks involved in this issue, which haven't been summarized clearly yet, so let me give it a shot:

  • Allow to add a toolbar tab without a corresponding tray.
  • Allow to add a toolbar tab that is a direct link.
  • Allow to add a toolbar tab that is not a link. (which may or may not open a corresponding tray)
  • Allow to add a toolbar tab that is compound of multiple output/rendered elements. (of which potentially only certain sub-elements may or may not toggle a corresponding tray)
  • Allow to add a toolbar element that is not styled as a tab.
  • Allow to put other content into a toolbar tray that is not a list/tree of links.
  • Allow to force-position a particular toolbar tray aligned to the right instead of left window/viewport boundaries.
  • Allow to force the vertical orientation for a particular toolbar tray even if the user toggled the horizontal orientation.

My first and foremost recommendation is to take a step back and have a look at the situation we're facing architecturally:

  1. There's a container that contains arbitrary content, of which certain elements ["tabs"] may trigger/toggle/expose/disclose a block ["tray"] that is associated with the corresponding element.
  2. Each block ["tray"] contains arbitrary content. Like any other block.
  3. The content of a block is not necessarily special or explicitly prepared for appearing as a block that is associated to the container. Only the container elements (triggers) are widgets that are specially prepared to appear within the container.

This immediately leads to the conclusion that the trays should be entirely detached from the tabs. In other words, a tab only specifies which block should be rendered for it when it is toggled (if any). But the tabs/tray's block is not necessarily rendered in advance, which inherently means that its render structure should also not necessarily be built ahead of time.

This clear separation has a range of positive consequences: Tray blocks are just blocks; any block is suitable. Once Blocks/Layouts and WSCCI are fully in place, complex/large blocks (such as a fully rendered admin menu link tree) can automatically leverage the entire architectural plumbing and infrastructure that exists for building, rendering, and most importantly, caching of blocks. And of course, developers don't have to learn a custom API for producing toolbar tray block contents.

In turn, it helps to shift the perspective on the toolbar from "A list of links" to "A layout consisting of content blocks as well as various JS-driven triggers that load and expose content blocks in a content region of the layout."

As a result, the technical implementation should account for:

  1. Widgets and triggers.
  2. Blocks.

The render array structure for each "toolbar item" should therefore not contain the tray/block content. @benjifisher's proposal of separate 'tab' and 'tray' keys comes closest to that:

$item[tab] = $tab_render_array;
$item[tray] = $block_render_array;

But we actually need to go one step further and detach the tray block content from the tab item entirely:

Trigger    » Callback
Home         NULL
Menu         new MenuBlock($menu_name = 'admin', $start_level = 1, ...)
Bookmarks    new ShortcutMenuBlock()
User         new MenuBlock($menu_name = 'account', $max_depth = 1)
Search       new SearchResultsBlock()

Now, if someone were to Ajax-ify or ESI-fy the entire toolbar leveraging D8's Blocks/Layouts/Services New World Order™, then that would translate into:

Trigger    » URI
Home         NULL
Menu         GET /block/menu/menu?menu_name=admin&start_level=1
Bookmarks    GET /block/shortcut/menu
User         GET /block/menu/menu?menu_name=account&max_depth=1
Search       POST /block/search/results

In other words, the actual tray block contents can either be pre-generated ahead of time via internal sub-requests, but they can also be omitted altogether and get dynamically requested purely from the frontend only instead.

That would technically lead us to the following hook definition/structure:

$item[tab] = $tab_render_array;
$item[tray] = $callback_definition;

There's only one remaining tidbit with that: The above list of goals states that a rendered tab may not necessarily be a toolbar tab (link) and/or that the actual tab/triggering-element is only one sub-element of the rendered elements.

Typically, one would resolve that last detail by doing two things:

  1. Wrapping each tab into an invisible container that gets a reliable HTML ID.
  2. Registering the associated tray/block callback/definition/content with the HTML ID.
  3. Introducing and requiring that the rendered elements in a tab contain at least one element with a .toolbar-tray-trigger HTML class.

In turn, 1) multiple elements within a tab can trigger the tray, 2) arbitrary elements (not only links) can trigger the tray, 3) a tab may not trigger a tray at all, 4) the tray can be triggered/toggled via external JS/Ajax.


In any case, I cannot make much sense of the toolbar_item, toolbar_tab, toolbar_tray, and the other proposed theme/wrapper functions. I believe they are wrongly and too focused on the interpretation of toolbar as "A list of links."

In general, we should use non-special standard markup for the toolbar tabs and trays. We should treat the toolbar as a layout:

+---------------------------------------------+
| region-first    |          | region-second  |
| DIV | DIV | DIV |          | DIV [| DIV]    |
+---------------------------------------------+
| region-tray                                 |
|                                             |
+---------------------------------------------+

Each of the DIVs essentially wraps a toolbar item ("tab"). The toolbar tab item's content can be anything. It can be a simple link with or without a .toolbar-tray-trigger class. It can also be a search form. Which may or may not load search results through an Ajax POST request and manually trigger the tray. It can also be plain text - which may or may not be wrapped in a SPAN or DIV having a .toolbar-tray-trigger class.

The necessary CSS plumbing should happen at the layout's region level, and for the upper two regions, the wrapping DIVs should be the basis for applying the special toolbar layout styles. Speaking of, I'm not sure whether I see a point in allowing items to opt out of the toolbar tab styling in those regions — the one and only styling detail I could foresee is that a tab/item might want to "visually merge" itself to the left or right; i.e., omitting a potential tab border to a preceding or following tab item.

Likewise, the toolbar tray block contents should not require special markup to work within a toolbar tray. E.g., the admin menu simply is a rendered menu link tree. There should be sane default styling for various HTML elements that may appear within a tray; potentially/ideally triggered by a HTML class only (so there is a way to opt-out of the default styling). And we can attach additional CSS and potentially even JS to a particular tray block content to make it work better within this particular toolbar tray context (e.g., to hide deeper menu levels in a menu link tree and inject toggles to expose them).


In summary:

  1. We should split the tab/item definition from the tray/block content definition.
  2. We should turn the tab/item definition into a standard render array.
  3. Each tab/item definition should be wrapped into a "tab container" by the toolbar, not by the tab definition.
  4. Each tray/block content should be produced by a callable/callback that produces a render array for a block, like any other block. The toolbar should wrap that block content into a tray container, not the block content.
  5. We should require tab/item render arrays to manually add a HTML class to selective/particular elements for triggering the associated tray.
  6. For each tab/item, we should define the tray/block content callback.

Apparently, I did not provide a concrete proposal for the last point yet. There are multiple possible ways for doing so. One central aspect is the question of whether it should be possible for one tab/item to trigger the tray of another item; i.e., re-use of trays, which would turn the tab:tray mapping from 1:1 into n:1. However, while I can see technical arguments for that, off-hand, I can't see a real-world use-case for it, so I'd like to ignore that for now.

The most simple way would be to mix a custom #property into the top level of the render array for a tab/item render array definition; e.g., like this:

$items['account'] = array(
  '#toolbar_tray' => array(
    'menu_build_tree', array('admin'),
  ),
  // Whatever render array is desired:
  '#...' => '...',
  ...,
);
return $items;

Let's bear in mind that one module (hook implementation) should be able to return more than one toolbar item; therefore, $items['account'] is the "top level", not $items.

Another way would be to turn only the second level into the actual render array for the toolbar item, and have the first/top level be a non-render array declaration; e.g., like this:

$items['account'] = array(
  'title' => t('User account'),
  'callback' => 'menu_build_tree',
  'callback arguments' => array('admin'),
);
// Whatever render array is desired:
$items['account']['output'] = array(
  '#...' => '...',
  ...,
);
return $items;

And once we arrived at that conclusion, it's guaranteed that there will inherently be someone who will ask the fundamental question:

Why aren't the toolbar tabs/items not plugins, just like toolbar tray/block content plugins, too?

A perfectly reasonable question. In turn, that would translate into:

core/modules/user/lib/Drupal/user/Plugin/toolbar/tab/Account.php:

namespace Drupal/user/Plugin/toolbar/tab;

use Drupal\Core\Annotation\Plugin;
use Drupal\Core\Annotation\Translation;
use Drupal\Core\Menu\MenuBlock;
use Drupal\toolbar\ToolbarTab;

/**
 * User account toolbar tab.
 *
 * @Plugin(
 *   id = "user_account",
 *   label = @Translation("User account"),
 *   module = "user",
 *   renderTrayWithTab = false
 * )
 */
class Account extends ToolbarTab {

  /**
   * Implements ToolbarTabInterface::access().
   */
  public function access() {
    // Only applicable to authenticated users.
    return !empty($GLOBALS['user']->uid);
  }

  /**
   * Implements ToolbarTabInterface::buildTab().
   */
  public function buildTab() {
    $build = array(
      '#...' => '...',
    );
    return $build;
  }

  /**
   * Implements ToolbarTabInterface::buildTray().
   */
  public function buildTray() {
    $settings = array(
      'menu_name' => 'account',
      'max_depth' => 1,
    );
    return new MenuBlock($settings);
  }

}

In turn, that would have the positive consequence that each toolbar tab/item is a (~regular) block itself. Which inherently means that you can potentially add e.g. the existing Search Form block as a tab/item to the toolbar, without having to code it up first (which may not be fully true for every block, since custom CSS/JS and potentially Ajax may need to be attached to make it work in the toolbar, but regardless of that, it significantly increases possibilities).

jessebeach’s picture

Much food for thought. Thank you sun for the thorough feedback. benjifisher and I will walk through it and incorporate it.

jessebeach’s picture

Issue summary: View changes

updated the proposed resolution

benjifisher’s picture

Title: Refactor toolbar_view() and structure of hook_toolbar() » Update the structure returned by hook_toolbar()
Assigned: benjifisher » Unassigned

I am done rewriting the issue summary. I preserved and updated jessebeach's addition to the Remaining tasks. I hope I did not lose any other updates.

I agree that the title needs to be updated, but toolbar_view() is obsolete. Here is another new title.

jessebeach: Thanks for the clarification about separating tabs from trays. I see why my limited tests did not run into any problems.

I added some follow-up issues, including one to continue the discussion from #45–47. If I am the only one who thinks it needs changing, then there will be no harm beyond a couple of lonely issues.


@sun:

Thanks for the new ideas in #62. From the start, my goal on this issue has been to address your request in #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop that the toolbar be made flexible enough that you can build on it for the Admin menu module.

There are some important points where we agree.

We should turn the tab/item definition into a standard render array.

That has been my idea since my first patch on this issue, and it is incorporated in the patch in #58. I am sorry I did not finish reworking the issue summary earlier; I think I might have saved you some trouble. I think this one point handles most of the list of "goals and tasks" mentioned at the top of #62.

Each tab/item definition should be wrapped into a "tab container" by the toolbar, not by the tab definition.

I think this is the point of the theme wrappers. If you mean that the Toolbar module pre-render functions should be responsible for adding the theme wrappers, and the module implementing hook_toolbar() should not have to know about them, then I agree. I need to spend some more time looking at these.

My first and foremost recommendation is to take a step back and have a look at the situation we're facing architecturally:

I think we all agree on the three points listed under this in #62, except that so far "arbitrary content" is interpreted as "any render array" rather than "any block." I am afraid I do not follow the next part: "This immediately leads to the conclusion that the trays should be entirely detached from the tabs." What are the use cases?


There are some points where I have little to contribute but questions. Patches are always welcome ...

Currently, the tray can be any render array. I do not know enough about "Blocks/Layouts and WSCCI" nor plugins to be of any help in replacing the render arrays with anything more general. The best I can hope for is that the structure we have defined so far is good enough that it can be modified later: the toolbar_item element will still have a '#tray' property, but in the future it may be something other than a render array.

Question: can we use render arrays as wrappers for blocks or plugins?

jessebeach and I have been debating how to allow a sub-element of a tab be the trigger. We have tentatively agreed to postpone this decision, and I have added a new issue for it: #1877468: Finalize usage of hook_toolbar_alter(). (My description in that issue may not make the connection clear.) I am willing to deal with the decision in this issue, and it would help to have input from others, since so far jessebeach and I have not been able to agree.

I know enough about layout in CSS to realize that it can be hard. I defer to jessebeach and others, and I do not think it is part of the scope of this issue.


The main decision that has to be made in this issue is the API provided by hook_toolbar(). IIUC, the main API change proposed in #62 is to separate the tab from the tray. The current structure is

function mymodule_toolbar() {
  $tab['key'] = array(
    '#type' => 'toolbar_item',
    '#tab' => array(/* render array */),
    '#tray' => array(/* render array */),
  );
  return $tab;
}

(As I said above, I cannot comment on the merits of making the tray something other than a render array.) One significant advantage of this approach, keeping the tab and tray as properties of a single render element, is that we can guarantee "a reliable HTML ID" (borrowing a phrase from #62).

I am not sure what alternative you are proposing for hook_toolbar(). I suppose one option is to have two hooks, one to define tabs and one to define trays. I think I prefer something like this:

function mymodule_toolbar() {
  $items = array();
  $items[] = array(
    '#type' => 'toolbar_tab',
    '#tab' => array(/* render array */),
    '#trigger' => /* please fill this in */,
  );
  $items[] = array(
    '#type' => 'toolbar_tray',
    '#tray' => array(/* render array */),
    '#trigger' => /* please fill this in */,
  );
  return $items;
}

(With this structure, it might make more sense to replace the '#tab' and '#tray' properties with children ... but then we would have to figure out what to do if there are multiple children.) Whether or not this is close to what you had in mind, my main question is how we guarantee that the tab and tray use a consistent "reliable HTML ID".

We should require tab/item render arrays to manually add a HTML class to selective/particular elements for triggering the associated tray.

I am not sure how this would work, and I am not sure that this would make it easier for a module developer. Perhaps this is the same as the previous question.

jessebeach’s picture

This is just a quick message about process now that we have a functional-but-non-ideal solution in patch in this issue and a better-but-yet-uncoded improvement proposed by sun.

Sun, you gave this example in #62 of what might be considered a half-way point for the ultimate improvement of the Toolbar item registry.

Each of the DIVs essentially wraps a toolbar item ("tab"). The toolbar tab item's content can be anything. It can be a simple link with or without a .toolbar-tray-trigger class. It can also be a search form.

-- #62

The most simple way would be to mix a custom #property into the top level of the render array for a tab/item render array definition; e.g., like this:

$items['account'] = array(
  '#toolbar_tray' => array(
    'menu_build_tree', array('admin'),
  ),
  // Whatever render array is desired:
  '#...' => '...',
  ...,
);
return $items;

Let's bear in mind that one module (hook implementation) should be able to return more than one toolbar item; therefore, $items['account'] is the "top level", not $items.

The patch in #58 brings us this far. Here's an example from the toolbar.api.php file that is similar to your proposal.


$items['user_messages'] = array(
// Include the toolbar_tab_wrapper to style the link like a toolbar tab.
// Exclude the theme wrapper if custom styling is desired.
'#type' => 'toolbar_item',
'#tab' => array(
'#type' => 'link',
'#theme' => 'user_message_toolbar_tab',
'#theme_wrappers' => array(),
'#title' => t('Messages'),
'#href' => '/user/messages',
'#options' => array(
'attributes' => array(
'title' => t('Messages'),
),
),
),
'#tray' => array(
'#heading' => t('User messages'),
'messages' => array(/* renderable content */),
),
'#weight' => 125,
);

return $items;

Now I realize this isn't exactly the same as what you've proposed. But it does move us one step closer to a better design and more importantly, it allows us to remove custom code from Edit module that addresses the shortcomings of the current Toolbar architecture.

So what I'd love to do is do a few small cleanups on the patch in this queue and move the plugin/blockification to a followon so we don't have to wait several more weeks to clean up Edit. The new issue will also allow us to focus the discussion on the improvements that Sun suggests rather than introducing a pivot to this issue already 60+ comments long.

What do you both think? We can do a little more clean up here, get this incremental improvement in and continue to work on it further in a followon. If that's acceptable, I'll put the followon task together with details from this issue that describe the approach.

benjifisher’s picture

jessebeach:

I would like to do additional work, but I agree that there is a strong case for committing something without getting close to the February 1 deadline. The work I want to do could be attached to follow-up issues.

The one thing I think should be settled here is the API. Unless someone else comes up with a patch soon, I do not think we can move away from render elements. (I do not know enough to move in that direction.) But there is still Sun's suggestion in #62 that "the trays should be entirely detached from the tabs." I responded to this suggestion in the last section of #64. I am afraid that I do not follow Sun's logic, and his $items['account'] snippets are missing enough that I am not sure exactly what he is suggesting.

You suggested in #54 a 1-week comment period. I am willing to go along with that, meaning that we recommend commiting the current API unless there are further objections, clarifications, or patches by January 7. For my part, I will (for the sake of having a stable patch) not offer anything new except possibly improving the documentation.

jessebeach’s picture

This is a reroll of the patch in #58 to incorporate changes to Edit module.

We acknowledge that this patch is not everything we want right now. It is a measured step forward that addresses several shortcomings of the current Toolbar hook architecture. More work will need to be done, specifically moving on sun's suggestions in #62 and benjifisher's responses in #64. I've created a followup issue here that pulls the info from those comments so we can focus on them exclusively.

#1894964: Make the Toolbar PHP and JavaScript API more flexible so that it enables contrib to leverage it

Moving on the patch in this issue will allow us to address the following issues now:

#1860436: Remove toolbar-specific code from the Edit module
#1678002: Edit should provide a usable entity-level toolbar for saving fields

The following describes the changes this patch introduces:

  1. Removes the dependency on the presence of a tray in order for the #attach property of a toolbar item to be processed. This is the major blocking design flaw in toolbar right now.
  2. Removes the twig template in favor of a theme function to render the toolbar.
  3. Allows for a module to register content to the toolbar that will not be rendered as a tab. In other words, any renderable content. Currently, all content added to the toolbar must be a link (i.e. the element is passed to theme_links as a link).

These improvements address several of sun's suggestions in #62:

  1. We should split the tab/item definition from the tray/block content definition.
  2. We should turn the tab/item definition into a standard render array.
  3. Each tab/item definition should be wrapped into a "tab container" by the toolbar, not by the tab definition.

We are punting in this issue on whether toolbar items should be plugins and whether tray content should be loosely associated with a triggering tab. That will be hashed out in the followup.

effulgentsia’s picture

This just moves toolbar.module functions around to match their location in HEAD to cut down on the patch noise. If you intended to reorganize order of functions, please do so in a separate issue.

With this structure, it might make more sense to replace the '#tab' and '#tray' properties with children

I'm also not quite clear on why we're using properties instead of children to store render arrays, but my first impression is that that's wrong. I'm still getting up to speed on the patch though, so maybe there's something subtle I'm not seeing yet.

effulgentsia’s picture

It's possible that this is totally broken, but this changes '#tab' and '#tray' properties into 'tab' and 'tray' children. If anything is broken by this, I'm pretty sure we can fix it, so please report what you find.

jessebeach’s picture

I'm also not quite clear on why we're using properties instead of children to store render arrays, but my first impression is that that's wrong.

benjifisher and I have been going back and forth on this detail in our little echo chamber. Having some outside input helps precipitate a direction.

This patch changes instances of #tab/#tray to tab/tray.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
654 bytes
36.98 KB

I think some of the toolbar.module pre_render/preprocess/theme functions in this patch can be cleaned up some more, but it's late, and I'm too tired right now to communicate detailed thoughts on that. In the spirit of iterative development though, I think that what's here is a very nice improvement to what's in HEAD, especially with respect to the API of hook_toolbar(), and therefore, should go in, and improving the internal rendering flow further can be worked on in a follow up. Similarly, #1894964: Make the Toolbar PHP and JavaScript API more flexible so that it enables contrib to leverage it may change some of what's here, but I think what's here is still a worthwhile interim step. Therefore, RTBC.

This just moves #access out of toolbar_element_info(), because the result of that hook is cached globally, not per-user.

webchick’s picture

Issue tags: +Spark

Tagging.

webchick’s picture

Priority: Normal » Major
Status: Reviewed & tested by the community » Fixed

This looks a lot more straight-forward to me. Thanks a lot for all the hard work here.

Given that there are numerous features bound by feature freeze bound on this patch making it possible to put anything but links in the toolbar (escalating to major in response), I'm inclined to commit this. I'm skeptical about both the performance and DX implications of a plugin-based approach, but we can cover that in #1894964: Make the Toolbar PHP and JavaScript API more flexible so that it enables contrib to leverage it.

Committed and pushed to 8.x. Thanks!

I don't think this really needs a change notice, because it's a new feature anyway. But it might need an upgrade to an existing change notice.

effulgentsia’s picture

Status: Fixed » Needs review
FileSize
6.59 KB

Per #71, I'd like to help clean up some of the toolbar.module code, but let's start with just a docs-only cleanup.

effulgentsia’s picture

Priority: Major » Normal

Adjusting priority to reflect remaining work.

jessebeach’s picture

Status: Needs review » Closed (fixed)
jessebeach’s picture

Issue summary: View changes

Update proposed resolution, API changes, and other parts now that we are nearing consensus.