User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release) Build ID: 20131123030208 Steps to reproduce: Place the zoom buttons in the navbar. Actual results: The buttons use the windows default toolbar button styling. I think this is caused by bug 878065

Could you provide a screenshot of what you mean, and how it differs to the correct styling?

(In reply to :Gijs Kruitbosch from comment #1) > Could you provide a screenshot of what you mean, and how it differs to the > correct styling? OK done :)

oh crap, it didn't catch the hover effect.

Oh, was this meant to just be about the hover effect? Also, Mike, I could swear we had a bug about the buttons being mis-centered in the navbar, but I can't find it.

Well, the :active effect is also affected. Also, I also found another bug, when the widget is in the panel menu, it will always have a bottom border, even when there's nothing after it.

(In reply to ntim007 from comment #7) > Well, the :active effect is also affected. > Also, I also found another bug, when the widget is in the panel menu, it > will always have a bottom border, even when there's nothing after it. That's by design.

(In reply to :Gijs Kruitbosch from comment #6) > Oh, was this meant to just be about the hover effect? > > Also, Mike, I could swear we had a bug about the buttons being mis-centered > in the navbar, but I can't find it. I don't know of a bug for that, but this one will surely cover it. This looks wrong and needs to be fixed. I'll take a look!

(In reply to Mike de Boer [:mikedeboer] from comment #8) > (In reply to ntim007 from comment #7) > > Well, the :active effect is also affected. > > Also, I also found another bug, when the widget is in the panel menu, it > > will always have a bottom border, even when there's nothing after it. > > That's by design. Well, it looks quite awkward. It also misses a top border when there's no split widget before it btw. (In reply to Mike de Boer [:mikedeboer] from comment #9) > (In reply to :Gijs Kruitbosch from comment #6) > > Oh, was this meant to just be about the hover effect? > > > > Also, Mike, I could swear we had a bug about the buttons being mis-centered > > in the navbar, but I can't find it. > > I don't know of a bug for that, but this one will surely cover it. This > looks wrong and needs to be fixed. I'll take a look! Thanks ! I think the fix will be quite easy :) Changing the classes when the widget is in toolbar should be enough.

Gijs, you rightfully questioned the removal of the 'toolbarbutton-1' class for wide-widget buttons and bluntly insisted on removing them in bug 878065. I was wrong!

Comment on attachment 8342307 [details] [diff] [review] Patch 1: restore styling for buttons of wide widgets in toolbars Review of attachment 8342307 [details] [diff] [review]: ----------------------------------------------------------------- So, while I like being right as much as the next person, part of your original justification was that there were rules for these buttons that interfered with what you were doing. Have you checked (cross-platform) that that isn't the case (anymore), as there doesn't seem to be anything in this patch to adjust e.g. specificity of certain rules? ::: browser/components/customizableui/src/CustomizableWidgets.jsm @@ +811,5 @@ > } > }); > } > #endif > +#endif Hmm? ::: browser/themes/osx/browser.css @@ +1432,5 @@ > margin: 0; > } > > +#zoom-controls[cui-areatype="toolbar"]:not(.overflowedItem) > #zoom-reset-button > .toolbarbutton-text { > + padding-top: 8px; Was this intentionally part of this patch?

> So, while I like being right as much as the next person, part of your > original justification was that there were rules for these buttons that > interfered with what you were doing. Have you checked (cross-platform) that > that isn't the case (anymore), as there doesn't seem to be anything in this > patch to adjust e.g. specificity of certain rules? Yeah, it interfered with the zoom-reset button label text vertical centering, but removing this className is a bit too blunt whereas a simple, more specific rule in osx/browser.css fixes it better. I tested for any other possible regression on all platforms (linux, win, osx) for all positions (menu-panel, overflow-panel, nav-bar, any other toolbar). > ::: browser/components/customizableui/src/CustomizableWidgets.jsm > @@ +811,5 @@ > > } > > }); > > } > > #endif > > +#endif Ehm, yeah, I'll remove this. > ::: browser/themes/osx/browser.css > @@ +1432,5 @@ > > margin: 0; > > } > > > > +#zoom-controls[cui-areatype="toolbar"]:not(.overflowedItem) > #zoom-reset-button > .toolbarbutton-text { > > + padding-top: 8px; > > Was this intentionally part of this patch? Yeah (see above). `:not(.overflowedItem)` also needs to be added, because wide widgets in the overflow-panel have the exact same appearance as in the menu-panel.

(In reply to Mike de Boer [:mikedeboer] from comment #9) > (In reply to :Gijs Kruitbosch from comment #6) > > Also, Mike, I could swear we had a bug about the buttons being mis-centered > > in the navbar, but I can't find it. > > I don't know of a bug for that, but this one will surely cover it. This > looks wrong and needs to be fixed. I'll take a look! This one? Bug 940255 (That's about Cut/Copy/Paste though and also about the spacing inside the menu, so I doubt it's a dupe)

(In reply to Peter Retzer (:pretzer) from comment #14) > This one? Bug 940255 > (That's about Cut/Copy/Paste though and also about the spacing inside the > menu, so I doubt it's a dupe) No, because that one is - like you said - all about menu panel. AFAIK we didn't find the bug Gijs thought he saw to date.

Comment on attachment 8342307 [details] [diff] [review] Patch 1: restore styling for buttons of wide widgets in toolbars Review of attachment 8342307 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/src/CustomizableWidgets.jsm @@ +811,5 @@ > } > }); > } > #endif > +#endif With this thing fixed, r=me!

Not sure if I did something wrong, but this bug doesn't seem fixed in latest nightly (29.0a1 (2013-12-11))

(In reply to Tim Nguyen [:ntim] from comment #19) > Not sure if I did something wrong, but this bug doesn't seem fixed in latest > nightly (29.0a1 (2013-12-11)) Sorry, well it is partially fixed, but the faint seperators still appear.

The faint separators as pictured here, http://screencast.com/t/A4JNR3TVOR, are intentional.

(In reply to Jared Wein [:jaws] from comment #21) > The faint separators as pictured here, http://screencast.com/t/A4JNR3TVOR, > are intentional. Why ? It doesn't seem consistent with the rest of the toolbar buttons. And don't also match shorlander specs.

The other toolbar buttons don't have them because they are not a logical group. Buttons that are logically grouped together have these splitters between them to show the connection between them.

(In reply to Jared Wein [:jaws] from comment #21) > The faint separators as pictured here, http://screencast.com/t/A4JNR3TVOR, > are intentional. Seems like people aren't talking about the same thing. To be clear, button groups are supposed to have separators, but the separators in that image don't look right.

(In reply to Stephen Horlander [:shorlander] from comment #24) > (In reply to Jared Wein [:jaws] from comment #21) > > The faint separators as pictured here, http://screencast.com/t/A4JNR3TVOR, > > are intentional. > > Seems like people aren't talking about the same thing. > > To be clear, button groups are supposed to have separators, but the > separators in that image don't look right. That's what I was trying to explain. The seperators should look like the one between the bookmark star and the bookmark menu icons.

Sorry, you are right. I filed bug 949151 to get this fixed.