Toolbar incorrectly add whitespace to body when there's no expandable content to fill it

Reproduced the bug.

However, I think this a Toolbar module bug and any fix for it should be there, not in the Shortcut module.

Also, by doing it in hook_enable() and hook_disable(), doesn't that mean the bug is only fixed for the administrator who actually enables/disables the module, not any other users?

So I think that the ideal way to fix this would be to modify this code:

/**
 * Implements hook_preprocess_html().
 *
 * Add some page classes, so global page theming can adjust to the toolbar.
 */
function toolbar_preprocess_html(&$vars) {
  if (isset($vars['page']['page_top']['toolbar']) && user_access('access toolbar')) {
    $vars['classes_array'][] = 'toolbar';
    if (!_toolbar_is_collapsed()) {
      $vars['classes_array'][] = 'toolbar-drawer';
    }
  }
}

The part where 'toolbar-drawer' is added to the page class shouldn't just check _toolbar_is_collapsed(), but also if the toolbar drawer has any content in it.

The problem is that there seems to almost be a conspiracy in the page rendering system to make that difficult or impossible to do. The 'page_top' region (in which the toolbar lives) is rendered in template_process_html(), so toolbar_preprocess_html() is too early to figure out if something was added to the drawer. And there doesn't seem to be a clean way to check later in the process.

What about using js to remove toggle button in case drawer has no children? This patch also adapts padding.

The previous patches were no good- This is not a js problem nor something only related to shortcut.

Because the toolbar drawer is intended to be used by other modules, not just shortcut, and there is no way to tell if the drawer will have content when creating the toolbar, I added a hook to determine if the toolbar has a drawer (and an implementation of it in shortcut).

This is needed both when adding the toolbar-drawer class to body (which caused the extra padding shown in the image in the initial issue) and when adding the toolbar toggle icon into the render array.

+++ b/core/modules/toolbar/toolbar.moduleundefined
@@ -114,6 +114,24 @@ function _toolbar_is_collapsed() {
+ * @return boolean

Should be @return bool

+++ b/core/modules/toolbar/toolbar.moduleundefined
@@ -114,6 +114,24 @@ function _toolbar_is_collapsed() {
+  if (module_implements('toolbar_drawer')) {

No need for this if statement, just foreach over it.

Also the new hook needs docs in toolbar.api.php

Made tim's changes.

Missing the api docs, guessing you used `git diff` to roll the patch. In this case you'd have to git add everything first, and then git diff --staged.

Alright smartyhawk. Here it is, thanks

Looks good to me.
Tentatively tagging for backport? Unsure if that's possible.

Note that the entire bottom half of the patch is mostly indenting. git diff -w shows this:

@@ -147,7 +163,7 @@ function toolbar_pre_render($toolbar) {
 function toolbar_preprocess_html(&$vars) {
   if (isset($vars['page']['page_top']['toolbar']) && user_access('access toolbar')) {
     $vars['classes_array'][] = 'toolbar';
-    if (!_toolbar_is_collapsed()) {
+    if (_toolbar_has_drawer() && !_toolbar_is_collapsed()) {
       $vars['classes_array'][] = 'toolbar-drawer';
     }
   }
@@ -259,6 +275,7 @@ function toolbar_view() {
     '#attributes' => array('id' => 'toolbar-home'),
   );
 
+  if (_toolbar_has_drawer()) {
   // Add an anchor to be able to toggle the visibility of the drawer.
   $build['toolbar_toggle'] = array(
     '#theme' => 'toolbar_toggle',
@@ -276,7 +293,7 @@ function toolbar_view() {
   }
   $build['toolbar_drawer']['#type'] = 'container';
   $build['toolbar_drawer']['#attributes']['class'] = $toolbar_drawer_classes;
-
+  }
   return $build;
 }

I'm not really comfortable with the new hook here, but don't really have a better idea given the inability to detect whether there's content or not in the toolbar without it. I think this could use more review to be honest.

I don't think we are going in the right way. This bug is way more simple: There's a cookie that check if the toolbar is collapsed. We assume that the lack of the cookie means that the toolbar is expanded. But it's impossible to tell if it's really expanded, or just if there's nothing to be expanded (and therefore set the collapsed cookie).

I'm trying to fix it changing the cookie from Drupal.toolbar.collapsed to Drupal.toolbar.expanded.

That way, we always know when the toolbar is really expanded and when it's really collapsed, no matter if it's because the user collapse it or because there's no content to be expanded.

Anyone has any idea of how this behaves in D8 now with the new toolbar?

This is not a issue in D8 with the new toolbar. Backporting to 7.x.

This works just fine.

Before:

After:

Got a fatal error in OpenIDFunctionalTestCase->testBlockedUserLogin. I don’t think this has anything to do with this patch.

Yeah, it’s green now!

Moving back to RTBC as set by #18.

When I apply this patch to a default installation of Drupal core, the shortcut bar stops working entirely. You can no longer collapse it (and in one case I'm pretty sure I was unable to expand it instead).

I would suspect it has something to do with this, since the code here looks totally wrong:

 Drupal.toolbar.toggle = function() {
-  if ($('#toolbar div.toolbar-drawer').hasClass('collapsed')) {
+  if ($('#toolbar div.toolbar-drawer').hasClass('expanded')) {
     Drupal.toolbar.expand();
   }
   else {
-    Drupal.toolbar.collapse();
+    Drupal.toolbar.expand();
   }
 };

Also, it would be ideal not to mess with Drupal's cookies in the middle of a stable release; that is kind of part of the public API. Maybe there's another way to fix this still?

Though I have to say that the idea of fixing it the way this patch did (switching up collapsed vs expanded checks) sounds pretty clever :)

I wonder how I managed to make this work 2 years ago. :P

Probably something was lost with the backport from 8.x to 7.x. At the time the recommendation was to write patches to 8.x branch, but between the time I wrote my first and second patches a lot of things changed in the D8 toolbar.

Anyway I will check this later.

Duplicated.