Fix toolbar height with CSS and make text zoom usable

Whoops ! Second screenshot is missing.

Text zooming is only affected in IE6, isn't it ? In this case, we would need to arbitrate between supporting text zooming in IE6 or having broken UI without javascript.
I would prefer the second solution personally.

#2 : Also happens without the patch I think ?
#4 : So it means it is okay ?

Indeed, you are right, this is more usable using ems than px, it is problematic to have some part of the page hidden on top.
So this may be a no-go.

I made some progress on this. Using only ems it is possible to get this right.
We need to clean-up all pixel values in shortcut.css and toolbar.css.

I'll come back with a patch.

This was not easy. But here it is.

It has many good things in it :

- Converts all px values to ems for toolbar and shortcut modules
- Makes text resizable while keeping the top of the screen stick to the toolbar height
- Allows us to remove javascript resizing for the toolbar
- Does not rely on javascript to place the toolbar

Moreover, with this patch, the technique used in #535066: Use CSS3 / IE filter to render toolbar shadow to resize the toolbar for IE in javascript is not useful anymore.

To summarize, it makes things cleaner, more standard and rely less on javascript.

The only caveat is a 1 pixel bug in Safari/Chrome/IE7, but i have hope we can fix this.
It is related to the way these browsers compute ems sizes.

Screenshots are attached :
- All browsers compared in their default text size.
- All browsers compared when text is zoomed.
- A screenshot before this patch when zooming text.

Edit : I removed the drop shadow to make it easier to see.

Hehe, that was the easy part ;)

Yes, the screenshot of the previous behavior is with JS enabled.

Here is a new patch that gives us best of both worlds :

- Without javascript enabled, it behaves as in #10, so on some browsers, the top padding may be 1 or 2 pixels too high.
- With javascript enabled, instead of setting the top padding of the body to the toolbar height, we set the min-height of the toolbar to the top padding of the body. The pixel glitches in some browsers will be fixed and we still keep text-resizing working as expected.

As you can see, I came late on all this, so I admit I did not digg in all issues, but maybe it helps to have a fresh look ;)
Would be useful to have other's opinions though.

This last patch for today fixes one last issue, adjusting the min-height of div.toolbar-menu also to fit the parent div. Not very elegant, but for now, it will do.

Thank you webchick, that's an honor :)

@seutje : Interesting. I have the same results, when text is zoomed, Chrome does not seem to return the correct value... strangely, Safari does, though.

There is still a little issue with this patch :

When loading the page with an already set zoom-in and then zooming out, the toolbar does not scale down. This is because we set the min-height the first time the toolbar is displayed.

What we can do :

- Let this as is and consider it as a reasonable tradeoff.
- Do not bother to adjust the toolbar to a pixel perfect height with javascript for buggy browsers at all and live with some extra pixels.
- Consider a maximum toolbar height under which we do resize the toolbar using javascript, otherwise, do not resize it. This could be a min-height value set in CSS to let themers control it.
- Use a timer to check for the toolbar height regularly and adjust the height of the toolbar when text is zoomed in and out. But I guess this has already been considered, so there may be a reason it was not done.

What do you think ?

The more I dig in this issue, the more I understand what others might have been through ;)

I implemented the setInterval solution and it works pretty well.
Everything works as expected and it feels cleaner.

See attached patch.

Forgot to remove the display:none I added to hide the shadow.

Using pixels would definitely be simpler. Personally, I don't care if text zooming is not supported in IE6.
I am not very fond of the interval based thing too, this patch was proposed as a proof of concept, although I did not notice performance issues with it.

The only problem is that if we do not want to use pixel-sized fonts for the toolbar (see #668280: Toolbar font size keeps jumping around), we need to fix the toolbar height/padding with ems too.

Using pixels only for the toolbar font and height would be a lot easier, it would have a consistent look across themes, it would remove the need of resizing handling in toolbar.js and let us have a pure CSS solution. The only tradeoff being no text zoom support in IE6. So this is my favorite solution, but depends on what is decided in that other issue.

Otherwise, we could use one of the other solutions I proposed in #17, that is if we want to support a decent text zoom.

so either we put everything in the toolbar in relative heights

The patch already fixes every height, padding top / bottom and font size values in ems, so we are able to have its exact height in ems and set the top padding. Or did I misunderstand what you mean ?

or we put an explicit relative height value on the wrappers

I am not really sure about what you mean here ? Using a height value to prevent wrapping to occur ?

So in the end, we should stick to the patch behavior : have a minimal CSS solution (toolbar in ems or in pixels), fix the top padding in CSS for the most common scenario (toolbar is not wrapping) and fallback to JS if it is enabled to adjust toolbar height.

Edit : and decide what method to use for JS (see the alternatives in comment #17).

I am not aware of a way to listen to text resizing events in JS, that's why we use a setInterval method. Otherwise we could have used the event directly to resize the toolbar I guess. Ultimately, we could have a setInterval and compare the toolbar value each time the function is called and resize it only if it is different from the last set size. But I am not sure it would be more CPU friendly.... Or did I misunderstand again ? (Sorry again if it is the case !).

+    // Equalize body paddingTop on resize
+    // We use a timeout because some browsers fire a ton of these
+    var resizeTimeout = setTimeout(Drupal.toolbar.adjustHeight, 500);
+
+    $(window).resize(function() {
+      clearTimeout(resizeTimeout);
+      resizeTimeout = setTimeout(Drupal.toolbar.adjustHeight, 500);
+    });

We do not want to resize the toolbar on window resize, but rather when text zooming, or both, but setInterval made this unnecessary.
Could be nice if we don't go for setInterval though.

how's this? attempts to equalize the padding-top of the body to the height of the toolbar

That was the approach of the previous patch, wasn't it ?

also, the use of .height() opposed to .css('height') fixes the chrome issue

Nice catch.

We should decide how to handle text zooming as stated in #17 and go for it with the height() workaround.

how's this? attempts to equalize the padding-top of the body to the height of the toolbar
That was the approach of the previous patch, wasn't it ?

Uh, sorry, I get it now.

No, this won't work because the whole point of this is to make text zooming work while keeping the toolbar height stick to the top area of the viewport.
Setting the top padding will fix this so it won't scale properly.

Consequently, the height() trick is unuseful.
Moreover, although I had the same erroneous values for top padding as you with Chrome/Safari, it is not a problem since we set the value and not retrieve it, and in this case it works fine.

Edit : So for me patch in #19 still stands, unless we decide to use a different approach from #17 for handling text zooming (or not handling it).

Sorry for the bunch of comments here.

I reattach patch from #19 and set this back to CNR to get more feedback and performance review.

I think performance problems raised by the overlay using setInterval would not occur here since the toolbar isn't the overlay, much simpler and straightforward.

So still waiting for a decision regarding #17 here.

Yes it does, some browsers do not accurately convert ems to pixels, (e.g. Safari and Chrome) so the toolbar height has 1 or 2 pixels missing. See the comments and screenshots above.

That is why I tried to address this using javascript.

imo interval isn't an option

As I explained, I'm not fond of it neither, but if it does not have performance issues, why wouldn't it be (at least) an option ? Otherwise, we could choose one of the other options. Anyway, any alternative from #17 is better than current behavior I think.

why ur setting a min-height on the toolbar rather than a padding on the body is entirely beyond me, it's like ur inviting it to overlap

I tested #24 in safari 4.0.3, chrome 4.0, IE6, IE7, IE8 and FF 3.5.7, I got consisitent, usable behaviour in all of them, xcept when js is off and the item wrap

Try text zooming with the patch and without, or with your last one. You will see the toolbar overlaps the top of the page. See previous screenshots.
The baddest thing is that when JS is enabled, you get a worse behavior than with it : without JS, toolbar resizes (almost) well, with JS adjusting top padding, it overlaps the page.

text-zooming is not only broken in IE6, it works the same in IE7 and 8

You mean... without patch applied ?

Edit : I guess you meant in general, so yes, as long as pixels are involved.

so... which browser gave which unexpected behaviour for u?

See screenshots in #10, everything is summarized there.

err, resize handler is getting thrown on zoom for me

I tried :

    $(window).resize(function() {
      console.log('resize');
      clearTimeout(resizeTimeout);
      resizeTimeout = setTimeout(Drupal.toolbar.adjustHeight, 500);
    });

And did not get any output in the console when zooming text, on Safari and Firefox on Mac, but got output when effectively resizing the window.

Tagging.

I thought I put it back to CNR, but doesn't seem so, and would be nice to have some more opinions.

or how about converting it back to ems?

But... It is already in ems in all patches I submitted and no, it does not make Safari / Chrome happy, there is still a 1-2 px quirk.
When toolbar is collapsed, there are chances that the size fits perfectly, but try with toolbar expanded or a different theme or with a different base body font size, it may fail (Firefox seems perfect, but IE < 8 and Safari/Chrome aren't).

About the resize event, I made some more tests :

Windows :
- Firefox : triggers resize event on text zoom
- IE : does not

Mac OS X :
- Firefox : does not (wtf ??)
- Safari : does not
- Chrome : does work !!

I'd bet Chrome on windows does trigger the event too, but did not test. Err, that's so weird !

@seutje : I admit I was skeptic about the resize event being fired under Windows, but testing confirms your affirmations.

So I don't think it can be a reliable event. Or we could handle exceptions, but I'm afraid it would be hard.
Too bad, it would have been a perfect solution...

Devil is always in the details ;)

I leave the issue as CNR since the #19 patch addresses all bugs, but at the price of a setInterval and #34 gives a less robust but CSS only solution.
Updated the title to reflect the last discussions.

The questions all this thread tries to answer (for newcomers eventually) :

- Do we want the toolbar height fixed in px or in ems ? Ems would be essentially to support IE text zooming.
- If we go for ems, there may be some added pixels to the top padding of the page and top padding may not follow toolbar height when window is resized. Is that bad ?
- If this is bad, we need to adjust the top padding using JS.
- As we need to listen to text resizing and since the "resize" event does not fire properly in all browsers, we can't rely on it to resize the toolbar, so another solution from #17 may be considered as a reasonable tradeoff. Or we could support only browsers that fire the "resize" event. But that would be sad.

Edit : updated summary to reflect problems with toolbar wrapping.

Ok, I understand better now.
But what does the "12" value would correspond to ?
Sounds fragile. This would vary in function of the theme and style, and I'm not sure we can compute this in case of inheritance, but maybe I'm missing something here ?

That's a cool patch ! I'm totally convinced by this approach finally.
The toolbar height would remain unchanged and the top padding be adjusted while keeping its height in ems.
Great !

But I found a nasty bug in jQuery :

Adding an alert to debug roughly in IE,

Drupal.toolbar.adjustHeight = function() {
  alert($('body').css('fontSize'));
  $('body').css('padding-top', Drupal.toolbar.height()/parseFloat($('body').css('fontSize')) + 'em');
};

Will return a totally irrelevant number.

Issue on jQuery bug tracker : http://dev.jquery.com/ticket/760.
I used the proof of concept patch and it addresses the issue.

Issue has been fixed, but in version 1.4, so I think we should wait for #653580: Upgrade to jQuery 1.4 to see if it goes in.
If it does, this patch will be great !

Just one little thing :

+    var resizeTimeout = setTimeout(Drupal.toolbar.adjustHeight, 500);

Do we need to fire the timeout in the first place ?
Since the init function always fires collapse or expand, which both fire adjustHeight, it is not useful I think.

Otherwise great work, let's hope jQuery 1.4 gets in !

It's more serious than that, toolbar will completely overlap the top of the page in all versions of IE because of this jQuery bug.
So unless it is okay to have a broken UI in IE, I am afraid we cannot commit this now.

Or we could let the JS resizing bit apart for now and submit a new patch when jQuery is updated to 1.4.

With either solution, we could update the patch in #535066: Use CSS3 / IE filter to render toolbar shadow and get rid of the IE trick in there (Yeah dependencies again... ;) ).

Hmmm... I realize we'll probably still need the IE trick anyway since we use height(), forget my last sentence.

So what about committing CSS fixes only in the meanwhile ?
It's just about a few extra pixels on some browsers and would be better than current behavior anyway.

I tested patch #41 in Konqueror and it works fine !

Here are 2 patches :

- 698976-toolbar-height-51-CSS.patch :
CSS only solution that could be committed now.

- 698976-toolbar-height-51-CSS-and-JS.patch :
CSS and JS fallback that could be committed when jQuery 1.4 is implemented.

Note that I made a few changes :

- 698976-toolbar-height-51-CSS.patch :
Back to exact ems height instead of playing with subpixel values. So instead of :

body.toolbar {
  padding-top: 2.545em;
}

body.toolbar-drawer {
  padding-top: 5.52em;
}

We have :

body.toolbar {
  padding-top: 2.52em;
}

body.toolbar-drawer {
  padding-top: 5.49em;
}

That was made as an attempt to make Safari and Chrome more accurate, but won't be needed if JS version goes in and having exact values is safer in the end.

- 698976-toolbar-height-51-CSS-and-JS.patch :
Remove first call to setTimeout(Drupal.toolbar.adjustHeight, 500);
Used outerHeight() instead of height() in Drupal.toolbar.height() to be safer. One could add a border to the toolbar.

In fact I thought it was me who changed those values.

These values are "exact" means that it is the em size I got by adding line-height + padding :

#toolbar div.toolbar-menu :
[element height] (1.8em + .5em + .5em) * [font-size] 0.9em = 2.52em;

#toolbar a#edit-shortcuts :
[element height] (2.3em + .5em + .5em) * [font-size] 0.9em = 2.97em;

So the total height of the expanded toolbar is 2.52 + 2.97 = 5,49em.

Anyway, comparing with computed values makes no sense IMHO, since if body font size changes, then computed values will change too, so we cannot be sure how this will be converted to pixels.

I think we should let the browser convert ems to pixels, leaving sub-pixel values as they are, I did not mean to avoid them but the opposite.

Btw, your comment's "using 5.49: toolbar computed height" and "using 5.52: toolbar computed height" point to the same image.

Agreed to other changes in the patch with JS ?

Oh, of course ;)

Other than that, agree with the other things ?

@mgifford : I'm totally over busy right now, but I'll try to give it a shot tonight.

The (brilliant) patch from #668280: Toolbar font size keeps jumping around implied some changes in the calculation of the toolbar height :

#toolbar div.toolbar-menu :
[element height] (1.8em + .5em + .5em) * [font-size] 0.9em = 2.52em;

#toolbar a#edit-shortcuts :
[element height] (2.3em + .5em + .5em) * [font-size] 0.9em = 2.97em;

So the total height of the expanded toolbar is 2.52 + 2.97 = 5,49em.

Since the toolbar font is not em based anymore, we do not have to multiply by the font size anymore.

Here is a re-rolled patch for latest CVS, but this time it is a pure CSS patch, no change has been made to the javascript bits, since it was here because of the font size, and moreover the approach taken in #787940: Generic approach for position:fixed elements like Toolbar, tableHeader will probably change this part in the future (I did not dig too much there, but if I understand correctly).

I did not have time to test the patch very well though.

Whoops, status, status...

@mgifford : you meant "The effects of this patch are visible without javascript enabled.", right ?

Oh okay, no problem.

@Bojhan: Without the patch, and with JS disabled, the toolbar leaves too much room at the top of the page. With patch applied, still no JS, it should leave exactly enough room for the toolbar, and when using text-zoom in the browser this should still be true.

Also see comment #77.