Generic approach for position:fixed elements like Toolbar, tableHeader

sub, and postponing #787670: Clean up tableheader.js untill this lands (or not).

See also #428844: Use JS to adjust layout (a fix for all margin/absolute/background problems)

Has this been tested on all browsers?

       // As the toolbar is an overlay displaced region, overlay should be
       // notified of it's height change to adapt its position.
-      $(window).triggerHandler('resize.overlay-event');
+      $(window).triggerHandler('resize');

Either the comment is wrong, or the triggerHandler should be namespaced again...

Yes Dries I used this patch together with #668640: Overlay shouldn't be based on jQuery UI Dialog to test them together.
I tested in a lot of browsers.

OK, committed this to CVS HEAD. Thanks. Hopefully that will allow us to make progress on some other bugs.

Yay! That should indeed make things easier, thanks casey!

Found some minor typos:

Fixed positioning (CSS declaration position:fixed) is done relatively to the
Should be relative?

// Test for position:fixed support as IE6 does not.
In the doc above, there was already a section about position fixed support and how IE6 does not support it properly. Also, "as IE6 does not" comes off a bit weird to me. I'd change it to:
// Test for position:fixed support.

var el = $('<div style="position:fixed;top:10px"/>').appendTo(document.body);
Needs spacing to enhance readability and coding standards?
var el = $('<div style="position:fixed; top:10px" />').appendTo(document.body);

Attached patch changes the above mentioned issues.

Found another typo (overriden -> overridden) and moved the fixed support test to the main drupal.js file. This way the test is available for all scripts (like tableheader.js, which is duplicating the test found here).

The code doc needs some review, not entirely sure if I used the right syntax. Also, I used a rather verbose syntax for the variable positionFixedSupported, but that is because it is namespaced under the main drupal object, and I want to avoid conflicts.

Agreed, it is an internal variable.

Doc changes as discussed with casey on irc.

Toggling the shortcut bar on the modules page now takes several seconds and jumps back to the top of the page.

Actually, it is reloading the page when I toggle on the modules page - it doesn't do this on other pages.

Behaves like this with Firefox 3.6.3 & Chrome

Do you have firebug installed for firefox? If so, please open firebug, reload the page, and watch for any errors in the console.

I can't reproduce this on windows /firefox 3.6.3.

Yes - on the modules page I get

uncaught exception: Syntax error, unrecognized expression: Syntax error, unrecognized expression: target

but it doesn't provide any further information.

Committed to CVS HEAD. Thanks.

um, I'm a bit baffled that this patch went in without any in-depth code review...

I kindly ask to roll back the already committed patch, so we can redo a proper and solid patch here.

The first patch contained some questionable lines. This (second) one at minimum:

+++ misc/drupal.js	14 May 2010 12:02:43 -0000
@@ -300,6 +300,24 @@
+Drupal.positionFixedSupported = function () {

should pre-check + conditonally initialize jQuery.support.positionFixed -- this is jQuery 1.4, friends :)

80 critical left. Go review some!

I understand. But still. This code went into Drupal core without ANY technical review. That's totally not acceptable at this stage of the cycle.

I'm not saying that it cannot be done and committed for D7. But look: We have no target for reviewing now (3 separate patches), and any change request we're doing now is going to be treated like an API change, since this code was already committed. I do not have the time nor energy to review and clean-up all patches next to the already committed code, while constantly having to inter-diff all these targets.

Therefore, let's roll back this entire thing first. Then, roll a new + complete patch, and properly review that to get this done, please.

What follows is a review of the latest patch, but in reality, I have much more to criticize on the first original code, which has been partially changed in the meantime.

+++ misc/drupal.js	17 May 2010 21:14:08 -0000
@@ -299,23 +299,25 @@
+  /**
+   * Checks if position:fixed is supported.
+   *
+   * @return
+   *   Boolean indicating whether or not position:fixed is supported.
+   *
+   * @see http://yura.thinkweb2.com/cft/#IS_POSITION_FIXED_SUPPORTED
+   */

Description in @return can be moved into docblock summary.

@see can be removed.

+++ misc/drupal.js	17 May 2010 21:14:08 -0000
@@ -299,23 +299,25 @@
+  $.support.positionFixed = function () {
+    if (this._positionFixed === undefined) {
+      var el = $('<div style="position:fixed; top:10px" />').appendTo(document.body);
+      this._positionFixed = el[0].offsetTop === 10;
+      el.remove();
+    }
+  ¶
+    return this._positionFixed;
+  };

$.support.X are properties, not functions. Please have a look at jQuery's source code to see how $.support implementations are done.

Additionally, we should move this code to the bottom of drupal.js, but before any procedural code starts in the file. It is very likely that we will add further $.support properties in the future.

+++ misc/displace.js	17 May 2010 21:14:07 -0000
@@ -13,34 +13,62 @@
+ * To position an element absolute at the top of the viewport add beside the
+ * "displace-top" or "displace-bottom" also the class "displace-absolute".

We do not need to repeat the -top and -bottom classes from the preceding paragraph.

+++ misc/displace.js	17 May 2010 21:14:07 -0000
@@ -13,34 +13,62 @@
  * When a browser doesn't support position:fixed (like IE6) the element gets
  * positioned absolutely by default, but this can be overridden by using the
  * "displace-unsupported" class.

I had to look up the actual code to understand what this paragraph is trying to tell me.

+++ misc/displace.js	17 May 2010 21:14:07 -0000
@@ -50,6 +78,7 @@
 Drupal.displace.elements = [];
+Drupal.displace.mixed = [];
 Drupal.displace.displacement = [];

"mixed" is a very poor property name.

+++ modules/system/system.css	17 May 2010 21:14:08 -0000
@@ -263,12 +263,18 @@
+.displace-processed .displace-absolute.displace-absolute-fixed {

It's weird that absolute and fixed are applied in parallel. Makes no sense for me.

+++ modules/system/system.css	17 May 2010 21:14:08 -0000
@@ -263,12 +263,18 @@
+.displace-processed .displace-absolute {
+  position: absolute;
...
 .displace-unsupported .displace-top,
 .displace-unsupported .displace-bottom {
   position: absolute;

So what's the difference between absolute and unsupported?

80 critical left. Go review some!

Now you're doing exactly what I explained: Defending the already committed code and pointing me to this last follow-up patch instead. While my focus starts reversed; instead of doing countless of follow-up patches and inter-diff reviews, we want to ensure a properly reviewed and agreed on technical implementation in the first place.

Attached patch reverts the first and second patch.

Afterwards, we can apply this patch reversed, plus the patch from #30, and, review the entire implementation.

I don't understand why we wouldn't be able to improve the already committed code with follow-up patches? I don't mind rolling back the patch but I'm puzzled by the 'why'.

That's what we need to discuss and review in detail, but in total. Therefore, rollback first, proper reviews afterwards.

Again, the committed code had _zero_ technical reviews. Not even an agreement on the overall approach to begin with.

The status went from "needs review" to "fixed". We obviously missed some stages here.

Rolled back from CVS HEAD using the patch in #32.

Rolled back displace.js too.

+++ misc/displace.js	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,316 @@
+  Drupal.displace.clearCache();
...
+/**
+ * Focusin event handler: detects when the focus is being set to an element.
+ */

Focussing

+++ misc/displace.js	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,316 @@
+  // Adjust paddingTop and paddingBottom of document root element to make sure
+  // nothing is hidden beneath any displaced elements. ¶

trailing white space

77 critical left. Go review some!

Thnx for the link...
I was wrong

interesting approach, seems a lot more workable than all those 1-off solutions

will try to play around with this some more

We need sun's blessing before this can get commited

Would love to get sun's take too.

Did I understand the most recent follow-ups correctly, in that this entire, current displace.js approach and patch did not work out like it should have? (due to an issue in jQuery UI?)

I need this patch If I wonna use media module in overlay...
Seriously...

Sorry, just a useless follow-up for now -- webchick bumped #655416: "Demonstrate block regions" bugs with regions, navigation, overlay to critical, so this is a major bug at the very least.

Will try to do a in-depth review ASAP.

The patch looks really good. One minor quibble. With JS turned off I lose the box shadow on the toolbar. The box shadow attributes shouldn't be part of .displace-processed #toolbar, they need to stay with #toolbar so they show with or without JS turned on.

The CSS that is currently in system.css belongs in system-behavior.css. Also, it would be helpful to document (briefly) why and where this code is being used, rather than just having " To be used with displace.js."

Rerolled against HEAD, moving the CSS to system-behavior.css. I replaced "To be used with displace.js." with "Displace position: fixed elements.", which isn't much better. Eek.

Getting this in will allow #888412: Make footer stick to the bottom of the page to work, which would be awesome.

+  // Apply adjustments to the document.
+  $(document.body).css(adjustment)
+  // IE7 isn't reflowing the document immediately.
+  // @todo This might be fixed in a cleaner way.
+  .addClass('displace-trigger-reflow').removeClass('displace-trigger-reflow');

http://stackoverflow.com/questions/1702399/how-can-i-force-reflow-after-...
http://stackoverflow.com/questions/510213/when-does-reflow-happen-in-a-d...

I'd prefer to call .className instead of calling the jQuery wrapper functions addClass and removeClass, since this is a bugfix for IE7, and we know exactly what we want to do (force a reflow, nothing fancy with adding a class or removing a class). I haven't tested this myself, but according to the first link it should work.

+  // Apply adjustments to the document.
+  $(document.body).css(adjustment);
+  // IE7 isn't reflowing the document immediately.
+  document.body.className = document.body.className;

Yes it should be fast(er), and yes, it is a micro optimization. But also, it's a bit cleaner code-wise since there is no extra redundant class necessary.
http://stackoverflow.com/questions/693719/javascript-performance-of-clas...

lastly, the doc line doesn't really tell why you're adding/removing a class. Something along the lines of:

// Fake a style change to force an immediate document reflow in IE7.

Would be more descriptive, regardless of my proposed micro change.

To help things moving, rerolled with suggestions.

I didn't roll because I didn't test it. I still haven't tested it (no time for it), so it still needs a review. Or did you test it aspilicious?

Thanks for rolling!

Nop I didn't I just rerolled :)

small testcase for Kiphaas7's micro-optimization -> http://jsperf.com/reflow-trigger

Bikeshedding probably, but I got excited when I saw jsperf.com. (Thanks for the link seutje! :) ).

The entire modified code should be included, since jQuery does benefit from chaining.
http://jsperf.com/reflow-trigger/4

I've started writing some tests in qUnit for displace.js. Will try to post a .zip file with a first working draft tomorrow.

Just a beginning, first few functions have limited test coverage. If anyone else has time to add tests, please do! I probably won't have enough time to finish it by myself. If you see faults in my tests, please improve them, it's my first time writing tests for jQuery code :).

Can anyone summarize, in non-js-ninja language, what this patch accomplishes?

And secondly, should we mark any of these duplicates or postponed on this issue:

#546126: Toolbar interferes with anchor links
#567754: Wrong anchor adjustment in tableheader.js
#800270: Anchor/focus hidden beneath toolbar/tableheader (CKEditor full screen menu)

The patch adds a generic (read reusable) solution to placing fixed positioned elements on the page without having them overlap each other or the content on the page.

This means that if I develop an awesome banner module that keeps a banner docked on the top of the screen, I can use this displacement behavior to guarantee that it won't overlap the content at the top of the page. I also don't have to worry about special cases like the presence of the admin toolbar or the shortcut bar while I'm developing my module.

This needs a reroll as the patch no longer applies cleanly.

#67: 787940-67-displace.patch queued for re-testing.

Given that this was already committed in some form already, and given that the lack of a fix here exposes several pretty nasty user-facing bugs, I'd still like to get this into D7, assuming it's done by RC1.

Well, this patch applies, but it really messes up the Skip to Main stuff.

Can we get someone to review the patch, fix the Skip to Main problem & then document how to thoroughly test it so we can get this RTBC before RC1?

I can't really take this any further. I wanted the patch to be able to apply to core, but this needs a CSS/JS guy and that's not me..

@james.elliott - do you have some test code for this?

@aspilicious - can you look over my patch which is based on yours and see what I may have missed. So much has changed.

@tim.plunkett - your patch did get by the bots. Can you look to find ways to get this patch through the bots now?

@casey - how do we make this work, particularly with the skip to main? How close is this patch to what you see as needed now?

I can apply patches to my sandbox and do some testing, particularly against accessibility issues.

I only rerolled it :).
Can't fix this, I'm srry.

I only rerolled as well, but I'll take a look at it this week.

Going to put this into my list. However, not sure whether I'll be able to work on it before the weekend, so feel free to beat me to it (assign to yourself first).

ok, tried to move forward here, but serious problem:

All of the re-rolled patches in #63, #65, #67, #80 are entirely different. Some are missing displace* files, some are missing changes to Overlay, and some missing changes to tableHeader and/or Toolbar. The corresponding comments do not mention whether the changes were intentionally left out or whether there was a problem in re-rolling. Therefore, we have at least 4 different targets in this issue, from which every single one seems to be incomplete.

What we need is a clear, single, and up-to-date target. Looks like all of the 4 patches need to be extracted into the files they are changing in order to figure out what changes the current patch should actually contain, and merge them back into a single patch afterwards. The existing patches cannot be applied, since HEAD has changed in the meantime. That's going to take some time.

Looking at the diffstat and innerdiff of each patch, there doesn't seem to be too many missed changes between #63, #65 and #67, but those are the oldest and are definitely behind HEAD in a big way (two of the files don't even exist anymore).

sun, is this assigned to you because you still intend to undertake the task you outlined in your last comment? If not, I might be able to spend some time on it.

Straight re-roll against HEAD.

- Moved displace styles into dedicated /misc/displace.css. (but didn't adjust asset/library loading yet)

Go bot go! Thanks @sun!

sadly, this patch needs more than a re-roll.

It's adding about 1cm of blank to the top of the themes I tested it on.

Yes, already mentioned in #88. Additionally, the implementation is a bit over-engineered, IMHO. Hope to be able to revisit this patch today/tomorrow, but it's not too high on my todo/priority list.

I see this got postponed for D8. Any chance we'll have a working solution for D7? I don't mean start chasing d7/d8 dev changes and re-rolling/re-coding, just a patch that'll simply do magic in D7.0 final (ideally without the issue in #90 above). I am seeking a solution for admin_menu and 'sticky' table headers to play nice together.

bump.

#1417378: Remove eval() from tableHeader JavaScript is somehow related in that it allow multiple module/script to declare an offset function for tableheader placement. It's very simple and very effective so it might make this issue simpler to solve.

#1440628: [Meta] javascript toolbar/tableheader with url fragment mess

#88: drupal.displace.88.patch queued for re-testing.

+toolbar-followup

I won't have time to work on this.

It may not be exactly the same behavior described in this issue, but we're working on a generic way to measure elements that displace (like the toolbar) other components (like the overlay and sticky table headers) as well as an event-based update system redraw elements that have been displaced when the displacing elements change. It draws on some of the ideas in this issue, but puts the onus of measuring the displacement on the modules that own the views of the displacing elements.

#1847084-48: Measure/track displacing elements better + provide change events for them (fix overlay + toolbar)

I'm marking this fixed based on the the commit of #1847084: Measure/track displacing elements better + provide change events for them (fix overlay + toolbar).