We don't use the light text colour so we shouldn't invert either IMO.

I had to add [cui-areatype="toolbar"] too since otherwise it would be more specific than the menu panel list-style-image rule so the menu panel would have the toolbar icons. This seems to work on OS X and it seems sane to limit "Toolbar.png" to cui-areatype="toolbar"] (it has toolbar right in the filename). The only other supported type in the menu panel so I don't see any other places we'd need this to work. I confirmed customization mode still works fine (we disable LWT there anyways).

Comment on attachment 8391983 [details] [diff] [review] v.1 Don't invert icons for -moz-lwtheme-brighttext in the overflow panel Review of attachment 8391983 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/shared/toolbarbuttons.inc.css @@ +2,5 @@ > #bookmarks-menu-button > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon { > list-style-image: url("chrome://browser/skin/Toolbar.png"); > } > > +:-moz-any(@primaryToolbarButtons@)[cui-areatype="toolbar"]:not([overflowedItem=true]):-moz-lwtheme-brighttext, This won't work because we don't set [cui-areatype] on nested buttons like the cut-button (which are part of @primaryToolbarButtons@).

(In reply to :Gijs Kruitbosch from comment #2) > > +:-moz-any(@primaryToolbarButtons@)[cui-areatype="toolbar"]:not([overflowedItem=true]):-moz-lwtheme-brighttext, > > This won't work because we don't set [cui-areatype] on nested buttons like > the cut-button (which are part of @primaryToolbarButtons@). Thanks Gijs, I now fixed the code to propagate [cui-areatype] to the zoom and edit buttons. I didn't both propagating [overflowedItem=true] since the selectors already handled that and I want to reduce risk. This also removes duplication and inconsistency in the button classes.

Comment on attachment 8391993 [details] [diff] [review] v.2 Don't invert icons for -moz-lwtheme-brighttext in overflow (combined button support) Review of attachment 8391993 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/shared/toolbarbuttons.inc.css @@ +2,5 @@ > #bookmarks-menu-button > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon { > list-style-image: url("chrome://browser/skin/Toolbar.png"); > } > > +:-moz-any(@primaryToolbarButtons@)[cui-areatype="toolbar"]:not([overflowedItem=true]):-moz-lwtheme-brighttext, This doesn't work for the back, forward, overflow, or menu buttons. Perhaps having a define for buttons which are CustomizableWidgets would be useful so we know which buttons should have the cui attributes.

This fixes the four buttons that didn't work in v.2. I went through the list of primaryToolbarButtons and I think they should work with this approach. An alternative approach would be to add !important to https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/menupanel.inc.css?rev=02be46ac2b55&mark=6#3 and then I think I can drop the [cui-areatype=…] addition to the selector.

Comment on attachment 8392042 [details] [diff] [review] v.3 :not([cui-areatype="menu-panel"]) Review of attachment 8392042 [details] [diff] [review]: ----------------------------------------------------------------- Nice to to tackle this properly now! ::: browser/components/customizableui/src/CustomizableWidgets.jsm @@ -430,5 @@ > const kPanelId = "PanelUI-popup"; > let areaType = CustomizableUI.getAreaType(this.currentArea); > let inPanel = areaType == CustomizableUI.TYPE_MENU_PANEL; > let inToolbar = areaType == CustomizableUI.TYPE_TOOLBAR; > let closeMenu = inPanel ? "none" : null; This is now set in `updateCombinedWidgetStyle` too, so you can remove this attribute from the code here. @@ +641,5 @@ > setAttributes(btnNode, aButton); > node.appendChild(btnNode); > }); > > + updateCombinedWidgetStyle(node, this.currentArea, true); Since we don't modify the `closemenu` property for this button, I think you'd better pass `false` here. ::: browser/themes/osx/browser.css @@ +748,5 @@ > list-style-image: url("chrome://browser/skin/Toolbar@2x.png"); > } > > + :-moz-any(@primaryToolbarButtons@):not([cui-areatype="menu-panel"]):not([overflowedItem=true]):-moz-lwtheme-brighttext, > + #bookmarks-menu-button[cui-areatype="toolbar"]:not([overflowedItem=true]):-moz-lwtheme-brighttext > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon { This can now also be ```css :-moz-any(@primaryToolbarButtons@):not(@inAnyPanel@):-moz-lwtheme-brighttext, #bookmarks-menu-button:not(@inAnyPanel@):-moz-lwtheme-brighttext > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon { ``` ...which looks nice & clean, don't you think? ::: browser/themes/shared/toolbarbuttons.inc.css @@ +3,5 @@ > list-style-image: url("chrome://browser/skin/Toolbar.png"); > } > > +:-moz-any(@primaryToolbarButtons@):not([cui-areatype="menu-panel"]):not([overflowedItem=true]):-moz-lwtheme-brighttext, > +#bookmarks-menu-button:not([overflowedItem=true]):-moz-lwtheme-brighttext > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon { This can now also be ```css :-moz-any(@primaryToolbarButtons@):not(@inAnyPanel@):-moz-lwtheme-brighttext, #bookmarks-menu-button:not(@inAnyPanel@):-moz-lwtheme-brighttext > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon { ``` ...which also fixes the inconsistency I see with the bookmarks-menu-button rule in osx/browser.css.

When using @inAnyPanel@, you also need to add `cui-areatype="toolbar"` to the 'PanelUI-menu-button' and 'nav-bar-overflow-button' buttons in browser.xul.

This isn't just about toolbarbutton-combined. toolbarbutton-1 buttons can exist freely in toolbaritems with arbitrary structure. See also bug 975794.

(In reply to Dão Gottwald [:dao] from comment #8) > This isn't just about toolbarbutton-combined. toolbarbutton-1 buttons can > exist freely in toolbaritems with arbitrary structure. See also bug 975794. This rule is just about the builtin Toolbar-inverted.png, however, so I don't think it's necessary to care about other things than what we ship - they'd have their own icons.

Oh, I forgot to mention you need to add `cui-areatype="toolbar"` to 'back-button' and 'forward-button' too.

(In reply to Mike de Boer [:mikedeboer] from comment #7) > When using @inAnyPanel@, you also need to add `cui-areatype="toolbar"` to > the 'PanelUI-menu-button' and 'nav-bar-overflow-button' buttons in > browser.xul. (In reply to Mike de Boer [:mikedeboer] from comment #10) > Oh, I forgot to mention you need to add `cui-areatype="toolbar"` to > 'back-button' and 'forward-button' too. I intentionally used :not([cui-areatype="menu-panel"]) rather than your version which expands to [cui-areatype="toolbar"] because of the issue with these four buttons. I didn't think we should be adding "cui-…" attributes to non-CUI widgets as it feels a bit dirty. I'm now thinking it was a mistake to have that attribute with the prefix because of the case of these buttons but it's obviously too late to change it now. I then saw that we're already setting @cui-areatype on non-removable items like #urlbar-container so I suppose it's not so bad to add more. #PanelUI-menu-button does feel a bit dirtier since it's not even in a customization target but handling it the same makes the CSS simpler.

Comment on attachment 8392719 [details] [diff] [review] v.4 :not(@inAnyPanel@) and more @cui-areatype Review of attachment 8392719 [details] [diff] [review]: ----------------------------------------------------------------- TBH, I actually kinda like consistency :) I agree with you that the 'cui-' prefix is not necessary anymore, and may even be confusing to some. Good stuff for a follow-up and even a good first bug (tm), I'd say. Signing off, this looks awesome. Thanks!

(In reply to Mike de Boer [:mikedeboer] from comment #12) > TBH, I actually kinda like consistency :) I agree with you that the 'cui-' > prefix is not necessary anymore, and may even be confusing to some. Good > stuff for a follow-up and even a good first bug (tm), I'd say. I would have to be done yesterday or basically never since we can't break addon-compat in beta.

Comment on attachment 8392719 [details] [diff] [review] v.4 :not(@inAnyPanel@) and more @cui-areatype [Approval Request Comment] Bug caused by (feature/regressing bug #): not sure - Australis overflow panel User impact if declined: Overflow panel icons will be inverted when they shouldn't Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): Low risk overflow panel styling change. Small chance that the attributes added for consistency will fix and/or break other things. String or IDL/UUID changes made by this patch: None

remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/09c64c10cdb6 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/0c853fd14db5

Is there a way to set the theme -moz-lwtheme-brighttext (in order to verify this bug is fixed)? Thank you

I've installed a light theme from addons.mozilla.org and I could reproduce the bug. Verified as fixed on Firefox 29 beta 2 (20140324101726), latest Aurora 30.0a2 (20140325004002) and latest Nightly 31.0a1 (20140325030201) on Win 7 and Ubuntu 64-bit.

(In reply to Petruta Rasa [QA] [:petruta] from comment #18) > Is there a way to set the theme -moz-lwtheme-brighttext (in order to verify > this bug is fixed)? Thank you It's easiest to just install a theme with a light text color (usually ones with dark background images). I'm guessing you figured that out.