Closed
Bug 616156
Opened 14 years ago
Closed 14 years ago
Navigation button changes wrongly affect all toolbars
Categories
(Firefox :: Theme, defect)
Tracking
()
VERIFIED
FIXED
Firefox 4.0b10
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: mkaply, Assigned: dao)
References
Details
(Whiteboard: [softblocker][fx4-fixed-bugday])
Attachments
(3 files, 2 obsolete files)
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•14 years ago
|
||
Changing Component to try to get some eyeballs on this.
Component: Extension Compatibility → Theme
QA Contact: extension.compatibility → theme
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 2•14 years ago
|
||
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•14 years ago
|
Attachment #499537 -
Flags: review?(shorlander)
Comment 3•14 years ago
|
||
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•14 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•14 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•14 years ago
|
||
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)
Comment 7•14 years ago
|
||
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 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•14 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•14 years ago
|
||
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•14 years ago
|
blocking2.0: --- → ?
Updated•14 years ago
|
Attachment #499676 -
Flags: feedback?(fryn)
Comment 11•14 years ago
|
||
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+
Comment 12•14 years ago
|
||
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•14 years ago
|
||
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.
Updated•14 years ago
|
blocking2.0: ? → final+
Whiteboard: [softblocker]
Assignee | ||
Updated•14 years ago
|
Attachment #499676 -
Flags: review?(dolske)
Comment 14•14 years ago
|
||
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•14 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•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b10
Comment 17•14 years ago
|
||
Thanks, dao!
Comment 18•14 years ago
|
||
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•14 years ago
|
||
This is the normal Icon/button for Flashblock
http://img255.imageshack.us/img255/6743/flashblockgood.png
Assignee | ||
Comment 20•14 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.
Assignee | ||
Comment 21•14 years ago
|
||
(In reply to comment #20)
fixed: http://hg.mozilla.org/mozilla-central/rev/141d92d974b4
Comment 22•14 years ago
|
||
(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•14 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.
Comment 24•14 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.
Description
•