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

Since bug 934032, at least on Windows 7, the toolbar icons seem lighter.

Looking at the spritesheets before and after bug 934032, it looks like hover and active states were added. I think that might have been accidental - if I had to guess, I'd say that the OS X spritesheets had been copied over instead of using the ones for Windows.

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

Here are the spritesheets from before bug 934032 landed.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 2 • 11 years ago

And after.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 3 • 11 years ago

Marina - unless I'm very much mistaken, I don't think we wanted to add the hover and active states just yet for Windows 8, and we weren't going to use those for XP or 7.

Would it be possible to use the older spritesheets, and just include the Metro icon in there?

We might want to sync up with shorlander too to make sure we're all on the same page. At the very least, I think he needs to know about the Metro icon being in the spritesheets to make sure his local assets are up to date.

	Marina Samuel [:emtwo] Assignee
	Comment 4 • 11 years ago

Hi mmaslaney, it looks like the new spritesheets from bug 934029 are not the correct colour (they're lighter) and we actually don't want the hover/active states yet. Could we please get the Metro icon added to the spritesheets in the attachment here labelled "windows-tooblar-icons-before.zip"? We'd like the spritesheets exactly as they are in that attachment but with the Metro icon added. Thanks!

	Tim Nguyen :ntim
	Comment 7 • 11 years ago

The new icons should be kept on Windows 8/8.1 for bug 859751

	Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com)
	Comment 8 • 11 years ago

Marina, the news icons are only for Windows 8.

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

Bug 934032 wasn't supposed to mess with all icons on Windows 8 (let alone other Windows versions). We're not ready to use different icons for the hover and active states. Please fix this as requested in comment 4.

	Marina Samuel [:emtwo] Assignee
	Comment 10 • 11 years ago

Hi mmaslaney, please take a look at comment 9.

	Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com)
	Comment 11 • 11 years ago

Updated from the "before" set.

	Marina Samuel [:emtwo] Assignee
	Comment 12 • 11 years ago

Any update on what we're going to do here?

	Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com)
	Comment 13 • 11 years ago

Yes, I'm going to deliver the Metro icon as a separate png. Should have it done within the next couple hours.

	Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com)
	Comment 14 • 11 years ago

Attached, the Metro toolbar glyphs in "Aero" and "Inverted" Window 7 themes.

	Gary [:streetwolf52]
	Comment 15 • 11 years ago

Is someone going to incorporate these new glyphs in a patch in order to fix the problem?

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

(In reply to Gary [:streetwolf] from comment #15)
> Is someone going to incorporate these new glyphs in a patch in order to fix
> the problem?

Yes - I assume emtwo will, since the bug is assigned to her. Now that the assets are available, a patch will be written (probably tomorrow, since it's late in the evening here now).

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

Comment on attachment 8342551 [details] [diff] [review]
v1: Restore old toolbar png files and use new metro glyphs

Review of attachment 8342551 [details] [diff] [review]:
-----------------------------------------------------------------

It doesn't look like the Metro glyphs got added to your patch, and you've only updated the toolbar rules - I assume you'll need to update the menu-panel ones as well (https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/menupanel.inc.css#33).

And those should probably get taken out of toolbarbuttons.inc.css / menupanel.inc.css, since they really shouldn't be shared - they should probably go in browser/themes/windows/browser.css.

	Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com)
	Comment 19 • 11 years ago

Do we need the Metro glyph for menuPanel-small as well?

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 20 • 11 years ago

(In reply to mmaslaney from comment #19)
> Created attachment 8342589 [details]
> Metro_Glyph-menuPanel.png
> 
> Do we need the Metro glyph for menuPanel-small as well?

Yes we do, thanks!

	Marina Samuel [:emtwo] Assignee
	Comment 22 • 11 years ago

Addressed comments. I wasn't sure what we want to do with the Metro_Glyph-menuPanel-small.png though. I put it in the jar file but I assume we want it in case we decide to make it possible to make the Metro button part of the wide widgets so it needs a smaller icon?

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 23 • 11 years ago

(In reply to Marina Samuel [:emtwo] from comment #22)
> Created attachment 8342621 [details] [diff] [review]
> v2: Restore old toolbar & menu-panel png files and use new metro glyphs
> 
> Addressed comments. I wasn't sure what we want to do with the
> Metro_Glyph-menuPanel-small.png though. I put it in the jar file but I
> assume we want it in case we decide to make it possible to make the Metro
> button part of the wide widgets so it needs a smaller icon?

Ah shoot - I may have jumped the gun on menuPanel-small. I forgot that was just for wide-widgets. Sorry about that mmaslaney. :/

Forget about menuPanel-small - I forgot what it was for.

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

Comment on attachment 8342621 [details] [diff] [review]
v2: Restore old toolbar & menu-panel png files and use new metro glyphs

Review of attachment 8342621 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this is the right idea. Let's drop the menuPanel-small Metro_Glyph's though. My bad.

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

Comment on attachment 8342653 [details] [diff] [review]
v3: Restore old toolbar & menu-panel png files and use new metro glyphs

Review of attachment 8342653 [details] [diff] [review]:
-----------------------------------------------------------------

This appears to do the job. Thanks emtwo! Let's land this puppy.

	Marina Samuel [:emtwo] Assignee
	Comment 27 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/8cc62acf53a5

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/8cc62acf53a5

	Manuela Muntean [Away]
	Comment 29 • 11 years ago

Verified as fixed, for iteration #20, with latest Nightly (build ID: 20131209053402) on Win 8 64-bit.

The "Windows 8 Touch" button isn't too light. Please see the attached screenshot for details.