Closed Bug 1205661 Opened 6 years ago Closed 6 years ago

The active blue Hello icon in the toolbar does not match the blue or other icons

Categories

(Hello (Loop) :: Client, defect, P2)

defect
Points:
1

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: sevaan, Assigned: fcampo)

Details

(Whiteboard: [visual refresh defect][color])

Attachments

(7 files, 3 obsolete files)

When a conversation is active, the Firefox Hello icon in the toolbar turns blue. That blue should match the other active states of other icons in the toolbar.

Here is a screenshot showing the icon next to the active download icon: http://i.sevaan.com/image/422b2O3K2h0J
Rank: 21
Priority: -- → P2
Whiteboard: [visual refresh defect]
Points: --- → 1
Whiteboard: [visual refresh defect] → [visual refresh defect][color]
Sevaan, we'll need new pngs for these files with the new colours:

http://mxr.mozilla.org/mozilla-central/find?string=/toolbar.png&hint=browser/loop/browser/themes/shared/toolbarbuttons.inc.css

Note that we may need to consider if we need to match the blues of the other toolbar buttons as well, as per comment 1.
Flags: needinfo?(sfranks)
Correction, these are all the images we'll need new files for:

http://mxr.mozilla.org/mozilla-central/find?text=&string=themes.*loop.*toolbar
(In reply to Mark Banner (:standard8) from comment #3)
> Correction, these are all the images we'll need new files for:
> 
> http://mxr.mozilla.org/mozilla-central/find?text=&string=themes.*loop.
> *toolbar

Maslaney, would you be able to provide these? I'm not sure who generated them in the first place.
Flags: needinfo?(sfranks) → needinfo?(mmaslaney)
Assignee: nobody → fernando.campo
This has been quiet for a while, maybe is it worth to relaunch the question, or ask another person for the assets?
Flags: needinfo?(sfranks)
Repinged mmaslaney. Assets incoming.
Flags: needinfo?(sfranks)
(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #7)
> Created attachment 8685145 [details]
> Hello-Toolbar-and-Menu-MM-110915-Recovered-assets.zip
> Assets attached.

Hi Michael, I've been checking the assets, and see that we have Win10 instead of Win8 now, and OSX-x instead of Yosemite. 

So my question is...do this new assets:
- substitute completely the existing (delete all old ones, keep just the new)?
- complement them (use the new ones but keep the old for which we don't have substitute)?
- or do we need to add specifics for all the systems (so we'd need new assets for win8 and osx-yosemite too)?
Flags: needinfo?(mmaslaney)
Flags: needinfo?(mmaslaney)
Hi Fernando,

The photoshop file I worked off of has some different naming conventions. 

Let me translate: 

• toolbar-win8.png is toolbar.png (feel free to rename back to toolbar-win8)

• Added Win10 (new theme for Windows 10) 

• toolbar-OSX-x.png is toolbar-osx-yosemite.png (feel free to rename back to the original)

Answers:

• Roughly 80% of the assets needed updating in the file, so I would replace the old with the new. 

• Keep the ones we don't have a sub for
Status: NEW → ASSIGNED
Attached image icon comparison
adding three examples for the icon difference after applying the patch (win 10, ubuntu linux, OSX yosemite)
Attachment #8693001 - Flags: ui-review?(sfranks)
Attachment #8693001 - Flags: ui-review?(b.pmm)
Attachment #8693001 - Flags: ui-review?(sfranks)
Attachment #8693001 - Flags: ui-review?(b.pmm)
Attachment #8693001 - Flags: ui-review+
Attachment #8693003 - Flags: review?(mdeboer)
Attachment #8693003 - Flags: review?(dmose)
just updating patch after add-on landed
Attachment #8693003 - Attachment is obsolete: true
Attachment #8693003 - Flags: review?(mdeboer)
Attachment #8693003 - Flags: review?(dmose)
Attachment #8693643 - Flags: review?(mdeboer)
Attachment #8693643 - Flags: review?(dmose)
Comment on attachment 8693643 [details] [diff] [review]
Blue shade of icon in toolbar does not match other blue icons

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

- browser/extensions/loop/skin/linux/toolbar-inverted.png
 - browser/extensions/loop/skin/linux/toolbar-inverted@2x.png
    - The blue icons seem to have switched positions. I think you should swap them back. Please also notify mmaslaney/ sevaan about this.
 - browser/extensions/loop/skin/linux/toolbar@2x.png
    - The file became 10k in size. Can you shrink it?
    - We now have drop-shadow here. Is that intentional?
 - browser/extensions/loop/skin/osx/toolbar-inverted.png
 - browser/extensions/loop/skin/osx/toolbar-inverted@2x.png
    - Same as with the Linux icons: swapped.
 - browser/extensions/loop/skin/windows/toolbar-inverted.png
 - browser/extensions/loop/skin/windows/toolbar-inverted@2x.png
    - Same as with the Linux and OSX icons: swapped.

If the swapping of the blue icons in the inverted icon sets is intentional, r=me with the one file crushed to a smaller size.
Attachment #8693643 - Flags: review?(mdeboer)
Attachment #8693643 - Flags: review?(dmose)
Attachment #8693643 - Flags: review-
Asking UX if swapping icons was intended. 
and about the size, all @2x assets have same size and 72ppi, so asking too about the export options to reduce without quality loss.
Flags: needinfo?(sfranks)
Flags: needinfo?(mmaslaney)
We are swapping the icons for an updated set that matches the blue highlight. I'm looking at the file and each glyph set has two sizes.
Flags: needinfo?(mmaslaney)
Attached image linux_diff
(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #15)
> We are swapping the icons for an updated set that matches the blue highlight.
I understand from this that the swap was intended and should stay as it is on the new icons, right?

> I'm looking at the file and each glyph set has two sizes.
Meant file size (linux-toolbar@2x was 4K before, is 10K now)
Also, Mike spotted a drop-shadow on the new version (attached, old version on the left, new on the right)
Flags: needinfo?(mmaslaney)
Yes, the swap is intended. 

I'm not sure why the assets are higher, as I ran them through the image optimizer.
Flags: needinfo?(mmaslaney)
(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #18)
> I'm not sure why the assets are higher, as I ran them through the image
> optimizer.
Guess it has to do with the drop-shadow, so only question is if this was intended too or should exchange for another version.

(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #19)
> Created attachment 8699529 [details]
> Screen Shot 2015-12-17 at 10.05.52 AM.png
> 
> Here's a correct one-to-one.
I might be a little dense today (it might have been the christmas party yesterday), but I'm not sure what are you trying to show me in this one-to-one. Is this related to the new drop-shadow on linux toolbar assets?
Flags: needinfo?(mmaslaney)
I've re-checked all the icons, and made a little comparison https://docs.google.com/spreadsheets/d/13Rja5ECj51ATlVOH5dLXQOZRtUjxxKk2PitxMWahIv0/edit?usp=sharing with the old ones on the left and  new ones on the right.
- all *-inverted.png icon swap is confirmed as intended
- after checking the current linux toolbar, it seems that the rest of icons (not related to Hello) all have the shadow, so my best guess is that the change is also intended.
- The extra shadow explains the extra size, and after putting the icon through an optimizer myself, there's no gain (without quality loss)
 
So at this point, with all the issues raised, I thinks it's better to land it as it is, instead of keep waiting.
Attachment #8693643 - Attachment is obsolete: true
Attachment #8704187 - Flags: review?(mdeboer)
bad patch attachment :(
Attachment #8704187 - Attachment is obsolete: true
Attachment #8704187 - Flags: review?(mdeboer)
Attachment #8704280 - Flags: review?(mdeboer)
Comment on attachment 8704280 [details] [diff] [review]
Blue shade of icon in toolbar does not match other blue icons

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

Fernando, <3 the icon comparison doc! Super useful!
Attachment #8704280 - Flags: review?(mdeboer) → review+
Fixing the images overrides on jar.mn after Mark warning
Carrying over the r+
Attachment #8704698 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f579b4fc5bf5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Flags: needinfo?(sfranks)
Removing obsolete NI
Flags: needinfo?(mmaslaney)
You need to log in before you can comment on or make changes to this bug.