Use CSS3 / IE filter to render toolbar shadow

agree. It could be used the same length of admin module. It creates the same effect but with less collateral ones.
http://drupal.org/project/admin

Agreed. It's just too much!

I do not agree..
The main focus of the admin toolbar is that it separates the normal site layout from the administration part. So this isn't distracting, it's the purpose of the whole thing of giving some depth to underlying websites content.

I could live with a smaller gradient for sure, and I think that around 5-10px would be more than enough to let the toolbar stand out..

I like it as well, I am not sure but it looks somewhat diffrent on my screen - compared to that.

It looks like this is a Windows screenshot, right?
Is it IE or FF?

Gr

That is indeed critically annoying. Besides that, I've been clicking around in d7 head the last few days for the first time in a while and found the dropshadow to be overly heavy and long as well.

Looking at the actual sprite image, I see the shadow takes up 15px. That's enough to render a fully visible checkbox unusable.
Here's a variant that uses just 5px. These 5px will still capture clicks of course, but 5px will never completely cover a checkbox or radio button etc.

+1 for yoroy's adjustment. Much better!

forgot status, haz patch

Hm. I definitely get the rationale behind this patch, but I really do like the look of the current drop shadow, I have to say. yoroy's change is functionally better, but loses the nice gradual shadow of the existing one. Does something slightly longer (7px, 10px) fulfill the same requirement but keep much of the 'look'?

I would have to error on the side of functionality if comes down to it. Yoroy's change definitely helps with that. I am with Webchick for investigating a happy medium if it is possible.

I'd prefer a longer (something between the two proposition) but lighter shadow.

This is not critical http://drupal.org/node/45111

I tried webchick suggestions(sizes) using the yoroy patch for the toolbar shadow and it seems like in FF you can click a checkbox with both 7px and 10px sizes.

Here are the screenshots attached.

In the other side, I also note that toolbar also hide sticky table headers, so I also tried to make a patch to solve that.

I forgot to put the snapshots of the after the patch is applied

The images before does not respect what it's done for the shadow(thanks cwgordon for the clarification), so thinking a little more about the problem, I notice that the shadow change is not really necessary for the idea I'm proposing.

So in this patch I'm taking that portion of css out.

So, the real problem is we can not click normally the check behind the shadow, but when we have the table header on the shadow that problem disappears :D

This is a slightly tricky one.

If the shadow div is too high, we have usability issues; too short and it doesn't look so good.

I've re-rolled the patch from #21 to change the height of the shadow div to 9px. The actual shadow is 7px high (this is the important number, as it's the area behind which you may see a link or widget).
I've knocked back the shadow a lot so it doesn't look ropey at this size but added two 1px-high solid definition lines to keep the contrast between the toolbar and the rest of the page.
There is an alternative: remove the shadow altogether. I've included a screenshot of that, too, as I think that works.

Hmm. Test fail. OK, so I didn't touch toolbar.js and it's not a PHP file. Anyone?

Just an idea... couldn't we use box-shadow (CSS3), just like it is going to be done with Overlay (as far as I know) ?
We are already using vendor properties like -moz-border-radius in toolbar, so we could easily use box-shadow, -moz-box-shadow and -webkit-box-shadow.

After testing it, it removes the problem of using elements that are underneath it and also remove that ugly and not semantic empty <div> used only for the shadow.
Of course the support is limited to user-agents supporting this CSS3 property and the ones implementing this feature via the vendor properties I've mentioned (I've tested on Firefox 3.5 and Chrome).

For user-agents not supporting this CSS3 property, they will just not see any shadow.

The values of the shadow are trying to match as best the current shadow, but as it is an irregular gradient we will not be able t reproduce it exactly I think, but you are welcome to tweak it more if you want to try :)

Oh I forgot... if we're doing this, the /modules/toolbar/toolbar.png should be modified to remove the shadow. Here's the modified file.

Great work, TheRec!

That's the way to go.

I tried this patch but I no longer have a drop shadow with Firefox 3.0.14 on Mac. Personally, I think it is a small regression because it is less visible that the toolbar floats on top of the page.

Fifrefox support is described here : https://developer.mozilla.org/en/CSS/-moz-box-shadow
Safari 4 does not support it completely for the moment (the "spread" value is not supported, but without it the shadow wouldn't look right), so no shadow will display, but it is already supported in the latest builds : https://bugs.webkit.org/show_bug.cgi?id=27417

It works in Chrome 3 which apprently uses one of those builds ;)

We haven't dropped support for IE6 across core yet, so I don't see how we should use a CSS 3 rule for this specific design element. The drop shadow is not trivial but a considered design element used to seperate the toolbar from the rest of the page. We should not remove it lightly for IE users. Either add the css to do it for IE as well (http://www.dynamicdrive.com/style/csslibrary/item/css3_box_shadows_box_s...) or revert to using the graphic.

I would prefer to see just the original issue fixed by making the dropshadow less high in the graphic and not redesigning it at the same time.

Then you should drop the use of shadow-box (and vendor alike) for overlay too and design the shadow as images or not semantic stuff like this.

Administration menu works and is usable for several years now and never required a drop shadow to make the user understand that it is a sticky toolbar.

This shadow is pure cosmetics and not required at all. Hence, we can certainly use a CSS3-based approach to add a neat, but optional shadow.

It's not cosmetics. And the comparisions with admin_menu are geting tired, I'd think the different approaches and especially intended audiences have been explained well enough now?

Toolbar is for novice/intermediate users. The dropshadow is a deliberate design element to help people discern between front end and back end functionality/contexts. The current height is covering up checkboxes, that is what needs fixing. Simply throwing it away is not an option. We're implementing a design we want to test first, amend later, not the other way around.

TheRec's got a point. IMO, this is a PERFECT example of progressive enhancement/enrichment...the toolbar is still perfectly usable without the shadow but just adds a little bit as a reward for anyone using a browser capable of seeing it. The same is true with what we're doing in the overlay issue, and If we can't use CSS3 properties and vender-specific properties here then we need to remove them elsewhere too.

+1 for using box-shadow. It's the most maintainable, most semantic, lightest, and most forward-looking approach.

Sure. It's just that the intended audience for this toolbar is least likely to be *not* having to use IE. Which is why I think it's more important to keep the shadow here for IE users, more so then in the overlay or other implementations.

Either way, I definitely agree that it needs to be much shorter. It's been bothering me (even if just from an aesthetics standpoint) ever since it made it into D7. The height in #11 looks perfect to me.

I think Rec's use of CSS3 is acceptable but would also support a more traditional image based approach. I tested D7 for the first time in a while and immediately was put off by the prominence drop shadow, which is how I turned up here. I find the current gradient obtrusive. It also does not gradually fade as a normal gradient would. I didn't catch on that this was supposed to be a drop shadow. If I, a technical user, did not clue in most normal users will be in the same boat. #25, #16 and #22 are all visual improvements from what we have now. I'd personally be happy with any of them.

Can we all agree that the current drop shadow should have more of a gradient and not be so long? If so, all that remains to be debated is the implementation, which I think everyone understands the pros and cons of. A call will need to be made on that.

subscribing

Been using d7 regularly the last weeks, the shadow is still too high :)
Maybe somebody can make a 7px version of the drop shadow *with* the gradient. Lets do it by changing the graphic.

7px toolbar screenshot, patch & updated toolbar.png attached for review...

Yes, 7px is the right size, but this dropshadow is too dark. I'm attaching a version that takes the original (from Mark Boulton's PSDs) dropshadow squeezed to 7px high.

Currently:

Patch and image from #40:

Patch from #40 with image as attached here:

Agreed. RTBC patch from #40 with image from #41.

I don't know about the proposed update. It feels messier now -- the 3D is less distinct.

Per Dries' feedback.

Sorry, but leaving at needs review, this is one opinion without useful pointers on how to improve. How is this messier? Does it need to be so distinct?

I agree with sun in #32.

What would the CSS3-based dropshadow look like?

See #25 for a CSS3-based box-shadow.

Note that it has been rejected by some contributors because it is "so critical" to see the difference of context between the toolbar and the content, that seems to be conveyed only by this drop shadown according to them. So CSS3 solution is basically a dead end until... hmm maybe when IE will support it ? ;)

Attached patch uses box-shadow (based on TheRec's patch). I think it looks really great.

For IE, someone could possibly try this sort of thing:

filter: progid:DXImageTransform.Microsoft.dropShadow(color=#561A0B, offX=-6, offY=-6, positive=true);

It doesn't matter if not everybody gets the shadow. As sun said:

Administration menu works and is usable for several years now and never required a drop shadow to make the user understand that it is a sticky toolbar.

This shadow is pure cosmetics and not required at all. Hence, we can certainly use a CSS3-based approach to add a neat, but optional shadow.

Using CSS also means elements underneath it can be clicked.

The screenshots in #51 look good to me. What does it look like in Safari and IE though?

@webchick, In Safari it will look the same as Firefox, and in IE you won't get any shadow, just a hard line. As jbrown said, we could combat that with an IE filter if we wanted to, and judging from yoroy's #33 and #35, I think we'll have to if we go this route.

If we decide to go with the CSS based approach, should we update toolbar.png to drop the dropshadow?

yeah - I've pretty much got the shadow working on ie without the png

Okay I updated toolbar.png .

An extra div is added for IE. I've only tested it on IE7. Can someone test it on 6 & 8. If it doesn't work on 6 we can always just disable it.

Working great on Firefox 3.6 and Chrome 4.

Actually, this patch fixes the drawer on IE.

Elements below the shadow still being clickable is a good reason to use the CSS approach. It's looking fine, too. But not so sure about the amount of browser specific code this adds. There's precedent for browser specific CSS, but the conditional for IE in toolbar.tpl.php might be taking it a bit too far.

There is precedent for browser specific HTML. Garland and Seven both use the same IE-specific HTML technique to include IE css.

My patch could work this way, but it is simpler to just output the div for IE as it is not required at all for non-IE browsers.

Safari 4 does not support it completely for the moment (the "spread" value is not supported, but without it the shadow wouldn't look right)

By removing the "spread" value and adjusting the "offset-y" value, I get the exact same result as with the "spread" value set on Firefox and Chrome plus getting it to work on Safari :

  box-shadow: 0 3px 20px #000;
  -moz-box-shadow: 0 3px 20px #000;
  -webkit-box-shadow: 0 3px 20px #000;

Here is an updated patch.

Using conditional comment for targeting IE directly in markup is really sad, there must be a better way. I'll try and see if I can find a more viable alternative.

Thanks - it looks exactly the same on FF with your modifications.

One possibility is to use opacity: 0; to block out the shadow div on non IE browsers, as IE doesn't support opacity. It might not work on every browser.

Adding khtml support should not hurt.

I'm happy for the shadow to not appear on IE (like the curved active menu items), but I think others disagree.

If I remember well, there is some kind of a bug in IE which can turn into a benefit in our case :

When using PNG24 with transparency, IE lets us select elements below the transparent image when opacity is lower than 50%. That could help to reduce the annoyance with the non selectable elements for IE using a transparent png background (since the non selectable area would only be on the part of the shadow where opacity is higher than 50%) while using css3 box-shadow property for other browsers.

I am not sure how this behaves with IE7 and IE8 though.

That said, it might not be any better than using a dropShadow filter.

I will try and see how that goes.

I have some good news !

I have been able to make the drop shadow using pure CSS work on all major browsers without any additional markup for IE. The only caveat is that checkboxes will be uncheckable on the very top of the screen on IE (will be ok on other browsers), but only on a few pixels (but that was the case with the previous patches). That seems an acceptable drawback to me.

Supported browsers are :

- IE 6, 7 and 8
- Firefox
- Safari
- Chrome

Opera will support box-shadows in 10.5 version.

See the attached screenshots for a comparison in each browser.

So judging by the screenshot IE6 makes it go to white instead of transparent? That's a bit of a bummer, but not a deal breaker. I say this is ready to roll with #66 and image from #56 (that is the image you were using, right jide?)

The "white part" you see is an unrelated issue, this is the header region which seems to not collapse when it is empty as it does on other browsers. Since the toolbar is fixed on IE6, I could not scroll to make the screenshot. But it is definitely transparent.

I attached another screenshot from IE6 with a block in the header region so you can see what I mean.

The toolbar image is the one from #56, yes.

I also discovered that the Glow filter does not behave like the Shadow filter, elements below the glow ARE selectable.

So we could make a workaround by using Glow instead of Shadow to make the drop shadow on IE and have the elements selectable. But that could require some extra markup, I am trying to see if it is possible without. The rendering of the glow is a bit different also, it has some noise, but it is hard to notice.

Worth trying ?

I overlooked some problems with padding on both sides of the toolbar. Ignore last patch for now, I am working on this.

Status change.

I like the original. There's a scroll bar if content falls underneath the shadow.

Okay, I am back with a new patch. This one adds the drop shadow with pure CSS, works on all major browsers and allow elements to get focus when below the shadow even on IE. The padding issue is also addressed.

But to achieve this, I had to make some CSS trickery and do an exception in the javascript file for IE. So this is not ideal and is quite a hack. The IE-specific CSS rules look like this :

/**
 * IE (all versions) Fixes.
 *
 * Use MS specific Glow filter to allow elements to be selectable below the 
 * shadow and adjust position since the Glow filter adds extra padding.
 */
#toolbar {
  filter: progid:DXImageTransform.Microsoft.Glow(color=#000000, strength='10');
  -ms-filter: "progid:DXImageTransform.Microsoft.Glow(color=#000000, strength='10')";
  left:-20px\9;
  right:0px\9;
  width:100%\9;
  margin-top:-10px\9;
}

Yes, it is ugly - but it works. Apparently, a negative bottom-margin does not work when using the Glow filter, so we have to do this in the js file :

$('body').addClass('toolbar-drawer').css('paddingTop', $.browser.msie ? $('#toolbar').outerHeight() -20 : $('#toolbar').outerHeight());

So to summarize the options :

1. Keep the original version with a PNG image :
   • Transparency won't work in IE6
   • Form elements won't be selectable below the shadow on any browser
2. Use CSS3 for standard browsers :
   • No shadow will be displayed at all in all versions of IE
3. Use CSS3 for standard browsers and Shadow filter for IE :
   • Could require some extra markup (a workaround is possible but it may require some additional IE specific CSS hacks)
   • Form elements won't be selectable below the shadow in IE only
4. Use CSS3 for standard browsers and Glow filter for IE :
   • Uses some ugly hacks in CSS
   • Adds browser sniffing in js file
   • Form elements WILL BE selectable below the shadow in all browsers
   • The shadow has some noise in it (minor)

The attached patch uses the last method and additional screenshots show the result in different versions of IE.

There may be a CSS only solution to correct the bottom margin bug, in this case the 4th solution might be more viable. If someone has an idea...

Please note that starting with Firefox 3.6, the pointer-events: none; CSS declaration is now supported. So even if only Firefox 3.6 supports it, we should start taking it into account in the future.

For example, on the drop shadow (PNG version), setting the pointer-events to none would make the object below the shadow (PNG version) respond to mouse events.

The CSS property pointer-events allows authors to control whether or when an element may be the target of a mouse event. This property is used to specify under which circumstance (if any) a mouse event should go "through" an element and target whatever is "underneath" that element instead.

References:
http://demos.hacks.mozilla.org/openweb/pointer-events/
https://developer.mozilla.org/en/CSS/pointer-events

Now, that being said, the patch in #66 is the way to go. But personally I would not provide any support for IE filters. Internet Explorer 9 will support CSS shadows properties as well as CSS rounded corners.

But currently, as we do not support IE for rounded corners in current Drupal 7, we should not support shadows either.

+++ modules/toolbar/toolbar.css	28 Jan 2010 18:53:42 -0000
@@ -33,10 +33,18 @@ body.toolbar-drawer {
+  filter: progid:DXImageTransform.Microsoft.Shadow(color=#000000, direction='180', strength='10');
+  -ms-filter: "progid:DXImageTransform.Microsoft.Shadow(color=#000000, direction='180', strength='10')";

I did cross-post!

@jideL: Nice work and summary.

Personally I opt for option 2. ;-)

I hate those ugly hacks. And these hacks will probably need to be removed when IE9 will start to show in betas.

@xmacinfo : I agree, the hacks are ugly. I am much more worried about the browser sniffing workaround in the js file though.

An alternative would be a conditional comment to target IE, but I do not think this is something done for core modules CSS elsewhere ?

I think options 2 and 3 are the most reasonable, but patch #66 needs some adjustments to correct a padding issue (hopefully without any hack !).

So judging by the screenshot IE6 makes it go to white instead of transparent?

Obviously, you were right. I misunderstood how the toolbar resizing was handled.

This new patch is a cleaner and more elegant approach (and has no bug !) based on n°3 in my list :

- CSS3 shadows for modern browsers
- Shadow filter for IE 6, 7 and 8, but elements are not selectable under the shadow
- no extra markup
- no ugly CSS hacks
- In javascript, we rely on the "filters" attribute to get the shadow size and correct the height of the toolbar

The apparently unused function Drupal.toolbar.height() is used to keep this clean.

Personally, I'm not sure it is worth introducing such a browser hack, if people can scroll. I'm all for a clean CSS-based solution though, but it sounds like we're not quite there yet.

I don't think this is critical because people will be able to click the checkbox, it just requires some scrolling.

The last patch uses only CSS to achieve this :

+  box-shadow: 0 3px 20px #000;
+  -moz-box-shadow: 0 3px 20px #000;
+  -webkit-box-shadow: 0 3px 20px #000;
+  -khtml-box-shadow: 0 3px 20px #000;
+  filter: progid:DXImageTransform.Microsoft.Shadow(color=#000000, direction='180', strength='10');
+  -ms-filter: "progid:DXImageTransform.Microsoft.Shadow(color=#000000, direction='180', strength='10')";

The only workaround used is the need to take the filter attribute into account in javascript to compute the toolbar height because of a bug, in IE, Shadow size is not taken into account by the outerHeight() function :

 Drupal.toolbar.height = function() {
-  return $("#toolbar").height();
+  var height = $('#toolbar').outerHeight();
+  // IE Fix.
+  if ($('#toolbar').attr('filters')) {
+    height -= $('#toolbar').attr('filters').item("DXImageTransform.Microsoft.Shadow").strength;
+  }
+  return height;
 };

If we want drop shadows with CSS in IE, there is no other way I'm afraid.

But that seems a relatively clean way to achieve this to me.

Ah and if that was not clear enough in my previous comment, the "white space at top of the screen" bug is fixed in last patch.

In other words, we could say the outerHeight() function is buggy when some filter is applied to an element, so that's a jQuery bug in a way.

+++ modules/toolbar/toolbar.css	29 Jan 2010 02:59:50 -0000
@@ -33,10 +33,18 @@ body.toolbar-drawer {
-  left: 0;
-  right: 0;
+  left: -10px;
+  right: -10px;
+  padding-left: 10px;
+  padding-right: 10px;

Would the following also work?

margin: 0 -10px;
padding: 0 10px;

+++ modules/toolbar/toolbar.css	29 Jan 2010 02:59:50 -0000
@@ -33,10 +33,18 @@ body.toolbar-drawer {
+  filter: progid:DXImageTransform.Microsoft.Shadow(color=#000000, direction='180', strength='10');
+  -ms-filter: "progid:DXImageTransform.Microsoft.Shadow(color=#000000, direction='180', strength='10')";

I'm fine with this, as long as it works.

+++ modules/toolbar/toolbar.css	29 Jan 2010 02:59:50 -0000
@@ -143,5 +142,7 @@ body.toolbar-drawer {
-  left: 0;
+  left:-10px;
+  right:0;
+  width:100%;

Missing leading space before values.

+++ modules/toolbar/toolbar.js	29 Jan 2010 02:59:50 -0000
@@ -96,7 +96,12 @@ Drupal.toolbar.toggle = function() {
+  // IE Fix.
+  if ($('#toolbar').attr('filters')) {
+    height -= $('#toolbar').attr('filters').item("DXImageTransform.Microsoft.Shadow").strength;
+  }

This inline comment should be replaced with the explanation given in one of the last comments of this issue (IE not counting shadow filter height).

+++ modules/toolbar/toolbar.tpl.php	29 Jan 2010 02:59:50 -0000
@@ -23,6 +23,7 @@
+

Please revert.

Powered by Dreditor.

Patch updated according to sun's review.

Edit : I also changed the Drupal.toolbar.height() function a little to be more robust :

if ($('#toolbar').attr('filters') && $('#toolbar').attr('filters').item("DXImageTransform.Microsoft.Shadow")) {

@sun and @jide:

+  left: -10px;
+  right: -10px;

...is not interchangeable with...

margin: 0 -10px;

The first is positioning, not margin. The patch in #84 should probably revert that change.

That is what I thought, but it works fine.

Thanks!

this still has issues - will detail in a few minutes

After jide's changes in #60 the extra padding over the edge of the viewport was no longer sufficient to prevent a slight horizontal gradient at the edges. My updated patch has a padding of 20px, which is sufficient.

Is -khtml-box-shadow for Konqueror? I'm not getting any shadow at all on Konqueror 4.3.2

Working great for me on Firefox 3.6, Chrome 4 and IE 7

I don't think it matters that on IE there is 5px where you can vaguely see something, but can't click on it. If people want to click on something then they should scroll it down so that they know that they are clicking on the correct thing. This could be seen as a feature. ;-)

Dries: there aren't really any hacks in the patch - just creative CSS and bugfixes.

I do not understand why you mention #60, it seems that your patch is based on #84 ?

It seems to me that using 10px for the shadow and a -10px margin makes a perfectly shaped shadow at the edges, and besides I can't see how it could not, mathematically ;) Besides, a 20px shadow with -20px margin should behave the same.

Moreover, this makes the shadow much bigger.

About -khtml-box-shadow, I added this "in case", but that may be unneeded.

The regression was introduced in changes you made in #60 using 4 parameters instead of 5. My patch is derived from #84.

I took a screenshot with #84 and zoomed in. Most of the first line of the gradient is #545451, but the first and last six pixels have a slight gradient. The left and right most pixels are #5f5f5c. My changes fix this.

The CSS3 shadow is not defined as having a specific height. It is the IE shadow that is 10px high.

My changes to the patch do not make the shadow any bigger.

The horizontal gradient occurs for 17px under the div. That is why the padding and negative margin are 20px.

In the attached image, the red line marks the end of the horizontal gradient.

Sorry I overlooked your patch but I get it now, obviously, the shadow size is the same. Thanks for the clarification. Good work !

A last small change to correct the shadow on the left side of the screen on IE6, see screenshots.

Cool! Great work jide.

Here is what the gradient looked like before #60 if anyone is interested.

Cool, good job. Anybody know if this fixes the stick table headers (see the title of this task)? I can't test ATM but it seems that someone either needs to test that here or we need to break out into a separate issue.

This issue has been fixed in #686670: Inconsistent code style in toolbar.js and has been committed.

Hmmm not sure of what you meant here... Sticky table headers did not work at all anymore, and the last patch I just submitted in #686670: Inconsistent code style in toolbar.js fixes it.
But if you were talking about how the shadow overlaps table headers, this is another story, they do still overlap but I do not find this annoying IMO.

Ok, changing title since sticky headers was fixed in #686670: Inconsistent code style in toolbar.js and marking RTBC.

This patch rocks!

RTBC++

This is the list of developers: TheRec, jbrown, jide, sun

Should anyone else be on the list?

In #698976: Fix toolbar height with CSS and make text zoom usable, comment #10, the proposed patch could avoid using javascript to resize the toolbar completely, making the IE-specific javascript not useful anymore.

+++ modules/toolbar/toolbar.js	29 Jan 2010 18:04:23 -0000
@@ -96,7 +96,13 @@ Drupal.toolbar.toggle = function() {
+  var height = $('#toolbar').outerHeight();

Why is 'shadow' written with a capital S?

+++ modules/toolbar/toolbar.js	29 Jan 2010 18:04:23 -0000
@@ -96,7 +96,13 @@ Drupal.toolbar.toggle = function() {
+  if ($('#toolbar').attr('filters') && $('#toolbar').attr('filters').item("DXImageTransform.Microsoft.Shadow")) {

If this is a real jQuery bug, I'd recommend adding a @todo.

It is the syntax Microsoft uses : http://msdn.microsoft.com/en-us/library/ms533086(VS.85).aspx

I am not sure we can consider this a real jQuery bug, since it is very particular to IE.

jQuery is browser independent, so its a bug. You should create an issue in their bug tracker.

@ jbrown : That's true, however, what makes me sceptic about this is that IE effectively considers the shadow as an addition to the element dimensions. So that may be better to have the height() (and others) function return the height + the shadow, like it is rendered by the browser.

I made some tests in a sandbox, see attached screenshot to see what I mean.

I think webkit/gecko are correct and IE is wrong. The shadow isn't part of the div, its just a shadow.

Definitely. But we do not want jQuery to remove this extra height IE gives to the div from the height() function, I think.

See Dries his review.

Dries concerns have been answered in #102 and in following comments.

The Shadow syntax is correct and I still do believe this is not a jQuery bug and have given arguments.

Jide's screenshot in 104 makes it obvious why this is needed. It's not a jQuery bug...if anything, it's an IE bug.

IMO this is still RTBC, but I'll just set to CNR for some more opinions.

To summarize about the "jQuery bug" : It is not a jQuery bug.

It is an IE bug that jQuery leaves as is and it is right to do so.
When adding a Shadow to an element in IE, the Shadow size is added to the element.

Screenshot in #104 just shows the default behavior of the browsers without any javascript, see how immediate sibling are beneath the shadow in IE ?
The box size in the DOM IS 10px bigger.

So it would be erroneous to make jQuery return a different height than the effective DOM box height.

Cross post... Thanks mcrittenden, I felt a little alone on this ;)

jquery should work consistently across all browsers.

Its a debatable point whether jquery should return the height to include the shadow on IE or not. For our purposes it should not, but IE regards the height as including the shadow and this will affect layout. Some uses of 'height' may wish to take this into account.

Bug or not, but it wont be determined here.

The patch works correctly with the version of jquery included with Drupal.

I don't know of any outstanding issues with the patch.

I really can't see how it should return a height that is not the element height in the html flow. But anyway, let's leave this debate apart.

I will fill a jQuery bug to see how they react over there.

As I stated above, #698976: Fix toolbar height with CSS and make text zoom usable could lead us to not having with this IE quirk at all (and not having to debate about this anymore ;) ).
Would be useful to see reviews there so we decide what we do here. Or come back to this issue later once this is eventually committed.

#698976: Fix toolbar height with CSS and make text zoom usable will probably not remove the need of the IE trick finally, so this is RTBC even more.

I filled a bug on jQuery.
Some interesting discussion here : http://dev.jquery.com/ticket/6014

-khtml-box-shadow is not supported by Konqueror, so I removed it.
I guess it should not affect RTBC status as it is a tiny modification.

Yes - I can't find any evidence that -khtml-box-shadow is real. I think people just made it up because -khtml-border-radius exists.

Note that this patch also needs toolbar.png from #93.

I found out that if the Shadow filter was not present in the stylesheet, an error would be thrown in all versions of IE because of :

  if ($('#toolbar').attr('filters') && $('#toolbar').attr('filters').item("DXImageTransform.Microsoft.Shadow")) {
    height -= $('#toolbar').attr('filters').item("DXImageTransform.Microsoft.Shadow").strength;
  }

It seems that the item() function cannot be used if no filter has been set, and that is a problem as we need to know what filter is applied (at least it is safer to do so).

A solution is to use a regular expression :

  if ($('#toolbar').css('filter').match(/DXImageTransform\.Microsoft\.Shadow/)) {
    height -= $('#toolbar').get(0).filters.item("DXImageTransform.Microsoft.Shadow").strength;
  }

I also corrected the comment to be more accurate.

Patch updated.

When would the filter not be in the stylesheet? How is the error reported?

Still needs toolbar.png from #93.

@jbrown : Someone could change the toolbar stylesheet, or remove it through preprocess function, or override it in theme stylesheet, or whatever, so I think we need to be safe for all situations. The reported error is a lame "Unspecified error".

Does the error pop in in a dialog box?
Can this be implemented without using a regex?

Also, I was wondering if there are any probs on IE with javascript disabled?

Yes, the error pops in a dialog box.

I tried other strategies, but they all failed. "typeof" always returns "string", and testing for existence of the function if no filters are present also throws an error.

IE works fine without javascript enabled, but not sure what you mean here.

Okay - I'll test later on IE7. Need to reboot a laptop out of Ubuntu.

The definitive answer to the "jQuery bug" issue from jeresig himself :

this just isn't something that we're likely to fix natively in core (since it's both obscure and a browser-specific bit of hackery)

Works great for me on IE7.

With the patch in #118 I removed the filter from CSS and did not get the error popup.

Same. RTBC #118 with #93's toolbar.png.

Reduced toolbar.png from 387 to 330 bytes.

Can we get one more set of screenshots? or are the latest in whatever comment still applicable?

Screenshots in #51 and #78 are up to date.

Patch in #118, toolbar.png in #128 and list of developers in #99.

I think we are ready for the pretty!

Screenshots in #66 still apply, except for IE6, see http://drupal.org/files/issues/ie6-after.png from #93.

Good work overall. Only niggle I have is I think removing the spacing around the home icon is risky, leaves no room for text zooming effects without exposing some of the gray in unwanted places. It's a sprite, I always see sprites using a couple of pixels around each seperate icon to leave room for rounding errors and whatnot.

@yoroy : I agree, see #704230: Buttons in toolbar look ugly when text size is bigger.

yoroy: when browsers are are scaling up images with text there is no issue. When browsers are scaling up without images then, yes, it will screw up - but adding extra space is not the way to fix this as it will always be possible to scale up more.

There is already an issue for this: #667918: Sprite height too short causing active state graphical bug in toolbar. Please don't pollute this issue.

Thanks for the crosslinks. I like to think of it as reviewing, not polluting. I ask here because the white space seems to get removed in this issue here, at http://drupal.org/node/535066#comment-2525618 specifically. Not only removing the shadow but also the few px of whitespace, which seems unnecessary optimization to me. If the other issues take care of that, fine.

I implemented it accurately. There is no need for a few px of whitespace whatsoever.

Edit : Hmmm, forget me.

This is ready to be committed. Full details in #130, #131.

Great. Committed to CVS HEAD. Thanks.

The filters for shadows in IE disable the background-color change on hover state for links in the toolbar and the shortcuts bar for IE8. IE6 and IE7 work fine. This came up here but is not specific to RTL: http://drupal.org/node/740182#comment-3516100

Anybody have an idea how to address this? I unfortunately don't.

@reglogge: Please open another bug issue specifying that the problem is for IE8(better if you can provide a screenshot) and link this issue there. This one was long and it was really about a decision IIRC, so IMHO it's a really bad idea to re-open.