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)
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.
Reporter | ||
Comment 1•11 years ago
|
||
Here are the spritesheets from before bug 934032 landed.
Reporter | ||
Comment 2•11 years ago
|
||
And after.
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.
Flags: needinfo?(msamuel)
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!
Flags: needinfo?(msamuel) → needinfo?(mmaslaney)
Updated•11 years ago
|
Assignee: nobody → msamuel
Blocks: metrov1it20
Status: NEW → ASSIGNED
Whiteboard: [Australis:P2] → [Australis:P2] [block28] feature=defect c=tbd u=tbd p=1
Updated•11 years ago
|
Summary: Toolbar icons have gotten lighter on Windows 7 → Defect - Toolbar icons have gotten lighter on Windows 7
Updated•11 years ago
|
Priority: -- → P2
QA Contact: jbecerra
Updated•11 years ago
|
Blocks: metroprofilesharing
Comment 7•11 years ago
|
||
The new icons should be kept on Windows 8/8.1 for bug 859751
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.
Assignee | ||
Comment 10•11 years ago
|
||
Hi mmaslaney, please take a look at comment 9.
Flags: needinfo?(mmaslaney)
Assignee | ||
Comment 12•11 years ago
|
||
Any update on what we're going to do here?
Flags: needinfo?(mmaslaney)
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.
Flags: needinfo?(mmaslaney)
Comment 14•11 years ago
|
||
Attached, the Metro toolbar glyphs in "Aero" and "Inverted" Window 7 themes.
Attachment #8341185 -
Attachment is obsolete: true
Comment 15•11 years ago
|
||
Is someone going to incorporate these new glyphs in a patch in order to fix the problem?
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).
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #8342551 -
Flags: review?(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.
Attachment #8342551 -
Flags: review?(mconley) → review-
Comment 19•11 years ago
|
||
Do we need the Metro glyph for menuPanel-small as well?
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!
Comment 21•11 years ago
|
||
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?
Attachment #8342551 -
Attachment is obsolete: true
Attachment #8342621 -
Flags: feedback?(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.
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.
Attachment #8342621 -
Flags: feedback?(mconley) → feedback+
Assignee | ||
Comment 25•11 years ago
|
||
Attachment #8342621 -
Attachment is obsolete: true
Attachment #8342653 -
Flags: review?(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.
Attachment #8342653 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 27•11 years ago
|
||
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
Comment 28•11 years ago
|
||
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
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.
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•