Make WordPress Core

Opened 14 years ago

Closed 14 years ago

#15851 closed defect (bug) (fixed)

Admin Bar Is Not Properly Reset

Reported by: johnonolan's profile JohnONolan Owned by: ocean90's profile ocean90
Milestone: 3.1 Priority: high
Severity: normal Version: 3.1
Component: UI Keywords: has-patch commit
Focuses: Cc:

Description (last modified by JohnONolan)

New admin bar is being affected by global styles applied by themes and plugins.

Eg. Margin applied to all forms: http://cl.ly/3fCb

Attachments (6)

15851.diff (350 bytes) - added by JohnONolan 14 years ago.
15851.2.diff (362 bytes) - added by JohnONolan 14 years ago.
Fixes bug raised in #15851
15875.diff (397 bytes) - added by duck_ 14 years ago.
15851.reset.patch (4.1 KB) - added by ocean90 14 years ago.
15851.reset2.patch (3.1 KB) - added by JohnONolan 14 years ago.
2nd pass at #admin-bar reset CSS
15851.reset3.patch (4.7 KB) - added by ocean90 14 years ago.
With Opera fix

Download all attachments as: .zip

Change History (41)

@JohnONolan
14 years ago

#1 @JohnONolan
14 years ago

  • Keywords has-patch added

Patch resolves the issue by resetting the margin.

#2 @nacin
14 years ago

(In [17017]) More admin bar css reset. props JohnONolan, see #15851.

#3 follow-up: @nacin
14 years ago

Can we run through admin-bar.dev.css and figure out what else we need to reset?

#4 @nacin
14 years ago

  • Keywords needs-testing added; ux-feedback has-patch removed

#5 @ocean90
14 years ago

I will test now the wp.com and wp.org themes.

#6 @nacin
14 years ago

  • Owner set to ocean90
  • Status changed from new to assigned

ocean90 will have a patch in the morning.

#7 in reply to: ↑ 3 @duck_
14 years ago

Replying to nacin:

Can we run through admin-bar.dev.css and figure out what else we need to reset?

Related #15875

@JohnONolan
14 years ago

Fixes bug raised in #15851

#8 @JohnONolan
14 years ago

  • Description modified (diff)
  • Summary changed from Admin Bar Search form Is Not Properly Reset to Admin Bar Is Not Properly Reset

@duck_
14 years ago

#9 @duck_
14 years ago

15875.diff from #15875

11:37 < JohnONolan> duck_: your patch is better than mine, could you reupload to the main admin bar css reset ticket? :)

#10 @ocean90
14 years ago

Thx, but I already have it in my patch. It should be ready in an hour.

Last edited 14 years ago by ocean90 (previous) (diff)

#11 @ocean90
14 years ago

  • Keywords has-patch added

#12 @JohnONolan
14 years ago

Not sure what content:normal is going to do - also seems that there are a lot more things which could be reset. Font-variant, border, position, etc.

#13 @filosofo
14 years ago

The :before pseudo-element shows how this could end up being a war of attrition. Rather than try to reset every possible standard, proposed standard, or proprietary CSS property, I suggest the following:

  • Make sure it works with the top say 25 most popular themes from the repository.
  • Make sure it works with handful of the well-known "premium" theme names.
  • Let the others adjust their stylesheets accordingly, particularly those who have poorly-thought-out rules (like the :before example, which should be better-targeted).

#14 @JohnONolan
14 years ago

Theme authors should not have to restrict any of their CSS based on something in core being badly coded and getting overridden, that's a recipe for disaster. It's not a theme author's fault if their theme's CSS breaks the admin bar, it's the admin bar's fault.

#15 @ocean90
14 years ago

JohnONolan: http://www.w3schools.com/Css/pr_gen_content.asp

filosofo: My patch works at the moment with all wp.com themes. Also +1 to your suggest. It would be easier, if theme authors doesn't use something like input {line-height: 1.4em !important}. So it's more a theme author's fault, if they break the bar.

#16 follow-up: @filosofo
14 years ago

JohnONolan: But "badly coded" is the key. It is the theme author's fault if she is using a rule like li:before, because it applies to every list element, globally. A rule like that unquestionably could be better targeted. Furthermore, the admin bar is an immediate child of the body element, so it's trivial for theme authors to avoid targeting it.

Besides, practically speaking it will be impossible to account for every obscure CSS variation without a ridiculously-bloated CSS ruleset. That's why I suggest striking the golden mean that has a smallish number of rules yet a minimum of disruption.

ocean90: the wp.com theme repository has 119 themes. Did you really test all of them? In what browsers?

#17 follow-up: @JohnONolan
14 years ago

Ocean90: so why normal and not none? Seems strange.

Filosofo: I completely disagree. Theme authors didn't ask for an admin bar to get in their way, and they aren't writing CSS for the admin bar. They're writing CSS for their own layout, so the CSS should only affect their own layout. The only time a theme or plugin's CSS should affect the admin bar is if it's prepended with .admin-bar.

A "ridiculously bloated" CSS file is a complete overstatement - it's 12kb vs 8kb, and is only ever going to be loaded by logged in users - size is not an issue. I agree with ocean90 that !important rules can't be accounted for, but everything else should be.

Is it neat? No. Is it necessary? In my opinion, absolutely.

Last edited 14 years ago by JohnONolan (previous) (diff)

#18 in reply to: ↑ 17 @filosofo
14 years ago

Replying to JohnONolan:

Filosofo: I completely disagree. Theme authors didn't ask for an admin bar to get in their way, and they aren't writing CSS for the admin bar. They're writing CSS for their own layout, so the CSS should only affect their own layout. The only time a theme or plugin's CSS should affect the admin bar is if it's prepended with .admin-bar.

John, I'm not sure what you're disagreeing with. Do you really think we should account for every possible rule that could be applied to the admin bar?

Theme developers in general are not just writing CSS for their own layout. They're writing it for an unknown WordPress site, which could have any number of unknown possibilities added by plugins and such. It's their responsibility to write themes in a manner that plays nicely with the multitude of unknown possibilities, and that means for CSS that it should be as specific as possible.

The theme developers who are designing for a particular site (and not a general audience) are not as much of a concern, because they can just globally disable the admin bar, if that's what they want.

A "ridiculously bloated" CSS file is a complete overstatement - it's 12kb vs 8kb, and is only ever going to be loaded by logged in users - size is not an issue.

I'm not sure where the 12kb comes from. No proposed patch yet resets all of the dozens of commonly-used CSS properties, much less every possibility. You've heard about :before today; it will be something else tomorrow, until we account for every possibility. I'm saying that such a war of attrition is pointless. Rather, try to account for a reasonable number, not every possible scenario. Surely you don't disagree.

And bandwidth increase is not something to sniffed at. It's much easier for a site admin to adjust her theme to fix a poor selector than try to redo the admin bar CSS to reduce bandwidth. Multiply a few kb by thousands of users on a social site (so they're usually logged in), and you're talking about real money, money being spent in the fear that someone might have a poorly-constructed CSS rule.

Is it neat? No. Is it necessary? In my opinion, absolutely.

In what sense is it necessary? By fixing the scenarios with popular themes, most people won't have to change anything. And the others--well, they're used to making changes. Every significant WP release has involved changes to theme authors' best practices. They'll account for it easily enough as they always have.

#19 @JohnONolan
14 years ago

I think we're saying the same thing - but we have a different definition of "middle ground" - we should account for a lot more properties than present in the existing patch. Not "every possible scenario" - but a lot more than we have right now.

#20 in reply to: ↑ 16 ; follow-up: @ocean90
14 years ago

Replying to filosofo:

ocean90: the wp.com theme repository has 119 themes. Did you really test all of them? In what browsers?

Sure, https://wpcom-themes.svn.automattic.com/ + Theme Switcher. Tested with Firefox, Chrome and sometimes with IE. Yeah, I have too much time. :P

#21 follow-up: @pablox
14 years ago

I believe when you design a wordpress theme you shouldn't keep in mind if your styles affect or no the admin bar, makes way more sense that if you want to affect it, preppend the #admin-bar (as JohnONolan said before). I don't understand why a theme developer should have the admin bar in mind, unless of course he wants to focus on it.

Obviously we should have an equilibrium between "bloated" css and simplifying the work for the theme designers, I believe that "reseting" a propriety like in this case li :before { content: none; } is not that we're bloating it.

#22 in reply to: ↑ 20 @filosofo
14 years ago

Replying to ocean90:

Sure, https://wpcom-themes.svn.automattic.com/ + Theme Switcher. Tested with Firefox, Chrome and sometimes with IE. Yeah, I have too much time. :P

Very nice.

#23 in reply to: ↑ 21 @filosofo
14 years ago

Replying to pablox:

I believe when you design a wordpress theme you shouldn't keep in mind if your styles affect or no the admin bar, makes way more sense that if you want to affect it, preppend the #admin-bar (as JohnONolan said before). I don't understand why a theme developer should have the admin bar in mind, unless of course he wants to focus on it.

Is this a theme for WordPress? Then you should account for HTML elements that are common in WordPress. In the past theme developers have had to account for widget classes, more recently the menu classes, and now they will have to handle admin bar classes. It's part of what it means to develop themes for WordPress.

Obviously we should have an equilibrium between "bloated" css and simplifying the work for the theme designers, I believe that "reseting" a propriety like in this case li :before { content: none; } is not that we're bloating it.

You believe that the :before pseudo element should have global scope; someone else believes that some other CSS rule should have global scope. Where does it stop? That's what has to be decided, and I have suggested common usage and a rough methodology for determining that usage, as the criteria. All I'm arguing for is that we have a criterion or two, rather than either trying to account for all scenarios or handling bugs as they are reported.

#24 @JohnONolan
14 years ago

No, it's not the same as widgets. You style widgets however you want, so you *expect* your styling to affect them. The admin bar is pre-styled and is not meant to inherit any of your styles.

They are not the same and should not be treated as such...

#25 @filosofo
14 years ago

pablox says, "I don't understand why a theme developer should have the admin bar in mind," and I was saying why the theme developer needs to keep it in mind, just like the theme developer needs to be aware of those other elements. The general principal is the same: being aware of likely ways in which your styling will affect a typical WordPress site.

If the theme developer chooses not to be aware of widget list elements or nested nav menu elements, the theme will be a mess for common usage. That's analogous to the theme developer who chooses not to be aware of the admin bar.

Besides, there are many ways in which your styling is restricted when making a theme, even with the things I mentioned. Try to make a widgetized sidebar much narrower than 150 pixels, and many third-party widgets will break. Try to set fixed width on site titles or navigational menus, and you'll run into similar issues. Don't use the wysiwyg-given classes for images, or fail to clear floated post elements, and you'll have other problems, etc.

The considerate theme developer is aware of common uses and problems with a WordPress theme and develops accordingly. The admin bar is just another thing to add to the list. Compared to some of the others, it's thankfully trivial to account for.

#26 @nacin
14 years ago

The reset rules, even #adminbar *, are not enough. Rules such as ul li a { margin ... } will break it.

I believe this calls for the nuclear option, !important.

#27 @nacin
14 years ago

Scratch that. Still testing things.

@JohnONolan
14 years ago

2nd pass at #admin-bar reset CSS

#28 @JohnONolan
14 years ago

That covers just about everything, only things added were width/height:auto and position:static to #wpadminbar * - which fixes anyone who sets a static height/width or type of position on somthing like ul li a

And display:inline to #wpadminbar .quicklinks .menupop a > span - which fixes anyone doing something like ul li a span {display:block} (for dropdowns or sliding doors bg image)

#29 @filosofo
14 years ago

I tried 15851.reset2.patch on every popular theme listed in the sidebar on Extend, and they all look pretty good.

#30 @nacin
14 years ago

  • Keywords commit added; needs-testing removed

#31 @ocean90
14 years ago

15851.reset2.patch looks good. The :before/:after reset makes some trouble in Opera, only frontend: http://grab.by/7YoO. I'm working on it.

@ocean90
14 years ago

With Opera fix

#32 @nacin
14 years ago

Can't test this much more than we already have. Nice work all four of you.

#33 @JohnONolan
14 years ago

Agreed - good discussion too. Working with opposing opinions made for a stronger end result :)

#34 @ryan
14 years ago

Looks good.

#35 @nacin
14 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [17064]) Thorough CSS reset of the admin bar to avoid theme conflicts. props ocean90, JohnONolan, filosofo, duck_. fixes #15851.

Note: See TracTickets for help on using tickets.