It has a tendency to slice text at the top of the viewport in half, which I find really distracting.

Files: 
CommentFileSizeAuthor
#128 toolbar.png330 bytesjbrown
#118 535066-toolbar-shadow-118.patch3.67 KBjide
PASSED: [[SimpleTest]]: [MySQL] 17,528 pass(es). View
#116 535066-toolbar-shadow-116.patch3.74 KBjide
PASSED: [[SimpleTest]]: [MySQL] 17,538 pass(es). View
#104 jQuery-shadow.jpg45.64 KBjide
#94 old-gradient.png5.48 KBjbrown
#93 ie6-after.png20.36 KBjide
#93 ie6-before.png20.54 KBjide
#93 toolbar.png387 bytesjide
#93 toolbar-shadow.patch3.78 KBjide
PASSED: [[SimpleTest]]: [MySQL] 17,305 pass(es). View
#92 gradient-end.png6.15 KBjbrown
#89 toolbar-shadow.patch3.78 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 17,325 pass(es). View
#89 toolbar.png387 bytesjbrown
#84 toolbar-shadow.patch3.99 KBjide
PASSED: [[SimpleTest]]: [MySQL] 17,324 pass(es). View
#78 toolbar-shadow.patch4.05 KBjide
PASSED: [[SimpleTest]]: [MySQL] 17,374 pass(es). View
#78 toolbar-shadow-ie6.png23.22 KBjide
#78 toolbar-shadow-ie7.png21.72 KBjide
#78 toolbar-shadow-ie8.png22.08 KBjide
#73 toolbar-shadow.patch4.15 KBjide
PASSED: [[SimpleTest]]: [MySQL] 17,381 pass(es). View
#73 toolbar-shadow-IE6.png35.93 KBjide
#73 toolbar-shadow-IE7.png28.21 KBjide
#73 toolbar-shadow-IE8.png26.09 KBjide
#68 Capture d’écran 2010-01-28 à 20.29.07.png25.18 KBjide
#66 dropshadows.jpg264.24 KBjide
#66 toolbar-shadow.patch2.48 KBjide
PASSED: [[SimpleTest]]: [MySQL] 17,384 pass(es). View
#63 toolbar-shadow.patch2.58 KBjide
PASSED: [[SimpleTest]]: [MySQL] 17,382 pass(es). View
#60 toolbar-shadow.patch2.54 KBjide
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch toolbar-shadow_1.patch. View
#57 toolbar-shadow.patch2.48 KBjbrown
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in toolbar.tpl.php. View
#56 toolbar-shadow.patch2.46 KBjbrown
FAILED: [[SimpleTest]]: [MySQL] 17,371 pass(es), 2 fail(s), and 0 exception(es). View
#56 toolbar.png387 bytesjbrown
#51 toolbar-box-shadow.patch1.47 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 17,393 pass(es). View
#51 Screenshot-Configuration | Drupal 7 - Mozilla Firefox.png47.86 KBjbrown
#51 Screenshot-Configuration | Drupal 7 - Mozilla Firefox-1.png48.64 KBjbrown
#51 Screenshot-Configuration | Drupal 7 - Mozilla Firefox-2.png54.12 KBjbrown
#51 Screenshot-Configuration | Drupal 7 - Mozilla Firefox-3.png55.11 KBjbrown
#51 Screenshot-Drupal 7 - Mozilla Firefox.png48.06 KBjbrown
#51 Screenshot-Drupal 7 - Mozilla Firefox-1.png49.68 KBjbrown
#41 toolbar.png537 bytesyoroy
#40 toolbar_7px_shadow.patch751 bytesmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 17,370 pass(es). View
#40 screenshot.png16.62 KBmr.baileys
#40 toolbar.png694 bytesmr.baileys
#26 toolbar.png736 bytesTheRec
#25 535066-toolbar-shadow_1.patch1.55 KBTheRec
Passed: 13282 passes, 0 fails, 0 exceptions View
#25 535066-toolbar-shadow_1-before.png2.51 KBTheRec
#25 535066-toolbar-shadow_1-after-expanded.png2.65 KBTheRec
#25 535066-toolbar-shadow_1-after-collapsed.png2.96 KBTheRec
#25 535066-toolbar-shadow_1-after-ie6.png2.17 KBTheRec
#22 toolbar-do-not-hide-table-headers-v3.patch2.19 KBmcjim
Failed: Invalid PHP syntax in modules/toolbar/toolbar.js. View
#22 toolbar.png2.04 KBmcjim
#22 toolbar-shadow-current.png57.77 KBmcjim
#22 toolbar-shadow-new.png58.36 KBmcjim
#22 toolbar-shadow-removed.png57.75 KBmcjim
#21 toolbar-do-not-hide-table-headers-v2.patch841 bytesmarvil07
Passed: 13269 passes, 0 fails, 0 exceptions View
#20 toolbar-collapsed.png9.55 KBmarvil07
#20 toolbar-expanded.png11.87 KBmarvil07
#19 toolbar-7px.png4.87 KBmarvil07
#19 toolbar-10px.png5.07 KBmarvil07
#19 toolbar-do-not-hide-table-headers.patch1.93 KBmarvil07
Failed: 12996 passes, 1 fail, 0 exceptions View
#16 toolbar.png707 bytesdavyvdb
#16 Picture 1.png20.83 KBdavyvdb
#11 toolbar.png1.43 KByoroy
#11 5pxdropshadow.patch560 bytesyoroy
Passed: 13238 passes, 0 fails, 0 exceptions View
#9 toolbar.png12.75 KBcatch
#8 toolbar.png22.21 KBcatch
toolbar.png22.21 KBcatch

Comments

psicomante’s picture

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

brianV’s picture

Agreed. It's just too much!

Stefan Nagtegaal’s picture

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

catch’s picture

It doesn't separate though, it bleeds in, that's the whole problem.

Bojhan’s picture

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

Stefan Nagtegaal’s picture

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

Gr

catch’s picture

It's FF 3.0 on Ubuntu Jaunty.

catch’s picture

Title: Dropshadow in toolbar is distracting » Dropshadow in toolbar is distracting and prevents selecting elements underneath it
Priority: Normal » Critical
FileSize
22.21 KB

On FF3.0/ubuntu I'm unable to select the checkbox, despite having just scrolled to it, because the z-index of the toolbar is higher and is over it, despite the checkbox being fully visible on my screen. Bumping to critical.

edit: wrong screenshot, see next post.

catch’s picture

FileSize
12.75 KB

yoroy’s picture

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.

yoroy’s picture

Issue tags: +Needs design review
FileSize
560 bytes
Passed: 13238 passes, 0 fails, 0 exceptions View
1.43 KB

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.

brianV’s picture

+1 for yoroy's adjustment. Much better!

yoroy’s picture

Status: Active » Needs review

forgot status, haz patch

webchick’s picture

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'?

Zarabadoo’s picture

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.

davyvdb’s picture

FileSize
20.83 KB
707 bytes

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

davyvdb’s picture

Priority: Critical » Normal

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

catch’s picture

Priority: Normal » Critical

Critical
When a bug renders a module, a core system, or a popular function unusable.

When you can't click a checkbox, that's unusable.

marvil07’s picture

Title: Dropshadow in toolbar is distracting and prevents selecting elements underneath it » Dropshadow in toolbar is distracting and prevents selecting elements underneath it and hide sticky table headers
FileSize
1.93 KB
Failed: 12996 passes, 1 fail, 0 exceptions View
5.07 KB
4.87 KB

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.

marvil07’s picture

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

marvil07’s picture

FileSize
841 bytes
Passed: 13269 passes, 0 fails, 0 exceptions View

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

mcjim’s picture

FileSize
57.75 KB
58.36 KB
57.77 KB
2.04 KB
2.19 KB
Failed: Invalid PHP syntax in modules/toolbar/toolbar.js. View

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

mcjim’s picture

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

TheRec’s picture

Status: Needs work » Needs review
FileSize
2.17 KB
2.96 KB
2.65 KB
2.51 KB
1.55 KB
Passed: 13282 passes, 0 fails, 0 exceptions View

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 :)

TheRec’s picture

FileSize
736 bytes

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.

sun’s picture

Great work, TheRec!

That's the way to go.

Dries’s picture

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.

TheRec’s picture

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 ;)

yoroy’s picture

Status: Needs review » Needs work

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.

TheRec’s picture

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.

sun’s picture

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.

yoroy’s picture

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.

mcrittenden’s picture

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.

yoroy’s picture

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.

mcrittenden’s picture

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.

rickvug’s picture

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.

jbrown’s picture

subscribing

yoroy’s picture

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.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
694 bytes
16.62 KB
751 bytes
PASSED: [[SimpleTest]]: [MySQL] 17,370 pass(es). View

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

yoroy’s picture

FileSize
537 bytes

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:
dropshadow-before

Patch and image from #40:

Patch from #40 with image as attached here:
dropshadow

mcrittenden’s picture

Title: Dropshadow in toolbar is distracting and prevents selecting elements underneath it and hide sticky table headers » Dropshadow in toolbar is distracting and prevents selecting elements underneath it and hides sticky table headers
Status: Needs review » Reviewed & tested by the community

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

Dries’s picture

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

mcrittenden’s picture

Status: Reviewed & tested by the community » Needs work

Per Dries' feedback.

yoroy’s picture

Status: Needs work » Needs review

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?

jbrown’s picture

I agree with sun in #32.

Dries’s picture

What would the CSS3-based dropshadow look like?

sun’s picture

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

TheRec’s picture

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 ? ;)

catch’s picture

This issue was about the height of the dropshadow which #40 + #41 fixed, someone should open a separate issue for converting it to CSS3, I don't see what was wrong with the original RTBC. Nor do I see how actually being able to click checkboxes, and etc., is messier.

jbrown’s picture

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.

webchick’s picture

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

mcrittenden’s picture

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

Dries’s picture

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

jbrown’s picture

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

jbrown’s picture

FileSize
387 bytes
2.46 KB
FAILED: [[SimpleTest]]: [MySQL] 17,371 pass(es), 2 fail(s), and 0 exception(es). View

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.

jbrown’s picture

FileSize
2.48 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in toolbar.tpl.php. View

Actually, this patch fixes the drawer on IE.

yoroy’s picture

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.

jbrown’s picture

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.

jide’s picture

FileSize
2.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch toolbar-shadow_1.patch. View

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.

jbrown’s picture

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.

catch’s picture

There's precedent for browser-specific CSS3 like border-radius - because all the major non-MS browsers are implementing these until there's an agreed standard -so that they can support the new standard without breaking backwards compatibility when it's finalised.

That's quite a lot different to using filter for IE though, I'd rather IE users get a degraded experience than add cruft there.

jide’s picture

FileSize
2.58 KB
PASSED: [[SimpleTest]]: [MySQL] 17,382 pass(es). View

Adding khtml support should not hurt.

jbrown’s picture

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

jide’s picture

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.

jide’s picture

FileSize
2.48 KB
PASSED: [[SimpleTest]]: [MySQL] 17,384 pass(es). View
264.24 KB

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.

mcrittenden’s picture

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?)

jide’s picture

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.

jide’s picture

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 ?

jide’s picture

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

jide’s picture

Status: Needs review » Needs work

Status change.

Noyz’s picture

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

jide’s picture

FileSize
26.09 KB
28.21 KB
35.93 KB
4.15 KB
PASSED: [[SimpleTest]]: [MySQL] 17,381 pass(es). View

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.

jide’s picture

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

xmacinfo’s picture

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')";
xmacinfo’s picture

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.

jide’s picture

@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 !).

jide’s picture

Status: Needs work » Needs review
FileSize
22.08 KB
21.72 KB
23.22 KB
4.05 KB
PASSED: [[SimpleTest]]: [MySQL] 17,374 pass(es). View

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.

Dries’s picture

Priority: Critical » Normal

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.

jide’s picture

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.

jide’s picture

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.

catch’s picture

Well the issue is that you scroll to it, then try to click it, find out it's unclickable, then have to scroll back up again to be able to click. This is all assuming you realise that the shadow is what's causing the checkbox to be unclickable - shadows in real life tend not to be physically tangible :p

I guess it doesn't actually stop you clicking the checkbox, so fair enough on the priority change, but it's really, really infuriating, so I very much hope this gets fixed. We can avoid the browser hack by just not making up for IE's shortcomings, as the toolbar already does with active state links.

sun’s picture

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

jide’s picture

FileSize
3.99 KB
PASSED: [[SimpleTest]]: [MySQL] 17,324 pass(es). View

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")) {
mcrittenden’s picture

Status: Needs review » Needs work

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

jide’s picture

That is what I thought, but it works fine.

sun’s picture

Status: Needs work » Reviewed & tested by the community

Thanks!

jbrown’s picture

Status: Reviewed & tested by the community » Needs work

this still has issues - will detail in a few minutes

jbrown’s picture

Status: Needs work » Needs review
FileSize
387 bytes
3.78 KB
PASSED: [[SimpleTest]]: [MySQL] 17,325 pass(es). View

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.

jide’s picture

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.

jbrown’s picture

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.

jbrown’s picture

FileSize
6.15 KB

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.

jide’s picture

FileSize
3.78 KB
PASSED: [[SimpleTest]]: [MySQL] 17,305 pass(es). View
387 bytes
20.54 KB
20.36 KB

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.

jbrown’s picture

FileSize
5.48 KB

Cool! Great work jide.

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

mcrittenden’s picture

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.

jide’s picture

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

jide’s picture

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.

mcrittenden’s picture

Title: Dropshadow in toolbar is distracting and prevents selecting elements underneath it and hides sticky table headers » Dropshadow in toolbar is distracting and prevents selecting elements underneath it
Status: Needs review » Reviewed & tested by the community

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

jbrown’s picture

This patch rocks!

RTBC++

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

Should anyone else be on the list?

jide’s picture

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.

Dries’s picture

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

jide’s picture

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.

jbrown’s picture

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

jide’s picture

FileSize
45.64 KB

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

jbrown’s picture

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

jide’s picture

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

Bojhan’s picture

Status: Reviewed & tested by the community » Needs work

See Dries his review.

jide’s picture

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.

mcrittenden’s picture

Status: Needs work » Needs review

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.

jide’s picture

Status: Needs review » Needs work

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.

jide’s picture

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

jbrown’s picture

Status: Needs work » Reviewed & tested by the community

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.

jide’s picture

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.

jide’s picture

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

jide’s picture

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

jide’s picture

FileSize
3.74 KB
PASSED: [[SimpleTest]]: [MySQL] 17,538 pass(es). View

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

jbrown’s picture

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.

jide’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.67 KB
PASSED: [[SimpleTest]]: [MySQL] 17,528 pass(es). View

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.

jbrown’s picture

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

jide’s picture

Still needs toolbar.png from #93.

jide’s picture

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

jbrown’s picture

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?

jide’s picture

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.

jbrown’s picture

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

jide’s picture

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)

jbrown’s picture

Works great for me on IE7.

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

mcrittenden’s picture

Status: Needs review » Reviewed & tested by the community

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

jbrown’s picture

FileSize
330 bytes

Reduced toolbar.png from 387 to 330 bytes.

webchick’s picture

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

jbrown’s picture

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!

jide’s picture

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

yoroy’s picture

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.

jide’s picture

jbrown’s picture

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.

yoroy’s picture

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.

jbrown’s picture

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

jide’s picture

Edit : Hmmm, forget me.

jbrown’s picture

Title: Dropshadow in toolbar is distracting and prevents selecting elements underneath it » Use CSS3 / IE filter to render toolbar shadow

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

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Great. Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

reglogge’s picture

Status: Closed (fixed) » Needs work

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.

marvil07’s picture

Status: Needs work » Closed (fixed)

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