Mike Conley (:mconley) (:⚙️) Assignee
	Description • 11 years ago

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.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 1 • 11 years ago

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

	u428464
	Comment 2 • 11 years ago

It should likely include the new overflow arrow.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 3 • 11 years ago

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.

	u428464
	Comment 4 • 11 years ago

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).

	Jared Wein [:jaws] (please needinfo? me)
	Comment 5 • 11 years ago

(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?

	u428464
	Comment 6 • 11 years ago

Filed bug 879160.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 7 • 11 years ago

Small icons that we're missing for (at least) Windows:

New Private Browsing Window
Save Page
Preferences
Add-ons
Open File
Developer Tools
Search

	u428464
	Comment 8 • 11 years ago

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

	Stephen Horlander [:shorlander]
	Comment 9 • 11 years ago

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.

	:Gijs (he/him)
	Comment 10 • 11 years ago

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

	:Gijs (he/him)
	Comment 11 • 11 years ago

(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. :-)

	Stephen Horlander [:shorlander]
	Comment 12 • 11 years ago

(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 :)

	Stephen Horlander [:shorlander]
	Comment 13 • 11 years ago

Changed layout of the OS X Toolbar*.png sheet

	u428464
	Comment 14 • 11 years ago

(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.

	Stephen Horlander [:shorlander]
	Comment 15 • 11 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 16 • 11 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 17 • 11 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 18 • 11 years ago

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?

	:Gijs (he/him)
	Comment 19 • 11 years ago

(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.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 21 • 11 years ago

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.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 22 • 11 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 24 • 11 years ago

Starting on Windows now...

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 25 • 11 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 26 • 11 years ago

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).

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 29 • 11 years ago

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.

	:Gijs (he/him)
	Comment 30 • 11 years ago

(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'.

	Dão Gottwald [:dao]
	Comment 31 • 11 years ago

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.

	:Gijs (he/him)
	Comment 32 • 11 years ago

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. :-)

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 33 • 11 years ago

Checkpointing work before switching to Windows again.

	Stephen Horlander [:shorlander]
	Comment 34 • 11 years ago

Missed the chevron in Toolbar-lunaBlue-lunaOlive.png

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 35 • 11 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 37 • 11 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 38 • 11 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 39 • 11 years ago

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.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 40 • 11 years ago

Just a note to self to test with rtl.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 43 • 11 years ago

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...

	Dão Gottwald [:dao]
	Comment 44 • 11 years ago

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.

	Justin Dolske [:Dolske]
	Comment 45 • 11 years ago

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

	:Gijs (he/him)
	Comment 46 • 11 years ago

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.

	Dão Gottwald [:dao]
	Comment 47 • 11 years ago

(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.

	:Gijs (he/him)
	Comment 49 • 11 years ago

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! :-)

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 50 • 11 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 51 • 11 years ago

Checkpointing work, addressing Gijs' comments.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 53 • 11 years ago

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. :)

	Dão Gottwald [:dao]
	Comment 54 • 11 years ago

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.

	:Gijs (he/him)
	Comment 55 • 11 years ago

(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.

	Dão Gottwald [:dao]
	Comment 56 • 11 years ago

(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.

	:Gijs (he/him)
	Comment 57 • 11 years ago

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)

	Dão Gottwald [:dao]
	Comment 58 • 11 years ago

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.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 59 • 11 years ago

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.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 60 • 11 years ago

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?

	Dão Gottwald [:dao]
	Comment 61 • 11 years ago

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.

	Dão Gottwald [:dao]
	Comment 62 • 11 years ago

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.

	Dão Gottwald [:dao]
	Comment 63 • 11 years ago

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.

	Dão Gottwald [:dao]
	Comment 64 • 11 years ago

(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...

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 65 • 11 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 66 • 11 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 67 • 11 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 68 • 11 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 69 • 11 years ago

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?

	Dão Gottwald [:dao]
	Comment 70 • 11 years ago

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"?

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 71 • 11 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 72 • 11 years ago

(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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 73 • 11 years ago

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

	u428464
	Comment 74 • 11 years ago

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

	fx4waldi
	Comment 75 • 11 years ago

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

	:Gijs (he/him)
	Comment 76 • 11 years ago

(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.

	Tim Nguyen :ntim
	Comment 77 • 11 years ago

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

	Tim Nguyen :ntim
	Comment 78 • 11 years ago

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

	Tim Nguyen :ntim
	Comment 79 • 11 years ago

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);
-}

	Tim Nguyen :ntim
	Comment 80 • 11 years ago

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

	:Gijs (he/him)
	Comment 81 • 11 years ago

(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.

	:Gijs (he/him)
	Comment 82 • 11 years ago

(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.

	:Gijs (he/him)
	Comment 83 • 11 years ago

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