With margin-top applied, we can use Javascript to adjust absolute positioned elements, as well as the <body> background.

Needless to say, it requires that you have Apply top margin option enabled.

CommentFileSizeAuthor
#46 admin_menu-auto_height_bind_plus_fixed_top-428844-#38-D7.patch2.27 KBStrVL
#36 admin_menu-428844-auto_height_bind_plus_fixed_top.patch2.52 KBtaocode
#35 admin_menu-428844-auto_height_bind.patch2.03 KBtaocode
#34 admin_menu-428844-auto_height.patch1.75 KBtaocode
#32 admin_menu-responsive-top-margin-adjustment.patch25.55 KBtaocode
#31 admin_menu-top-margin-auto-adjust.patch2.12 KBtaocode
#7 admin_menu-428844-04.patch3.42 KBAmrMostafa
#7 admin_menu-428844-04-D5.patch3.08 KBAmrMostafa
#5 admin_menu-428844-03.patch3.42 KBAmrMostafa
#3 admin_menu_D5-428844-03.patch2 KBAmrMostafa
#1 admin_menu-repositionLayout-D5.patch1.77 KBAmrMostafa
#2 admin_menu-428844-02.patch2.08 KBAmrMostafa
#2 admin_menu_D5-428844-02.patch1.79 KBAmrMostafa
admin_menu-repositionLayout.patch2.06 KBAmrMostafa
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

AmrMostafa’s picture

AmrMostafa CreditAttribution: AmrMostafa commented
FileSize
admin_menu-repositionLayout-D5.patch1.77 KB

... and since I originally developed this on D5 since my theme which needed the fix was there, here is a D5 patch as well.

AmrMostafa’s picture

AmrMostafa CreditAttribution: AmrMostafa commented
FileSize
admin_menu_D5-428844-02.patch1.79 KB
admin_menu-428844-02.patch2.08 KB

Fix for a bug where the :absolute selector was including #admin-menu's sub-menus in the processing which obviously misplaces them.

Updated patches included, HEAD and D5.

AmrMostafa’s picture

AmrMostafa CreditAttribution: AmrMostafa commented
FileSize
admin_menu_D5-428844-03.patch2 KB

The D5 versions of the patch above required jquery_update module (i.e. a recent version of jQuery) to work. But with this patch this is no longer required, stock 5.x jQuery will work fine.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Needs work

#427048: Sticky table headers hidden when keep menu at top of page is true seems to slightly overlap with this patch.

In general, that other issue made me finally think again about this approach here and I think I basically agree. However, I don't think we really need to add a new jQuery :absolute selector...?

Thoughts?

That aside, there are some indentation errors in this patch. It should also use single quotes where possible. And shouldn't the last hunk only run if $("body").css("background-position") results in anything?

AmrMostafa’s picture

AmrMostafa CreditAttribution: AmrMostafa commented
FileSize
admin_menu-428844-03.patch3.42 KB

Great, I've extended this patch to cover 'fixed' positioned elements, which includes #427048: Sticky table headers hidden when keep menu at top of page is true. However, since tableheader.js is included after us, this won't work yet. If you want to test it, you will have to open admin_menu.module and replace the 'module' bit with 'theme' for the drupal_add_js('admin_menu.js', ...) so admin_menu.js is included after tableheader.js. We will probably want to think about how to do this right.

However, I don't think we really need to add a new jQuery :absolute selector...?

Can you elaborate, why not? It appears to me to be the most optimal way to do this with jQuery. Otherwise, we would end up looping on the elements ourselves and no matter how good we can do it I doubt we can beat jQuery to it. We are getting all/any jQuery/Sizzle optimizations this way.

That aside, there are some indentation errors in this patch. It should also use single quotes where possible.

Fixed the quotes, but I can't seem to find the indentation issues (I assumed you mean hard tabs instead of spaces).

And shouldn't the last hunk only run if $("body").css("background-position") results in anything?

No, because even if the background-position is 0% 0% (the default, even if not set explicitly) we still want to adjust it.

Lastly, shouldn't we merge repositionLayout into adminMenuMarginTop?

AmrMostafa’s picture

AmrMostafa CreditAttribution: AmrMostafa commented
Status: Needs work » Needs review
AmrMostafa’s picture

AmrMostafa CreditAttribution: AmrMostafa commented
FileSize
Drupal 5 version of the same patch3.08 KB
admin_menu-428844-04.patch3.42 KB

Use $adminMenu instead of #admin-menu in some place, and a D5 version.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented

Subscribe

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Category: feature » task
Priority: Normal » Critical

In 3.x, admin_menu can have a variable size/height, due to optional, enhancing widgets and themes/designs that can be enabled. So we badly need this.

For 5.x-3.x, I'm fine with adding a dependency on jquery_update, btw. No need to special-case that, especially, because I want all 3.x code to be as comparable as possible, and many sites need jquery_update anyway.

What's left to continue here?

hass’s picture

hass CreditAttribution: hass commented

+

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
@@ -113,10 +113,32 @@ Drupal.admin.attachBehaviors = function 
+ * @TODO For optimal results, this should came after other JS files.
+ * For example, this won't adjust sticky table headers unless it has been
+ * executed after tableHeader (tableheader.js) behavior.

"@todo" is the proper directive. If a @todo spans over multiple lines, the following lines should be indented with extra 2 spaces.

Also, typo in "came".

uh oh - I thought the purpose of this patch is to fix all positioning issues like table headers?

@@ -113,10 +113,32 @@ Drupal.admin.attachBehaviors = function 
 Drupal.admin.behaviors.positionFixed = function (context, $adminMenu) {
   if (Drupal.settings.admin_menu.position_fixed) {
     $adminMenu.css('position', 'fixed');
+
+    // Re-position other elements accordingly

If this only applies to fixed positioning, then we need another solution for the absolute positioning case (which is the default, but still leads to issues when widgets start to span over "multiple lines").

@@ -113,10 +113,32 @@ Drupal.admin.attachBehaviors = function 
+    var height = parseInt($adminMenu.height());
...
+        $(this).css('top', (parseInt(top) + height) + 'px');

Always use "10" as extra argument to parseInt() if you want to get decimal numbers.

@@ -113,10 +113,32 @@ Drupal.admin.attachBehaviors = function 
+      fixed: 'jQuery(a).css("position") == "fixed"',

Use double quotes only where technically required, please.

@@ -185,6 +207,54 @@ Drupal.admin.behaviors.hover = function 
 /**
+ * If margin-top is applied, this will reposition all absolute positioned
+ * elements as well as the <body> background image, if set, so they don't
+ * overlap with us.
+ */

This JSDoc is missing a summary (what's there can be left as description).

@@ -185,6 +207,54 @@ Drupal.admin.behaviors.hover = function 
+      // Add the offset

Missing period (full-stop).

This review is powered by Dreditor.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

has the patch been rerolled since sun's critique?

ñull’s picture

ñull CreditAttribution: ñull commented

+ Very needed for sticky headers that disappear behind the menu

ñull’s picture

ñull CreditAttribution: ñull commented

bump

bensey’s picture

bensey CreditAttribution: bensey commented

Hi,
I really need this issue fixed also, but as a rank amateur who has trouble with patching in windows, I came up with a temporary solution until someone clever comes up with a better one, lol.

I often use conditional statements to define styles based on roles, and just threw the following into the head tag in page.tpl.php defining a style that adjusts the sticky header top margin for the role that the admin menu is displayed... (example is 30px as I set admin menu to 30px with larger font for accessibility)

Simple, but it works for me as a short-term fix.

<style>
	<?php
	if (in_array('administrator', array_values($user->roles))) {; ?>
		table.sticky-header {
			margin-top: 30px;
		}
    <?php } ?>
</style>
Senpai’s picture

Senpai CreditAttribution: Senpai commented
Status: Needs review » Needs work

The latest patch from #7 fails at hunk 1, line 113 against admin_menu-6.x-3.x-dev on Drupal-6-16.

Rejects file pasted below for reference:

***************
*** 113,122 ****
  
  /**
   * Apply 'position: fixed'.
   */
  Drupal.admin.behaviors.positionFixed = function (context, $adminMenu) {
    if (Drupal.settings.admin_menu.position_fixed) {
      $adminMenu.css('position', 'fixed');
    }
  };
  
--- 113,144 ----
  
  /**
   * Apply 'position: fixed'.
+  *
+  * This will also adjust all other 'fixed' positioned elements accordingly so
+  * they don't overlap with us.
+  *
+  * @TODO For optimal results, this should came after other JS files.
+  * For example, this won't adjust sticky table headers unless it has been
+  * executed after tableHeader (tableheader.js) behavior.
   */
  Drupal.admin.behaviors.positionFixed = function (context, $adminMenu) {
    if (Drupal.settings.admin_menu.position_fixed) {
      $adminMenu.css('position', 'fixed');
+ 
+     // Re-position other elements accordingly
+ 
+     // Extend jQuery with ':fixed' selector.
+     jQuery.extend(jQuery.expr[':'], {
+       fixed: 'jQuery(a).css("position") == "fixed"',
+     });
+     var height = parseInt($adminMenu.height());
+     // Offset fixed positioned element with our height.
+     $(':fixed').not($adminMenu).each(function () {
+       var top = $(this).css('top');
+       if (top != 'auto' && top != 'inherit') {
+         $(this).css('top', (parseInt(top) + height) + 'px');
+       }
+     });
    }
  };
boabjohn’s picture

boabjohn CreditAttribution: boabjohn commented

+ ... sort of nice, sort of essential. Any progress for alpha4? Sorry no code/css chops here: anything else you guys need? Ie, boxes of beer, three rousing cheers, ??

casey’s picture

casey CreditAttribution: casey commented

Using #787940: Generic approach for position:fixed elements like Toolbar, tableHeader admin menu would only need the class "displace-top", rest will be done automaticly.

In this patch I also see background-position repositioning... is it an idea to merge that into #787940: Generic approach for position:fixed elements like Toolbar, tableHeader?

casey’s picture

casey CreditAttribution: casey commented

I opened a seperate issue about using misc/displace.js: #801526: Use misc/displace.js to position admin_menu.

scott859’s picture

scott859 CreditAttribution: scott859 commented

subscribing

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

@AmrMostafa, TravisCarden, Senpai, bensey + others: Drupal 7 core may introduce a functionality along the lines of this issue/patch, as proposed by @casey. It would be great if all of you would help to review + validate the approach taken over in #787940: Generic approach for position:fixed elements like Toolbar, tableHeader

pillarsdotnet’s picture

pillarsdotnet CreditAttribution: pillarsdotnet commented

Until the 7.x solution gets back-ported to 6.x, I have implemented the following hack on my site. This also fixes the problem of table captions triggering the header-scroll too early. I'm just sharing in case somebody else needs an immediate work-around:

--- misc/tableheader.js 2009-04-30 00:36:53 +0000
+++ misc/tableheader.js 2010-08-17 18:50:31 +0000
@@ -44,7 +44,9 @@
     var viewHeight = document.documentElement.scrollHeight || document.body.scrollHeight;
     if (e.viewHeight != viewHeight) {
       e.viewHeight = viewHeight;
-      e.vPosition = $(e.table).offset().top - 4;
+      var captionHeight = $('caption', e.table).height();
+      var adminHeight = $('#admin-menu').height();
+      e.vPosition = $(e.table).offset().top -4 - adminHeight + captionHeight; // Allow room for admin menu, subtract for caption height
       e.hPosition = $(e.table).offset().left;
       e.vLength = e.table.clientHeight - 100;
       // Resize header and its cell widths.
@@ -56,6 +58,7 @@
           cellWidth = parentCell.get(index).clientWidth +'px';
         }
         $(this).css('width', cellWidth);
+       $(this).css('padding-top', adminHeight); // Don't let the header scroll behind the admin menu
       });
       $(e).css('width', $(e.table).css('width'));
     }

Also, I have the following CSS to prevent the table body from showing through the header, as if it were transparent:

table.sticky-header {                    
  z-index: 50;                          
 } 
 
table.sticky-enabled {             
  z-index: 0;                                                  
}
BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented

Thanks @pillarsdotnet - awesome patch. This problem has been driving me crazy.

Aurochs’s picture

Aurochs CreditAttribution: Aurochs commented

ignore this

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
CreditAttribution: podarok commented

@pillarsdotnet
#22 If You make this like a patch file for testing bot we can got it into code someday

pillarsdotnet’s picture

pillarsdotnet CreditAttribution: pillarsdotnet commented

@podarok: I sincerely doubt that drupal core will ever accept a patch that explicitly recognizes admin_menu.

Perhaps admin_menu can replace/override misc/tableheader.js with it's own version? I'd have to look through the code in jquery_update to see how they do it.

Todd Zebert’s picture

Todd Zebert CreditAttribution: Todd Zebert commented

#22 works great, thanks!!!!

pillarsdotnet’s picture

pillarsdotnet CreditAttribution: pillarsdotnet commented
Assigned: AmrMostafa » Unassigned
pillarsdotnet’s picture

pillarsdotnet CreditAttribution: pillarsdotnet commented

See #520368: Admin toolbar breaks sticky table headers
and #1210844: Admin menu covers sticky headers

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Version: 6.x-3.x-dev » 7.x-3.x-dev
taocode’s picture

taocode CreditAttribution: taocode as a volunteer commented
Version: 7.x-3.x-dev » 7.x-3.0-rc5
Issue summary: View changes
Issue tags: +margin top, +menu over content
FileSize
admin_menu-top-margin-auto-adjust.patch2.12 KB

Patch breaks javascript. Please ignore.

taocode’s picture

taocode CreditAttribution: taocode as a volunteer commented
FileSize
admin_menu-responsive-top-margin-adjustment.patch25.55 KB

My first patch submission. This broke javascript on some locations. Hopefully the patch below (comment #36) will help.

taocode’s picture

taocode CreditAttribution: taocode as a volunteer commented
taocode’s picture

taocode CreditAttribution: taocode as a volunteer commented
FileSize
admin_menu-428844-auto_height.patch1.75 KB

doesn't work. Please ignore.

taocode’s picture

taocode CreditAttribution: taocode as a volunteer commented
FileSize
admin_menu-428844-auto_height_bind.patch2.03 KB

wrong again. Sorry.

taocode’s picture

taocode CreditAttribution: taocode as a volunteer commented
FileSize
admin_menu-428844-auto_height_bind_plus_fixed_top.patch2.52 KB

This patch automatically adjusts the margin-top and absolute/fixed headers (with css class hook) according to current admin_menu height. Add 'hook-admin-menu-top' class to all your fixed/absolute positioned template elements to be re-positioned below the admin_menu.

Uses jQuery.bind to recalculate the admin menu height on load and resize, keeping the menu from overlapping the content no matter how tall it gets in a narrow browser.

mikeytown2’s picture

mikeytown2 CreditAttribution: mikeytown2 commented
Status: Needs work » Needs review

Patch applies, setting issue to needs review.

The last submitted patch, admin_menu-repositionLayout.patch, failed testing. View results

The last submitted patch, 2: admin_menu_D5-428844-02.patch, failed testing. View results

The last submitted patch, 2: admin_menu-428844-02.patch, failed testing. View results

The last submitted patch, 1: admin_menu-repositionLayout-D5.patch, failed testing. View results

The last submitted patch, 3: admin_menu_D5-428844-03.patch, failed testing. View results

The last submitted patch, 5: admin_menu-428844-03.patch, failed testing. View results

The last submitted patch, 7: admin_menu-428844-04-D5.patch, failed testing. View results

The last submitted patch, 7: admin_menu-428844-04.patch, failed testing. View results

StrVL’s picture

StrVL CreditAttribution: StrVL as a volunteer and at DrupalJedi commented
FileSize
admin_menu-auto_height_bind_plus_fixed_top-428844-#38-D7.patch2.27 KB

Patch #36 works, but it attaches more that one pair of load+resize events. After every attachBehaviors() call it adds new unnecessary events pair. There is a fix.

Chris Matthews’s picture

Chris Matthews CreditAttribution: Chris Matthews commented
Version: 7.x-3.0-rc5 » 7.x-3.x-dev
Issue tags: -margin top, -menu over content

The patch in #46 applied cleanly on 7.x-3.x-dev to admin_menu.css; admin_menu.js and admin_menu_toolbar.css works for me and appears to be ready to RTBC.

Chris Matthews’s picture

Chris Matthews CreditAttribution: Chris Matthews commented
Priority: Critical » Normal
Status: Needs review » Reviewed & tested by the community
thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
CreditAttribution: thalles at CI&T commented
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

Can you send screens before and after the patch?

On apply:
warning: admin_menu.js has type 100644, expected 100755