Defect - Toolbar icons have gotten lighter on Windows 7

VERIFIED FIXED in Firefox 28

Status

()

defect
P2
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: mconley, Assigned: emtwo)

Tracking

(Blocks 1 bug)

Trunk
Firefox 28
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:P2] [block28] feature=defect c=tbd u=tbd p=1)

Attachments

(7 attachments, 3 obsolete attachments)

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.
Here are the spritesheets from before bug 934032 landed.
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.
Flags: needinfo?(msamuel)
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!
Flags: needinfo?(msamuel) → needinfo?(mmaslaney)
Duplicate of this bug: 944790
Assignee: nobody → msamuel
Blocks: metrov1it20
Status: NEW → ASSIGNED
Whiteboard: [Australis:P2] → [Australis:P2] [block28] feature=defect c=tbd u=tbd p=1
Blocks: 934032
Summary: Toolbar icons have gotten lighter on Windows 7 → Defect - Toolbar icons have gotten lighter on Windows 7
Priority: -- → P2
QA Contact: jbecerra
Duplicate of this bug: 944939
The new icons should be kept on Windows 8/8.1 for bug 859751
Marina, the news icons are only for Windows 8.
Flags: needinfo?(mmaslaney)
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.
Hi mmaslaney, please take a look at comment 9.
Flags: needinfo?(mmaslaney)
Posted file Windows8_toolbar.zip (obsolete) —
Updated from the "before" set.
Flags: needinfo?(mmaslaney)
Any update on what we're going to do here?
Flags: needinfo?(mmaslaney)
Yes, I'm going to deliver the Metro icon as a separate png. Should have it done within the next couple hours.
Flags: needinfo?(mmaslaney)
Posted file Metro_Glyph.zip
Attached, the Metro toolbar glyphs in "Aero" and "Inverted" Window 7 themes.
Attachment #8341185 - Attachment is obsolete: true
Is someone going to incorporate these new glyphs in a patch in order to fix the problem?
(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).
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.
Attachment #8342551 - Flags: review?(mconley) → review-
Do we need the Metro glyph for menuPanel-small as well?
(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!
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?
Attachment #8342551 - Attachment is obsolete: true
Attachment #8342621 - Flags: feedback?(mconley)
(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.
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.
Attachment #8342621 - Flags: feedback?(mconley) → feedback+
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.
Attachment #8342653 - Flags: review?(mconley) → review+
https://hg.mozilla.org/integration/fx-team/rev/8cc62acf53a5
Whiteboard: [Australis:P2] [block28] feature=defect c=tbd u=tbd p=1 → [Australis:P2][fixed-in-fx-team] [block28] feature=defect c=tbd u=tbd p=1
https://hg.mozilla.org/mozilla-central/rev/8cc62acf53a5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][fixed-in-fx-team] [block28] feature=defect c=tbd u=tbd p=1 → [Australis:P2] [block28] feature=defect c=tbd u=tbd p=1
Target Milestone: --- → Firefox 28
Posted image screenshot.png
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.