Navigation button changes wrongly affect all toolbars

VERIFIED FIXED in Firefox 4.0b10

Status

()

defect
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: mkaply, Assigned: dao)

Tracking

Trunk
Firefox 4.0b10
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker][fx4-fixed-bugday])

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

9 years ago
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?
Reporter

Comment 1

9 years ago
Changing Component to try to get some eyeballs on this.
Component: Extension Compatibility → Theme
QA Contact: extension.compatibility → theme
Assignee

Updated

9 years ago
Assignee: nobody → dao
Blocks: 589236
Status: NEW → ASSIGNED
Assignee

Comment 2

9 years ago
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #499537 - Flags: review?(gavin.sharp)
Assignee

Updated

9 years ago
Attachment #499537 - Flags: review?(shorlander)
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.
Assignee

Comment 4

9 years ago
(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

9 years ago
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.
Assignee

Comment 6

9 years ago
Posted patch patch v2 (obsolete) — Splinter Review
Oops, I got confused about which selectors #nav-bar .toolbarbutton-1 overrides. There's actually no reason to exploit bug 561154 here.
Attachment #499537 - Attachment is obsolete: true
Attachment #499587 - Flags: review?(gavin.sharp)
Attachment #499537 - Flags: review?(shorlander)
Attachment #499537 - Flags: review?(gavin.sharp)
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

9 years ago
Comment on attachment 499587 [details] [diff] [review]
patch v2

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

Comment 9

9 years ago
(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.
Assignee

Comment 10

9 years ago
Posted patch patch v3Splinter Review
updated the selectors for the zoom in/out buttons
Attachment #499587 - Attachment is obsolete: true
Attachment #499676 - Flags: review?(gavin.sharp)
Attachment #499587 - Flags: review?(gavin.sharp)
Assignee

Updated

9 years ago
blocking2.0: --- → ?
Comment on attachment 499676 [details] [diff] [review]
patch v3

Haven't actually tested it, but the code looks okay to me.
Attachment #499676 - Flags: feedback?(fryn) → feedback+
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?
Reporter

Comment 13

9 years ago
Posted image 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.
blocking2.0: ? → final+
Whiteboard: [softblocker]
Assignee

Updated

9 years ago
Attachment #499676 - Flags: review?(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.
Attachment #499676 - Flags: review?(gavin.sharp)
Attachment #499676 - Flags: review?(dolske)
Attachment #499676 - Flags: review+
Assignee

Comment 15

9 years ago
(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.
Assignee

Comment 16

9 years ago
http://hg.mozilla.org/mozilla-central/rev/bc2e1dd4bf50
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b10
Thanks, dao!
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.
Assignee

Comment 20

9 years ago
(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.
(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

9 years ago
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.
Reporter

Updated

9 years ago
Depends on: 630226
Assignee

Updated

9 years ago
No longer depends on: 630226

Comment 24

9 years ago
Verified with Mozilla/5.0 (Windows NT 5.1; rv:2.0b11) Gecko/20100101 Firefox/4.0b11 ID:20110203141415
Status: RESOLVED → VERIFIED
Whiteboard: [softblocker] → [softblocker][fx4-fixed-bugday]
You need to log in before you can comment on or make changes to this bug.