- overly verbose selectors like div#toolbar div.toolbar-menu #toolbar-menu { ... } really don't serve any purpose aside from making it really annoying to override them using a theme css, as those would have to be as verbose or even more verbose than the original one

- locking the toolbar's height with a px value and having overflow:hidden breaks text-only zooming (this doesn't apply to span.toggle as that contains a background image that doesn't scale with text-zooming anyway)

- there are a bunch of position:absolute which really aren't necessary and tend to cause overlapping if anything hits the fan

- these IE6 hacks are hurting my eyes, will see if there is any way we can have a module add IE6 conditional CSS in some sort of sane way

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TheRec’s picture

Great initiative seutje :) As I told you on IRC I'll be happy to review and test it.

Last time we discussed the IE hacks with Damien Tournoud, he told me the hacks did not get in a conditional CSS because these are not supported by CSS aggregation. But maybe it is possible to add them in <head> manually with drupal_add_html_head(), it wouldn't be very pretty, but at least less ugly then CSS hacks ;)

For the rest, I think all your concerns are legitimate and if they can be corrected it would be a great improvement !

seutje’s picture

Assigned: seutje » Unassigned
Status: Active » Needs review
FileSize
18.55 KB
3.15 KB
34.89 KB
35.25 KB
34.32 KB
34.5 KB
34.71 KB
35.28 KB
31.46 KB
31.56 KB
31.64 KB
31.77 KB
3.52 KB

@1 : themes generally print head before css, which would cause the conditionals to be overridden anyway :(

however: I managed to get rid of most of the hacks, as IE6 automatically makes position:fixed into position:static because fixed is unrecognised, so it falls back to the default value, which is static. and empty divs that don't have an explicit width and are position:absolute don't show up in IE6, so no need to explicitly hide it

additional changes:
- Made nearly all selectors weaker so they are easier to override by contrib themes/modules
- Changed a bunch of px values to relative values to account for text-only zooming
- Removed nearly all overflow:hidden
- Removed nearly all postion:absolute and used floating instead, this also allowed me to remove the "left" and "right" values aswel
- Added proper CSS3 border-radius, left the proprietary ones in there though
- Removed the use of the background-image on div.toolbar-menu as this background was plain black but sprited which broke text-zooming, using a background image for a plain color is just silly

Tested (see before&after screenshots):
- IE6 (6.00.2900 and 6.00.3790 screenshots are 2900)
- IE7 (7.00.5730)
- IE8 (8.00.6001)
- FF3.5 (3.5.3)
- SF4 (4.0.3)

Things I didn't change, but which should be changed:
- Seven uses a different base font-size than Garland does, so things like the toolbar that persist over regular interface and admin interface show up in a different size if they use relative sizes (Garland uses body { font-size: 76%; } whereas Seven uses body { font-size: 0.8em; }) -> wil open a separate issue for this
- Active links in the top use a sprited background image with a gradient, this breaks with text-zooming so I'd propose to change this to a regular background color (see attached screenshot, Structure uses the current sprited bg-image, People uses background: #999;, text-zoomed twice to illustrate the problem
- The shortcuts <ul>-wrapper still has a px height and overflow:hidden, but this happens in shortcut.css so I didn't touch that just yet (but looks like that one could also use an overhaul)

This patch also fixes the wrapping (or the lack of) but exposes the problem we have with this padding-top solution on the body. When the items wrap, the toolbar expands vertically, but the padding-top on the body does not expand accordingly, causing the toolbar to overlap part of the page, this is still a big problem imho (see attached screenshot)

You might notice a slight inconsistency in the IE8 before & after, I think this is due to the way IE8 does sub-pixel rounding but I'm not entirely sure on this reason... imo this small inconsistency is pretty much irrelevant

toolbar.css filesize reduced by 10% (YEHAW!)

Status: Needs review » Needs work

The last submitted patch failed testing.

seutje’s picture

lovely, 1 line commit broke my patch :( -> #607106: Moving help to the right , in specific the patch in #10

should I account for this change in a proper margin way? or are we dead set on having 79578965867 elements with position:absolute; left:somevalue; top:somevalue; ...?

seutje’s picture

Status: Needs work » Needs review
FileSize
3.85 KB

attempt to account for the change in #607106: Moving help to the right

also changed the comments on the IE6 hack to reflect the changes

screenshots pretty much still apply, xcept that there's a lil more space between the toggle button and the toolbar-user

seutje’s picture

FileSize
3.79 KB

accidentally changed $Id$ line, fixed

TheRec’s picture

Tested in the same browsers and I got the same results so far.
Tested in Opera 10, the issues brought up by seutje are the same (except the ones related only to IE of course). Screenshots attached.

The wrapping issue seems to be the toughest one to solve only with CSS, but the situation currently is no better as the toolbar functionality is completely lost when things are overlapping. Maybe we could consider having the "position: fixed" replaced by a Javascript equivalent (or even just apply this property through JS), but that would be a bit of regression for users without Javascript :(. For them the toolbar would just be pushing the content down and always stick at top of the page (it wouldn't follow when the user would use the vertical scrollbar). With Javascript we could take advantage of the existing Drupal.admin.toolbar.height function to define the correct padding-top for the <body>. I mark the issue needs work, until we decide how this must be handeled now that the testbot has done its job.

Status: Needs work » Needs review

seutje requested that failed test be re-tested.

Status: Needs review » Needs work

The last submitted patch failed testing.

seutje’s picture

Status: Needs work » Needs review
FileSize
3.74 KB

re-roll

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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