We need the small icons for the items in menuPanel-icons. The only items in menuPanel-small-icons is the cut, copy, paste and zoom icons. Tentatively setting this as a M6 goal.

shorlander wanted to know which icons exactly were not satisfied by the menuPanel-small-icons and Toolbar.png spritesheets.

It should likely include the new overflow arrow.

As well as the hamburger icon, of which we currently only have the aero-dark icon. We are missing the aero-inverted version, as well as versions for non-aero and other platforms.

Also note that some icons are available for the toolbar, but show the whole icons set when placed in the menu bar/bookmark bar/tab strip (e.g. History widget).

(In reply to Guillaume C. [:ge3k0s] from comment #4) > Also note that some icons are available for the toolbar, but show the whole > icons set when placed in the menu bar/bookmark bar/tab strip (e.g. History > widget). Can you please file bugs for those?

Filed bug 879160.

Small icons that we're missing for (at least) Windows: New Private Browsing Window Save Page Preferences Add-ons Open File Developer Tools Search

The panorama icon is also inconsistent (the colors aren't the same when in toolbar).

Updated Toolbar.png and menuPanel.png for Windows XP, Windows 7, Windows 8 and OSX. Linux is waiting on the SVG test. Also forgot the Panorama toolbar icons, will update those separately. Note: The icon in the top/left position is the placeholder icon.

For OSX, will we use the same images everywhere? In the previous incarnation, we had separate sprites for (Mountain) Lion.

(In reply to :Gijs Kruitbosch from comment #10) > For OSX, will we use the same images everywhere? In the previous > incarnation, we had separate sprites for (Mountain) Lion. To clarify "previous incarnation": our current OSX theme has a Toolbar-Lion.png (and an @2x version). In addition to this question... would it be possible to reorganize the OS X sprite sheet for the toolbar icons to have the fullscreen images vertically after each other? This'll make it simpler to use the same sprite sheets cross-platform; now the OS X icons after the fullscreen button are all "one off" compared to the other platforms. Now that the icon sizes themselves are the same it'd be really nice if we could use the same coordinates cross-platform. :-)

(In reply to :Gijs Kruitbosch from comment #11) > (In reply to :Gijs Kruitbosch from comment #10) > > For OSX, will we use the same images everywhere? In the previous > > incarnation, we had separate sprites for (Mountain) Lion. > > To clarify "previous incarnation": our current OSX theme has a > Toolbar-Lion.png (and an @2x version). I would like to move to just one style for OS X. Lion was released almost two years ago, I am ok with back-porting a little of that to Snow Leopard. > In addition to this question... would it be possible to reorganize the OS X > sprite sheet for the toolbar icons to have the fullscreen images vertically > after each other? This'll make it simpler to use the same sprite sheets > cross-platform; now the OS X icons after the fullscreen button are all "one > off" compared to the other platforms. Now that the icon sizes themselves are > the same it'd be really nice if we could use the same coordinates > cross-platform. :-) Yes I can change that :)

Changed layout of the OS X Toolbar*.png sheet

(In reply to Stephen Horlander [:shorlander] from comment #9) >Also forgot the Panorama toolbar icons, will update those separately. The overflow arrow is still missing too.

Added overflow icon, fixed the dimensions on one of the sprites.

Stephen's delivered the icons - taking over now to drive 'er home.

Gonna try to sneak these into the User Research Build, as we're going to need the share icon for bug 879982.

Hey Stephen, I just started tackling this, and I've already hit a snag. Maybe I'm missing something here, but it looks like the spritesheets for at least OSX and Windows 7 are off of their coordinate grids. For example, for OSX's Toolbar@2x.png, I would expect that -moz-image-region: rect(0, 40px, 20px, 20px); would give me the back arrow icon for the nav-bar's back-button. But with those coordinates, the icon is off-center, and a little bit of the next icon to the right leaks in as well. Can you confirm that these icons are properly aligned to their coordinate grids?

(In reply to Mike Conley (:mconley) from comment #18) > Hey Stephen, > > I just started tackling this, and I've already hit a snag. Maybe I'm missing > something here, but it looks like the spritesheets for at least OSX and > Windows 7 are off of their coordinate grids. > > For example, for OSX's Toolbar@2x.png, I would expect that > -moz-image-region: rect(0, 40px, 20px, 20px); would give me the back arrow > icon for the nav-bar's back-button. > > But with those coordinates, the icon is off-center, and a little bit of the > next icon to the right leaks in as well. > > Can you confirm that these icons are properly aligned to their coordinate > grids? I actually am about halfway with integrating this, but won't have time to continue working on it before Monday (British time). However, if you get to it before then, I can at least answer your question: AIUI, all platforms were switched to 18x18px (36x36 on retina) icons as part of this change. That aligns correctly, as far as I've been able to see.

Hey Dao, This bug might be relevant to your interests - there have been quite a few bugs filed to update various icons for Australis. This is where shorlander put the new icons, so this is where the work will be going.

Please see bug 882807 for the need of inverted icon modes as well as the addition of an arrow icon.

Starting on Windows now...

Windows XP and Windows 7 seem to be doing the right thing now...

Comment on attachment 764393 [details] [diff] [review] WIP Patch 4 So, I'm doing a hack here to get the Windows 8 assets doing an override, and I'm not too pleased about it. I think we might have to wait for bug 810399 to land before we can get our Win 8 flavoured icons. Hey Gijs - this isn't exactly cleaned up yet, but can you give me a sanity-check? Is what I'm doing here sensical (minus the Windows 8 override, which I'll remove in the next iteration).

Ok, cleaned this up a bit more. Just need to check some things on Windows tomorrow, and then I'll be ready for a review.

(In reply to Mike Conley (:mconley) from comment #26) > Comment on attachment 764393 [details] [diff] [review] > WIP Patch 4 > > So, I'm doing a hack here to get the Windows 8 assets doing an override, and > I'm not too pleased about it. I think we might have to wait for bug 810399 > to land before we can get our Win 8 flavoured icons. FWIW, we always had an override for Lion, so I don't think a jar.mn override for Win8 is the end of the world, but it does look like bug 810399 is progressing. In the meantime, bug 810399 comment 5 has a hack that you may be able to use to get the right icons here. I'd actually personally favour getting that hooked up with a comment + followup bug depending on bug 810399 to use something 'nicer'.

The overrides are bogus, see bug 702558. We shouldn't make this a problem for more of our users, especially since we can expect Win 8 to get more market share than OS X Lion.

Comment on attachment 764550 [details] [diff] [review] Patch v1.2 Review of attachment 764550 [details] [diff] [review]: ----------------------------------------------------------------- Woo, awesome! Getting closer. :-) Sad thing: we need a solution for linux menupanel things again because we removed the shared styling they now have from the panelUIOverlay.inc.css . That and Win8 are my biggest concerns at this point. Note that I haven't checked all your -moz-image-region offsets, I just assume you've verified them visually. :-) ::: browser/themes/osx/browser.css @@ -415,5 @@ > margin: 0; > } > > .toolbarbutton-1, > #restore-button { I believe you'll need to fix the restore-button/fullscreen-button[checked] case still, as it doesn't seem to be there in the toolbarbutton.inc file? Especially as these have different styling on OS X (4 icons, IIRC). Same for retina styling, sadly. @@ +709,5 @@ > } > > :-moz-any(@primaryToolbarButtons@):not(#tabview-button) > .toolbarbutton-icon, > :-moz-any(@primaryToolbarButtons@):not(#tabview-button) > .toolbarbutton-menubutton-button > .toolbarbutton-icon { > width: 20px; This is wrong now, and should be 18px, I believe. I also think we should set a height everywhere we set a width (see bug 882910). I'm also wondering if we should just set this everywhere, not just for OS X @2x. We have 18px icons everywhere now, so we should be OK. We can then set h+w 32px for the panel/palette icons in some general place, too. If you don't want to put this in a generic place, you'll at least need to override this style for the menupanel/palette icons on OS X retina so they don't get squashed to 18px, right? ::: browser/themes/osx/customizableui/panelUIOverlay.css @@ +40,3 @@ > #PanelUI-contents #zoom-in-button .toolbarbutton-text, > #PanelUI-contents #zoom-out-button .toolbarbutton-text, > #PanelUI-contents #zoom-reset-button .toolbarbutton-icon { If you're touching these anyway: we don't display the reset button at all in the toolbar, and we don't display toolbarbutton text in toolbars either, so I *think* these can just use a single child selector on the buttons without caring about their area. ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css @@ +118,5 @@ > } > > #PanelUI-contents[type="grid"] toolbarbutton, > +toolbarpaletteitem[place="panel"] > toolbarbutton, > +toolbarpaletteitem[place="palette"] > toolbarbutton, If we're touching this anyway: that top selector, is it ever not a [type="grid"]? And, can we not use toolbarbutton[customization-area="panel"] ? (we might not be able to for specificity reasons... also, I'm totally OK with dealing with this and some of my other "if you're touching this anyway" in a followup bug, as this is a big change as it is, and what you're doing isn't wrong as much as "could we be better here". ::: browser/themes/shared/toolbarbuttons.inc @@ +35,5 @@ > + -moz-image-region: rect(0, 162px, 18px, 144px); > +} > + > +/* XXXmconley: set coordinates on this **/ > +#bookmarks-menu-button.bookmark-item[starred] { If memory serves me, there's a separate sprite for this already in the codebase? I may be wrong! @@ +294,5 @@ > +#edit-controls[customizableui-areatype="menu-panel"] > #cut-button, > +#edit-controls[customizableui-areatype="menu-panel"] > #copy-button, > +#edit-controls[customizableui-areatype="menu-panel"] > #paste-button, > +#zoom-controls[customizableui-areatype="menu-panel"] > #zoom-out-button, > +#zoom-controls[customizableui-areatype="menu-panel"] > #zoom-in-button { Like the buttons above, I believe these also need to have this style in the customization palette. ::: browser/themes/windows/browser.css @@ +7,5 @@ > @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); > @namespace html url("http://www.w3.org/1999/xhtml"); > > %include ../shared/browser.inc > +%include ../shared/toolbarbuttons.inc Windows 8 will also need the flat include, but that may be more work to get completely right because we'll need to flatten the buttons there, too. Followup bug might be easier, then? @@ +332,5 @@ > > /* ::::: primary toolbar buttons ::::: */ > > +%ifndef WINDOWS_AERO > +@media (-moz-windows-theme: luna-silver) { Is this ever true on aero? AKA do we really need this %ifndef? @@ +1478,5 @@ > #main-window[tabsintitlebar]:not([inFullscreen]) #TabsToolbar > #tabview-button { > list-style-image: url(chrome://browser/skin/tabview/tabview-inverted.png); > } > > + /* XXXgijs Check if this is OK */ I am fairly sure this is OK, but it doesn't hurt to check and/or get rid of the comment. :-)

Checkpointing work before switching to Windows again.

Missed the chevron in Toolbar-lunaBlue-lunaOlive.png

Some more tweaks - got the new assets in there too.

These bugs didn't make it into the UR Build that went out in bug 879846. Clearing the [User Research Build+] flag.

Removing the items from M7 that do not block landing on m-c.

So Gijs is right - this totally busts icons on Linux GTK. We're going to need the proper spritesheets. Not sure if we've made a decision on whether or not to use SVG - I'll ping MattN and report back.

Just a note to self to test with rtl.

Comment on attachment 765967 [details] [diff] [review] Patch v1.7 So I think I'm a bit closer here. I think I want Dao's OK on this, since we're changing a bunch of stuff in theme-land. One thing to note is the default -moz-image-region's for .toolbarbutton-1. UX asked for something like this so that toolbarbuttons that haven't defined an icon or region but have toolbarbutton-1 show just the first icon, and not the entire spritesheet. The drawback to this is that it means that add-ons that provide toolbarbutton-1 buttons will have the wrong dimensions (or will be cropped) unless the add-ons override the -moz-image-region property. Mike de Boer suggested we might take this opportunity to introduce a toolbarbutton-2 class that provides the list-style-image and -moz-image-region defaults, and just have toolbarbutton-1 provide what it currently does but *without* those properties. Not sure what the best way forward here is, or if the add-on headache is worth it. I do know, however, that showing the whole spritesheet is highly undesirable...

Comment on attachment 765967 [details] [diff] [review] Patch v1.7 You can use @primaryToolbarButtons@ to set Toolbar.png only for buttons that we actually provide icons for.

P1 since it sounds like we'd be flat-out missing icons, but P4 or P5 for purely visual updates.

Comment on attachment 765967 [details] [diff] [review] Patch v1.7 Review of attachment 765967 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Dão Gottwald [:dao] from comment #44) > Comment on attachment 765967 [details] [diff] [review] > Patch v1.7 > > You can use @primaryToolbarButtons@ to set Toolbar.png only for buttons that > we actually provide icons for. Right, but that'd defeat the point of having a better 'fallback image' (than the spritesheet or nothing) for buttons that don't have a defined image... Anyway, this LGTM, but for the concern about add-on buttons... I defer to Dao on that one. I personally like having an "oops" icon for add-ons, but I'm not sure how to have it not interfere with add-ons that do provide an icon, and if it does more harm than good it can just be an "oops" icon for ourselves if/when we add more buttons... ::: browser/themes/shared/menupanel.inc @@ +1,4 @@ > +/* Menu panel and palette styles */ > + > +:-moz-any(@primaryToolbarButtons@)[customizableui-areatype="menu-panel"], > +:-moz-any(@primaryToolbarButtons@)[customizableui-areatype="menu-panel"]:-moz-lwtheme-brighttext, Is this just here to override the :-moz-lwtheme-brighttext selector for @primaryToolbarButtons@ in general that'd have it use Toolbar-inverted.png? ::: browser/themes/shared/toolbarbuttons-flat.inc @@ +100,5 @@ > +} > + > +#fullscreen-button[checked="true"]:hover:active:not([disabled="true"]) { > + -moz-image-region: rect(54px, 432px, 72px, 414px); > +} This and the non-hover case should probably live in the OSX stylesheet and not here, even if OS X is currently the only consumer, as win8 will be including this too and doesn't have these icons.

(In reply to :Gijs Kruitbosch from comment #46) > (In reply to Dão Gottwald [:dao] from comment #44) > > Comment on attachment 765967 [details] [diff] [review] > > Patch v1.7 > > > > You can use @primaryToolbarButtons@ to set Toolbar.png only for buttons that > > we actually provide icons for. > > Right, but that'd defeat the point of having a better 'fallback image' (than > the spritesheet or nothing) for buttons that don't have a defined image... Showing no icon seems sensible for buttons that didn't get an image assigned. Showing some random icon seems rather unexpected/confusing and not really helpful.

Stephen, could I still get you to update the panel button sprites with the customize [+] and (?) graphics? I need them for bug 877684, and for performance it'd be best if they were in some sprite sheet that was loaded (anyway). Pretty please? Now that I'm in Toronto, I can buy you a drink! :-)

Going with Dao's suggestion, and only applying the default list-style-image and -moz-image-region to the primaryToolbarButtons.

Checkpointing work, addressing Gijs' comments.

Comment on attachment 767947 [details] [diff] [review] Patch v1.10 Alright Gijs, here's a reviewable patch. Some provisos: 1) Linux is still a bit shaky, since it imports toolbarbutton.inc, but we don't actually have any icons for it yet (since we're not sure whether or not to use SVG). So there's an interesting mix of the stock GTK widgets, Toolbar, and Toolbar-menuPanel being used here. That's a known issue, and once we sort out our SVG decision for the Linux GTK icons, we'll know what to do. I'd like to deal with that then. 2) On OSX, I've noticed that the active:hover states on the menu panel buttons cause the icons to not appear. Again, I'd like to deal with that in a follow-up. 3) Some new assets are likely to come in from Stephen (the ? and + icons were asked for, for example) - those aren't going to be dealt with here, and will be put into a follow-up bug. I think this is sane. Hopefully Dao agrees. :) But I'm targeting you for review, Gijs, because you know the code I'm dealing with, started the patch, and don't have a mountain as a review queue. :)

Comment on attachment 767947 [details] [diff] [review] Patch v1.10 >--- /dev/null >+++ b/browser/themes/shared/menupanel.inc >@@ -0,0 +1,176 @@ >+/* Menu panel and palette styles */ >+ >+:-moz-any(@primaryToolbarButtons@)[customizableui-areatype="menu-panel"], >+:-moz-any(@primaryToolbarButtons@)[customizableui-areatype="menu-panel"]:-moz-lwtheme-brighttext, >+:-moz-any(@primaryToolbarButtons@)[customizableui-areatype="menu-panel"]:hover:active, >+toolbarpaletteitem[place="palette"] > :-moz-any(@primaryToolbarButtons@) { >+ list-style-image: url(chrome://browser/skin/menuPanel.png); >+ -moz-image-region: rect(0px, 32px, 32px, 0px); >+} It looks like the :-moz-lwtheme-brighttext and :hover:active lines are redundant. >+#reload-button[customizableui-areatype="menu-panel"], >+#reload-button[customizableui-areatype="menu-panel"]:hover:active, >+toolbarpaletteitem[place="palette"] > #reload-button { >+ -moz-image-region: rect(0px, 64px, 32px, 32px); >+} same for :hover:active here... >--- /dev/null >+++ b/browser/themes/shared/toolbarbuttons-flat.inc Why is this shared? It's only used on OS X.

(In reply to Dão Gottwald [:dao] from comment #54) > Comment on attachment 767947 [details] [diff] [review] > Patch v1.10 > > >--- /dev/null > >+++ b/browser/themes/shared/menupanel.inc > >@@ -0,0 +1,176 @@ > >+/* Menu panel and palette styles */ > >+ > >+:-moz-any(@primaryToolbarButtons@)[customizableui-areatype="menu-panel"], > >+:-moz-any(@primaryToolbarButtons@)[customizableui-areatype="menu-panel"]:-moz-lwtheme-brighttext, > >+:-moz-any(@primaryToolbarButtons@)[customizableui-areatype="menu-panel"]:hover:active, > >+toolbarpaletteitem[place="palette"] > :-moz-any(@primaryToolbarButtons@) { > >+ list-style-image: url(chrome://browser/skin/menuPanel.png); > >+ -moz-image-region: rect(0px, 32px, 32px, 0px); > >+} > > It looks like the :-moz-lwtheme-brighttext and :hover:active lines are > redundant. I suspect these are necessary to get a higher specificity than the analogous selectors without a customizableui area, which are used for the toolbars. > >+#reload-button[customizableui-areatype="menu-panel"], > >+#reload-button[customizableui-areatype="menu-panel"]:hover:active, > >+toolbarpaletteitem[place="palette"] > #reload-button { > >+ -moz-image-region: rect(0px, 64px, 32px, 32px); > >+} > > same for :hover:active here... Here I think I agree with Dao, but I may be missing something. > > >--- /dev/null > >+++ b/browser/themes/shared/toolbarbuttons-flat.inc > > Why is this shared? It's only used on OS X. It'll be used for Win 8, but that's going to have to be a followup bug as well.

(In reply to :Gijs Kruitbosch from comment #55) > > >+:-moz-any(@primaryToolbarButtons@)[customizableui-areatype="menu-panel"], > > >+:-moz-any(@primaryToolbarButtons@)[customizableui-areatype="menu-panel"]:-moz-lwtheme-brighttext, > > >+:-moz-any(@primaryToolbarButtons@)[customizableui-areatype="menu-panel"]:hover:active, > > >+toolbarpaletteitem[place="palette"] > :-moz-any(@primaryToolbarButtons@) { > > >+ list-style-image: url(chrome://browser/skin/menuPanel.png); > > >+ -moz-image-region: rect(0px, 32px, 32px, 0px); > > >+} > > > > It looks like the :-moz-lwtheme-brighttext and :hover:active lines are > > redundant. > > I suspect these are necessary to get a higher specificity than the analogous > selectors without a customizableui area, which are used for the toolbars. What selectors in particular? From toolbarbuttons.inc: +:-moz-any(@primaryToolbarButtons@) { + list-style-image: url("chrome://browser/skin/Toolbar.png"); +} Shouldn't have a higher specificity. +:-moz-any(@primaryToolbarButtons@)[customizableui-areatype="toolbar"]:-moz-lwtheme-brighttext { + list-style-image: url(chrome://browser/skin/Toolbar-inverted.png); +} Has [customizableui-areatype="toolbar"]. > > >--- /dev/null > > >+++ b/browser/themes/shared/toolbarbuttons-flat.inc > > > > Why is this shared? It's only used on OS X. > > It'll be used for Win 8, but that's going to have to be a followup bug as > well. Does this mean that there would be no hover state on Windows 8? This doesn't seem like it would satisfy platform conventions.

Comment on attachment 767947 [details] [diff] [review] Patch v1.10 Review of attachment 767947 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me. r=me, but for brownie points, I *believe*, but haven't checked, that your OS X :hover:active issue is because the specificity of the menupanel image wins, as well as the :hover:active:not([disabled]) for the toolbar (!) coordinates on OS X. Those coordinates don't match, hence emptiness. For any of: #preferences-button[customizableui-areatype="menu-panel"] also add #preferences-button[customizableui-areatype="menu-panel"]:hover:active:not([disabled]) (or whatever you're doing for the toolbar case) and it should be OK. (related to that, I think that *might* mean the :hover:active Dao pointed out is required, too... but I'm not sure)

Comment on attachment 767947 [details] [diff] [review] Patch v1.10 The selector craziness needs to be fixed. I suppose the only problem is with the hi-dpi code for OS X. That should be dealt with by reordering stuff rather than adding redundant class and attribute selectors.

Ok, these make selectors on Retina OSX a little more sane, since we're wrapping the imports inside a !Retina rule. We've also made the toolbar button rules a little more specific using the customizableui-areatype attribute. This has greatly reduced the number of selectors being overridden.

Ok, this fixes the active:hover state OSX bug (thanks Gijs!), and I believe simplifies a lot of the selector stuff we were doing here across each platform. Instead of overriding and overriding and overriding, I've tried to make the toolbarbutton region rules more specific based on the customizableui-areatype. A quick glance at DOM Inspector, and we've wiped out a ton of useless rules. \o/ Any feedback on this Dao?

Comment on attachment 768008 [details] [diff] [review] Patch v1.12 >+++ b/browser/themes/shared/menupanel.inc >+#stop-button[customizableui-areatype="menu-panel"], >+#stop-button[customizableui-areatype="menu-panel"]:hover:active, >+toolbarpaletteitem[place="palette"] > #stop-button { >+ -moz-image-region: rect(0px, 96px, 32px, 64px); >+} >+ >+#home-button[customizableui-areatype="menu-panel"], >+#home-button[customizableui-areatype="menu-panel"]:hover:active, >+toolbarpaletteitem[place="palette"] > #home-button { >+ -moz-image-region: rect(0px, 128px, 32px, 96px); >+} >+ >+#bookmarks-menu-button[customizableui-areatype="menu-panel"], >+#bookmarks-menu-button[customizableui-areatype="menu-panel"]:hover:active, >+toolbarpaletteitem[place="palette"] > #bookmarks-menu-button { >+ -moz-image-region: rect(0px, 160px, 32px, 128px); >+} ... There's more redundancy here with :hover:active. >+++ b/browser/themes/shared/toolbarbuttons.inc >+#back-button:-moz-local-dir(rtl) > .toolbarbutton-icon, >+#forward-button:-moz-local-dir(rtl), >+#forward-button:-moz-local-dir(rtl) > .toolbarbutton-test { >+ transform: scaleX(-1); >+} typo 1: -moz-local-dir typo 2: toolbarbutton-test (this line actually shouldn't be needed anymore) >+++ b/browser/themes/windows/browser.css >+%include ../shared/toolbarbuttons.inc >+%include ../shared/menupanel.inc >+ >+%ifndef WINDOWS_AERO >+@media (-moz-windows-theme: luna-silver) { >+ :-moz-any(@primaryToolbarButtons@) { >+ list-style-image: url("chrome://browser/skin/Toolbar-lunaSilver.png"); >+ } > } >+%endif It looks like this should work, i.e. Toolbar-lunaSilver.png won't override Toolbar-inverted.png or menuPanel.png. Did you verify that this works as expected? > #main-window:not([customizing]) .toolbarbutton-1[disabled=true] > .toolbarbutton-icon, > #main-window:not([customizing]) .toolbarbutton-1[disabled=true] > .toolbarbutton-menu-dropmarker, > #main-window:not([customizing]) .toolbarbutton-1[disabled=true] > .toolbarbutton-menubutton-dropmarker, > #main-window:not([customizing]) .toolbarbutton-1[disabled=true] > .toolbarbutton-menubutton-button > .toolbarbutton-icon, >-#main-window:not([customizing]) .toolbarbutton-1 > .toolbarbutton-menubutton-button[disabled] > .toolbarbutton-icon, >+#main-window:not([customizing]) .toolbarbutton-1 > .toolbarbutton-menubutton-button[disabled] > .toolbarbutton-icon { > opacity: .4; > } Bug 885402 took care of this. >+++ b/browser/themes/windows/jar.mn >@@ -39,16 +38,18 @@ browser.jar: > skin/classic/browser/identity-icons-https.png > skin/classic/browser/identity-icons-https-ev.png > skin/classic/browser/identity-icons-https-mixed-active.png > skin/classic/browser/keyhole-forward-mask.svg > skin/classic/browser/KUI-background.png > skin/classic/browser/livemark-folder.png > skin/classic/browser/menu-back.png > skin/classic/browser/menu-forward.png >+ skin/classic/browser/menuPanel.png (menuPanel.png) >+ skin/classic/browser/menuPanel-small.png (menuPanel-small.png) The file names in parentheses aren't needed. >@@ -63,18 +64,19 @@ browser.jar: > skin/classic/browser/privatebrowsing-light.png > skin/classic/browser/privatebrowsing-dark.png > skin/classic/browser/reload-stop-go.png > skin/classic/browser/searchbar.css > skin/classic/browser/searchbar-dropdown-arrow.png > skin/classic/browser/Secure24.png > skin/classic/browser/setDesktopBackground.css > skin/classic/browser/slowStartup-16.png >- skin/classic/browser/Toolbar.png >- skin/classic/browser/Toolbar-inverted.png >+ skin/classic/browser/Toolbar.png (Toolbar-lunaBlue-lunaOlive.png) >+ skin/classic/browser/Toolbar-inverted.png (Toolbar-inverted.png) >+ skin/classic/browser/Toolbar-lunaSilver.png (Toolbar-lunaSilver.png) So Toolbar-lunaBlue-lunaOlive.png is packaged as Toolbar.png and then used for more than just luna blue/olive? The original file name seems confusing then.

Also please unfold toolbarbuttons-flat.inc into browser.css. It can be moved easily when the time has come, just like the hi-dpi stuff should be shared when such icons exist for Windows.

Comment on attachment 768008 [details] [diff] [review] Patch v1.12 >+ skin/classic/aero/browser/menuPanel-win8.png (win8/menuPanel.png) >+ skin/classic/aero/browser/menuPanel-small-win8.png (win8/menuPanel-small.png) >+ skin/classic/aero/browser/Toolbar-inverted-win8.png (win8/Toolbar-inverted.png) >+ skin/classic/aero/browser/Toolbar-win8.png (win8/Toolbar.png) Apparently these aren't used yet. You should only package them when they're used.

(In reply to Dão Gottwald [:dao] from comment #61) > Comment on attachment 768008 [details] [diff] [review] > Patch v1.12 > > >+++ b/browser/themes/shared/menupanel.inc > > >+#stop-button[customizableui-areatype="menu-panel"], > >+#stop-button[customizableui-areatype="menu-panel"]:hover:active, > >+toolbarpaletteitem[place="palette"] > #stop-button { > >+ -moz-image-region: rect(0px, 96px, 32px, 64px); > >+} > >+ > >+#home-button[customizableui-areatype="menu-panel"], > >+#home-button[customizableui-areatype="menu-panel"]:hover:active, > >+toolbarpaletteitem[place="palette"] > #home-button { > >+ -moz-image-region: rect(0px, 128px, 32px, 96px); > >+} > >+ > >+#bookmarks-menu-button[customizableui-areatype="menu-panel"], > >+#bookmarks-menu-button[customizableui-areatype="menu-panel"]:hover:active, > >+toolbarpaletteitem[place="palette"] > #bookmarks-menu-button { > >+ -moz-image-region: rect(0px, 160px, 32px, 128px); > >+} > ... > > There's more redundancy here with :hover:active. To get rid of this I guess you need to add more [customizableui-areatype="toolbar"] on OS X...

Thanks Dao - cleaned this up a bit more. Going to test on Windows and then re-request review.

Fixes for Windows, switching to lo-dpi Mac and Linux now...

Fixing lo-dpi selector for OSX. Testing on Linux now...

Icons look a tad funky on Linux, but that's because we're waiting for SVG.

Comment on attachment 768368 [details] [diff] [review] Patch v1.15 Review of attachment 768368 [details] [diff] [review]: ----------------------------------------------------------------- Feeling even better about this - it's getting more and more simplified. What do you think of this, Dao?

Comment on attachment 768368 [details] [diff] [review] Patch v1.15 >--- a/browser/themes/osx/browser.css >+++ b/browser/themes/osx/browser.css >+@media not all and (min-resolution: 2dppx) { . . . > @media (min-resolution: 2dppx) { Can these two blocks be adjacent or do they need to be this far apart from each other? > skin/classic/aero/browser/livemark-folder.png (livemark-folder-aero.png) > skin/classic/aero/browser/menu-back.png (menu-back-aero.png) > skin/classic/aero/browser/menu-forward.png (menu-forward-aero.png) >+ skin/classic/aero/browser/menuPanel.png (win7/menuPanel.png) >+ skin/classic/aero/browser/menuPanel-small.png (win7/menuPanel-small.png) >+ skin/classic/aero/browser/Toolbar.png (win7/Toolbar.png) >+ skin/classic/aero/browser/Toolbar-inverted.png (win7/Toolbar-inverted.png) Seems like these icons shouldn't be in a separate folder and have the -aero suffix instead. This would be consistent with many other icons and more accurate since they're also used on Vista. "customizableui-areatype" is pretty cumbersome for something being used that often. Can we rename it to something brief? Just "area"?

Fixed! Going to test this one last time on Windows, and then push. Thanks for the review!

(In reply to Dão Gottwald [:dao] from comment #70) > Comment on attachment 768368 [details] [diff] [review] > Patch v1.15 > > >--- a/browser/themes/osx/browser.css > >+++ b/browser/themes/osx/browser.css > > >+@media not all and (min-resolution: 2dppx) { > . > . > . > > @media (min-resolution: 2dppx) { > > Can these two blocks be adjacent or do they need to be this far apart from > each other? Yep, done. > > > skin/classic/aero/browser/livemark-folder.png (livemark-folder-aero.png) > > skin/classic/aero/browser/menu-back.png (menu-back-aero.png) > > skin/classic/aero/browser/menu-forward.png (menu-forward-aero.png) > >+ skin/classic/aero/browser/menuPanel.png (win7/menuPanel.png) > >+ skin/classic/aero/browser/menuPanel-small.png (win7/menuPanel-small.png) > > >+ skin/classic/aero/browser/Toolbar.png (win7/Toolbar.png) > >+ skin/classic/aero/browser/Toolbar-inverted.png (win7/Toolbar-inverted.png) > > Seems like these icons shouldn't be in a separate folder and have the -aero > suffix instead. This would be consistent with many other icons and more > accurate since they're also used on Vista. Done. > > "customizableui-areatype" is pretty cumbersome for something being used that > often. Can we rename it to something brief? Just "area"? Filed follow-up bug 888115. Landed in UX as https://hg.mozilla.org/projects/ux/rev/1010d94e216b

And a follow-up patch to include the missing menuPanel.png and menuPanel-small.png for Linux: https://hg.mozilla.org/projects/ux/rev/a96796c4d35f

Also note that as mentioned before "Panorama" has no high res icon anymore.

In the last UX on windows 7, toolbar buttons use icons from windows xp

(In reply to fx4waldi from comment #75) > In the last UX on windows 7, toolbar buttons use icons from windows xp I filed bug 888766 for this issue.

Using the new media query : -moz-os-version:win8, you should be able to implement the windows 8 assets too.

Also, the download button should also share the toolbar.png assets

Also, the #social-share-button should also share the sprite sheet. Here's how it can be fixed (remove those lines of code that are overriding the sprite sheet): -#social-share-button { - list-style-image: url(chrome://browser/skin/social/share-button.png); - -moz-image-region: rect(0, 16px, 16px, 0); -}

Sorry forgot this line of code for the social share button : +#social-share-button { +-moz-image-region: rect(0, 307px, 18px, 289px) +}

(In reply to ntim007 from comment #78) > Also, the download button should also share the toolbar.png assets (In reply to ntim007 from comment #79) > Also, the #social-share-button should also share the sprite sheet. Which platform are you talking about? Can you please file a new bug regarding these issues with a clear indication of which platform this is about, and mark it as blocking this bug? Thank you! (In reply to ntim007 from comment #77) > Using the new media query : -moz-os-version:win8, you should be able to > implement the windows 8 assets too. This is being tracked in bug 859751.

(In reply to ntim007 from comment #80) > Sorry forgot this line of code for the social share button : > +#social-share-button { > +-moz-image-region: rect(0, 307px, 18px, 289px) > +} Filed bug 898837 about this. (In reply to ntim007 from comment #78) > Also, the download button should also share the toolbar.png assets I think it mostly does, but I filed bug 898839 for the remainder.

https://hg.mozilla.org/mozilla-central/rev/1010d94e216b https://hg.mozilla.org/mozilla-central/rev/a96796c4d35f