Comments

AmrMostafa’s picture

... 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

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

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

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

FileSize
3.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

Status: Needs work » Needs review
AmrMostafa’s picture

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

TravisCarden’s picture

Subscribe

sun’s picture

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

+

sun’s picture

@@ -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

has the patch been rerolled since sun's critique?

ñull’s picture

+ Very needed for sticky headers that disappear behind the menu

ñull’s picture

bump

bensey’s picture

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

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

+ ... 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

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

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

scott859’s picture

subscribing

sun’s picture

@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

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

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

Aurochs’s picture

ignore this

podarok’s picture

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

pillarsdotnet’s picture

@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

#22 works great, thanks!!!!

pillarsdotnet’s picture

Assigned: AmrMostafa » Unassigned
pillarsdotnet’s picture

sun’s picture

Version: 6.x-3.x-dev » 7.x-3.x-dev
taocode’s picture

Version: 7.x-3.x-dev » 7.x-3.0-rc5
Issue summary: View changes
Issue tags: +margin top, +menu over content
FileSize
2.12 KB

Patch breaks javascript. Please ignore.

taocode’s picture

FileSize
25.55 KB

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

taocode’s picture

taocode’s picture

doesn't work. Please ignore.

taocode’s picture

FileSize
2.03 KB

wrong again. Sorry.

taocode’s picture

FileSize
2.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.