616156 - Navigation button changes wrongly affect all toolbars

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Mike Kaply [:mkaply]]

See attached image.

Addon toolbars look pretty bad on Firefox 4 on Windows (borders around all the buttons).

Looking at the mocks, I see this mockup:

https://wiki.mozilla.org/File:Firefox-4-Mockup-i06-(Win7)-(Aero)-(TabsBottom)-(ExtraToolbars).png

But the two toolbars that are in this mock don't use the "default" toolbar styles.

At:

https://developer.mozilla.org/en/Creating_toolbar_buttons

It says:

> class="toolbarbutton-1" makes the toolbar button appear correctly in Icons and Text mode; it also adjusts padding.

What we're finding is that when we follow the correct instructions, we get this behavior.

Was it the intention to change the appearance of all toolbar buttons, or just the firefox default navigation buttons?

Comment 1

[Mike Kaply [:mkaply]]

Changing Component to try to get some eyeballs on this.

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

Note that I'm using ":-moz-any(#nav-bar) .toolbarbutton-1" instead of "#nav-bar .toolbarbutton-1" in order to decrease the specificity of the selector, so that it can be overridden by "#nav-bar #back-button" for instance.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

I don't really understand the problem or the proposed solution.

Is the problem that addon toolbars want some of the behavior fixes of toolbarbutton-1 (icons+text), but not the styling (e.g. borders)? And so your patch makes the styling parts of toolbarbutton-1 only apply to nav-bar buttons? Won't that have a negative impact on our default buttons, when they are moved to custom toolbars? I also don't understand why the override-ability (comment 2) is desired/needed.

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #3)
> Is the problem that addon toolbars want some of the behavior fixes of
> toolbarbutton-1 (icons+text), but not the styling (e.g. borders)?

That's the primary concern here, yes.

> And so your
> patch makes the styling parts of toolbarbutton-1 only apply to nav-bar buttons?
> Won't that have a negative impact on our default buttons, when they are moved
> to custom toolbars?

It has impact, but a desired one. Right now we explicitly remove the styling for toolbars that users will likely move these buttons to (tab bar, addon bar). Basically I think we only really want it on the nav bar.

> I also don't understand why the override-ability (comment
> 2) is desired/needed.

Because the theme contains selectors like #nav-bar #back-button (#nav-bar:not([iconsize="small"])[mode="icons"] #back-button to be more accurate, in this very patch). This is the more specific case with adjusted margins, gradients, etc., whereas :-moz-any(#nav-bar) .toolbarbutton-1 is the generic case.

Comment 5

[Frank Yan (:fryn)]

Comment on attachment 499537 [details] [diff] [review]
patch

It is fragile to rely on a bug in the specificity of -moz-any(): bug 561154

dbaron has noted that -moz-any() is supposed to apply the correct specificity for each of its arguments.

Comment 6

[Dão Gottwald [:dao]]

Attachment: patch v2

Oops, I got confused about which selectors #nav-bar .toolbarbutton-1 overrides. There's actually no reason to exploit bug 561154 here.

Comment 7

[Ben Bucksch (:BenB)]

The basic problem here is that the Firefox team wanted a certain styling for their navbar, namely with big visible borders - which is fine. The same style, however, looks very ugly for custom toolbars with lots of buttons - so ugly that it's a release blocker for us (us = one toolbar).

So, making that new style specific to the navbar is the correct fix.

Thanks a lot dao!

Comment 8

[tymerkaev]

Comment on attachment 499587 [details] [diff] [review]
patch v2

Are you sure that you didn't forget about zoom controls buttons border-radius rules?

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> Comment on attachment 499587 [details] [diff] [review]
> patch v2
> 
> Are you sure that you didn't forget about zoom controls buttons border-radius
> rules?

You're right, that stuff is only wanted when the buttons are on the nav bar, although it doesn't hurt to apply it unconditionally.

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch v3

updated the selectors for the zoom in/out buttons

Comment 11

[Frank Yan (:fryn)]

Comment on attachment 499676 [details] [diff] [review]
patch v3

Haven't actually tested it, but the code looks okay to me.

Comment 12

[Dietrich Ayala (:dietrich)]

Though referenced in comment #0, there's no attached image. Can't evaluate for blocking without a better idea of how bad this actually looks. Can someone attach a screenshot?

Comment 13

[Mike Kaply [:mkaply]]

Attachment: Picture of problem

Unfortunately this is not the original attachment. It showed some other toolbars to show the problem.

Bugzilla doesn't attach an attachment on creation of a bug if you don't post any comments. Doh.

I'm trying to locate the original attachment.

Comment 14

[Justin Dolske [:Dolske]]

Comment on attachment 499676 [details] [diff] [review]
patch v3

>+.toolbarbutton-1[disabled="true"] > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
>+.toolbarbutton-1[disabled="true"] > .toolbarbutton-icon {
>+  opacity: .4;
>+}

The existing rule for this (where you've now added #nav-bar) has opacity .5. Intentional change?

>-.toolbarbutton-1:not([type="menu-button"]) {
>-  -moz-box-orient: vertical;
>-  list-style-image: url("chrome://browser/skin/Toolbar.png");
>-}

You're now applying list-style-image to all .toolbarbutton-1 classes, even when type=menu-button. Presumably this isn't breaking anything in the default browser's buttons (since I don't see it used anywhere), but couldn't this cause addons to suddenly get an unwanted image in their buttons?

r+ assuming these two questions have answers.

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #14)
> Comment on attachment 499676 [details] [diff] [review]
> patch v3
> 
> >+.toolbarbutton-1[disabled="true"] > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
> >+.toolbarbutton-1[disabled="true"] > .toolbarbutton-icon {
> >+  opacity: .4;
> >+}
> 
> The existing rule for this (where you've now added #nav-bar) has opacity .5.
> Intentional change?

It had 50% opacity on the icon and 80% on the button, resulting in a 40% net opacity.

> >-.toolbarbutton-1:not([type="menu-button"]) {
> >-  -moz-box-orient: vertical;
> >-  list-style-image: url("chrome://browser/skin/Toolbar.png");
> >-}
> 
> You're now applying list-style-image to all .toolbarbutton-1 classes, even when
> type=menu-button. Presumably this isn't breaking anything in the default
> browser's buttons (since I don't see it used anywhere), but couldn't this cause
> addons to suddenly get an unwanted image in their buttons?

Only if they don't specify their own icon, which would mean they're already broken.

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/bc2e1dd4bf50

Comment 17

[Ben Bucksch (:BenB)]

Thanks, dao!

Comment 18

[Jim Jeffery not reading bug-mail 1/2/11]

Could I suppose be an icon issue, but the Icon for Flashblock is borked.  Not sure this bug is totally responsible but dao's checkin with cset:
b792669a9218 contained a group of patches, this one seems most likely...

Does not matter if the Icon/button is placed in navbar or the new appbar - its still distorted beyond recognition.  Large/small icons make no change.

image: http://img440.imageshack.us/img440/5870/flashblock.png 
Flashblock is the button to the right of the bookmarks... Should be a letter 'f' with an 'x' in red letters.

Comment 19

[Jim Jeffery not reading bug-mail 1/2/11]

This is the normal Icon/button for Flashblock

http://img255.imageshack.us/img255/6743/flashblockgood.png

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #18)
> Could I suppose be an icon issue, but the Icon for Flashblock is borked.  Not
> sure this bug is totally responsible but dao's checkin with cset:
> b792669a9218 contained a group of patches, this one seems most likely...
> 
> Does not matter if the Icon/button is placed in navbar or the new appbar - its
> still distorted beyond recognition.  Large/small icons make no change.
> 
> image: http://img440.imageshack.us/img440/5870/flashblock.png 
> Flashblock is the button to the right of the bookmarks... Should be a letter
> 'f' with an 'x' in red letters.

Err, yes, the patch sets Toolbar.png on .toolbarbutton-1 > .toolbarbutton-menubutton-button (in addition to .toolbarbutton-1), preventing inheritance of the list-style-image set by extensions.

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #20)
fixed: http://hg.mozilla.org/mozilla-central/rev/141d92d974b4

Comment 22

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to comment #21)
> (In reply to comment #20)
> fixed: http://hg.mozilla.org/mozilla-central/rev/141d92d974b4

Thanks for the quick-fix,  verified fixed using the cset from comment 21

Comment 23

[Gomita]

Attachment: the buttons in toolbar customization palette

The patch to remove borders affects the buttons in toolbar customization palette.
See the attached image.
I think, the buttons in the palette should keep having borders.

Additionally, for extension authors, it is preferred that adding a class (like 'toolbar-with-borders') or attribute (like 'iconborder="true"') to xul:toolbar instead of excluding other toolbars except #nav-bar.

Comment 24

[Ben]

Verified with Mozilla/5.0 (Windows NT 5.1; rv:2.0b11) Gecko/20100101 Firefox/4.0b11 ID:20110203141415