Closed Bug 944781 Opened 11 years ago Closed 11 years ago

Defect - Toolbar icons have gotten lighter on Windows 7

Categories

(Firefox :: Theme, defect, P2)

x86
Windows 7
defect

Tracking

()

VERIFIED FIXED
Firefox 28

People

(Reporter: mconley, Assigned: emtwo)

References

(Blocks 1 open bug)

Details

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

Attachments

(7 files, 3 obsolete files)

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.
Attached file windows-tooblar-icons-before.zip (deleted) —
Here are the spritesheets from before bug 934032 landed.
Attached file windows-toolbar-icons-after.zip (deleted) —
And after.
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)
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
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)
Attached file Windows8_toolbar.zip (obsolete) (deleted) —
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)
Attached file Metro_Glyph.zip (deleted) —
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).
Attachment #8342551 - Flags: review?(mconley)
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-
Attached image Metro_Glyph-menuPanel.png (deleted) —
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+
Attachment #8342621 - Attachment is obsolete: true
Attachment #8342653 - Flags: review?(mconley)
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+
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
Status: ASSIGNED → RESOLVED
Closed: 11 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
Attached image screenshot.png (deleted) —
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.

Attachment

General

Created:
Updated:
Size: