1088508 - Update Loop button icon images and rendered size

Status: VERIFIED FIXED

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

Description

[Paul Silaghi, QA [:pauly]]

Attachment: Screenshot

Comment 1

[Dão Gottwald [:dao]]

The shade of gray will also be off on Windows depending on the OS theme.

Why is this icon not part of Toolbar.png?

Comment 2

[Dão Gottwald [:dao]]

[Tracking Requested - why for this release]:
This button is visible in the default primary UI. Shouldn't ship like this.

Comment 3

[Maire Reavy [:mreavy]]

Jared -- Can you take this one when you're back on Monday?

Comment 4

[Maire Reavy [:mreavy]]

Hi Mike (Maslaney) -- Can you take a look at this in Fx34 on Linux and tell us how badly this fix is needed?  It's currently tracking Fx34, and Jared and I would appreciate your feedback on whether this is a needed fix for Fx34 (versus Fx35).  Thanks.

Comment 5

[Michael Maslaney [:mmaslaney] ([e-mail])]

I would suggest we fix it. I'm currently in the process of updating the icons, in terms of size and can easily add this to the list of issues to fix.

Comment 6

[Maire Reavy [:mreavy]]

Thanks, Mike.  In order for us to fix this in fx34, we need the fix for this bug in the next day or two.  Is that possible?

Comment 7

[Michael Maslaney [:mmaslaney] ([e-mail])]

Sure, I'll try to get those assets updated, today.

Comment 8

[Michael Maslaney [:mmaslaney] ([e-mail])]

Updated assets placed in Bug 1088021

Comment 9

[Tim Nguyen :ntim]

This bug also affects Windows XP.

Comment 10

[Tim Nguyen :ntim]

Note that all loop assets need to be updated since the assets from Bug 1088021 fixed some alignment and inconsistency issues as well.

Comment 11

[:Paolo Amadini]

Attachment: First attempt

Despite the fix in bug 1099952, the icon of the "badged button" for Loop on Windows and Mac was still scaled down from 18px to 16px because of this rule:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#841

On Linux, this is overridden by the theme:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.css#585

The fix in this patch demonstrates how the problem can be solved, but it's quite likely that this will break other things.

Shane, Matteo, do any of you know the reasons for the existence of this rule? How can it be properly refactored for what we need here, possibly without the theme difference on Linux? Bug 891219 and bug 914921 came up from hg annotate.

Jared, the rest of this patch addresses the various other theme issues - separate comment upcoming in the bug.

Comment 12

[Dão Gottwald [:dao]]

Can somebody answer why these icons aren't part of Toolbar-*.png? If there's no strong reason for that, please move them. Having them separate means that they'll likely be forgotten again if we add new variants.

Comment 13

[:Paolo Amadini]

(In reply to Dão Gottwald [:dao] from comment #12)
> Can somebody answer why these icons aren't part of Toolbar-*.png? If there's
> no strong reason for that, please move them. Having them separate means that
> they'll likely be forgotten again if we add new variants.

Hey Dão, I was just about to answer this question. I don't know if this was a deliberate decision. I filed bug 1100308 with some additional details. Handling this issue in a separate bug is probably easier, so it can be weighted against the possibly imminent Beta and Aurora uplifts.

Comment 14

[:Paolo Amadini]

Hey Michael, in this bug I used the assets from bug 1088021 (attachment 8513779 [details]), that I think are the same as bug 1083396 (attachment 8513822 [details]). I have noticed the following issues:

1. The menu panel icons do not need inverted versions - they are only used for icons that can open panel subviews, like History. So, I've removed the second row from all variants of the menu panel PNGs on all platforms, as those areas are not referenced in CSS.

2. The inverted toolbar button icons on Windows and Linux look smaller than the normal icon because of the border. This effect does not happen for other toolbar icons. This can be easily verified using about:config, enabling the devtools theme and then switching between the dark and light versions. On Mac however, both normal and HiDPI, the inverted icons have the correct size.

3. There is a slight vertical misalignment on Windows for the menu panel icons between the Aero and normal (Windows 8) versions. You can verify this by overlaying the two images in any image viewer and switching between the two. Such misalignment is not present if you do this with the menuPanel.png and manuPanel-aero.png images in the product. I admit this is a minor issue because the two images are never used together on the same system.

Comment 15

[:Paolo Amadini]

Tim, I've noticed you attached optimized assets in bug 1088021. However, I've also noticed the difference is literally just a few bytes per folder, and if there are no other changes to the images, I'd rather use the versions I've already included in the patch to save some work.

I already ran pngcrush (without brute forcing) on all the files Michael had attached, and there was no improvement, indicating they were already optimized.

Comment 16

[:Paolo Amadini]

Jared, in the attached patch, I've fixed the CSS to distinguish between the XP and Aero versions of the toolbar buttons, that became available in the latest assets. Also, I think I fixed the version of the menu panel icon used on Windows 8, but I'm not sure since I can't test on that platform.

Comment 17

[:Paolo Amadini]

This is the test matrix for the toolbar button changes. It's crazy how many versions of the icons we have to create, package and maintain - 27 variants just for the basic state, and there are 7 possible icon states. And considering that for Loop we have special CSS for the palette to ensure a non-default state is never used there, the 27 places to test become 36.

Here is the platform list. Unfortunately, I could only test the platforms marked with an asterisk:

* Windows XP - Classic theme
* Windows XP - Default theme with Silver color scheme
* Windows 7
- Windows 8
* Mac OS X 10.9 - Standard DPI
* Mac OS X 10.9 - HiDPI
- Mac OS X 10.10 - Standard DPI
- Mac OS X 10.10 - HiDPI
* Linux

For each platform, this is what I've tested:

* Toolbar button
* Toolbar button with dark theme
* Menu panel button
* Menu panel button placed in the palette

On a sample of the platforms, I've also tested:

* Toolbar button in "do not disturb" mode

I've not tested on any platform:

- The other 5 states of the toolbar button

Comment 18

[Tim Nguyen :ntim]

(In reply to :Paolo Amadini from comment #15)
> Tim, I've noticed you attached optimized assets in bug 1088021. However,
> I've also noticed the difference is literally just a few bytes per folder,
> and if there are no other changes to the images, I'd rather use the versions
> I've already included in the patch to save some work.
> 
> I already ran pngcrush (without brute forcing) on all the files Michael had
> attached, and there was no improvement, indicating they were already
> optimized.
There was an economy of 0.1% when I optimized them (using lossless compression), which is almost nothing though, feel free to use the ones attached by mmaslaney.

(In reply to :Paolo Amadini from comment #14)
> Hey Michael, in this bug I used the assets from bug 1088021 (attachment
> 8513779 [details]), that I think are the same as bug 1083396 (attachment
> 8513822 [details]). I have noticed the following issues:
> 
> 1. The menu panel icons do not need inverted versions - they are only used
> for icons that can open panel subviews, like History. So, I've removed the
> second row from all variants of the menu panel PNGs on all platforms, as
> those areas are not referenced in CSS.
We *might* use those inverted versions for dev-edition if we decide to make the panels dark at some point.

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

We're very late in beta at this point. If this is simply packaging new assets and a few style lines (as shown in the first attempt patch, can this be wrapped up today for beta10? (Needs to be done in the next 2 hours.) If not, are we ok to ship 34 in the current state?

Comment 20

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:

Let's re-target this for Fx35.  We don't need to hold Fx34 for this.

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Thanks Maire. FYI, you can leave the tracking flag when changing the status flag. Anything other than 'affected' won't show on relman's radar.

Comment 22

[Maire Reavy [:mreavy]]

Hi Jared  -- Can you work with Paolo to get this landed as soon as we can?  Thanks!

Comment 23

[Maire Reavy [:mreavy]]

Hi Paolo -- is there any way we can move this forward?  Or are you stuck waiting for feedback?

Comment 24

[:Paolo Amadini]

Comment on attachment 8523808 [details] [diff] [review]
First attempt

I've talked to Matteo and it seems the CSS rules for badged buttons are leftovers that may just be removed, not sure for the social API though, still waiting for Shane to comment on this if he remembers. I might split the rule removal into another bug so we might track any possible regressions better.

Comment 25

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8523808 [details] [diff] [review]
First attempt

I don't see any obvious issue with the css change.

Comment 26

[:Paolo Amadini]

(In reply to Shane Caraveo (:mixedpuppy) from comment #25)
> I don't see any obvious issue with the css change.

Well, by now my question from comment 11 was buried between other discussions, and was rather about why the rule existed. Matteo suggested it is a leftover and could just be removed entirely for the badged button, is this the same for the social button? How can I test it?

Comment 27

[Shane Caraveo (:mixedpuppy)]

(In reply to :Paolo Amadini from comment #26)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #25)
> > I don't see any obvious issue with the css change.
> 
> Well, by now my question from comment 11 was buried between other
> discussions, and was rather about why the rule existed. Matteo suggested it
> is a leftover and could just be removed entirely for the badged button, is
> this the same for the social button? How can I test it?

I'm guessing (based on testing and vague memory) that max-width is necessary in case a socialapi provider uses a larger sized image for the button (we may not be able to control that).  It cannot be removed for the badged button right now (without some other fix).

I'd suggest making the change in the patch and using a different bug to look at removal.

Comment 28

[Tim Nguyen :ntim]

(In reply to Shane Caraveo (:mixedpuppy) from comment #27)
> (In reply to :Paolo Amadini from comment #26)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #25)
> > > I don't see any obvious issue with the css change.
> > 
> > Well, by now my question from comment 11 was buried between other
> > discussions, and was rather about why the rule existed. Matteo suggested it
> > is a leftover and could just be removed entirely for the badged button, is
> > this the same for the social button? How can I test it?
> 
> I'm guessing (based on testing and vague memory) that max-width is necessary
> in case a socialapi provider uses a larger sized image for the button (we
> may not be able to control that).  It cannot be removed for the badged
> button right now (without some other fix).
> 
> I'd suggest making the change in the patch and using a different bug to look
> at removal.
How about reducing it to 18px?

Comment 29

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #28)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #27)
> > (In reply to :Paolo Amadini from comment #26)
> > > (In reply to Shane Caraveo (:mixedpuppy) from comment #25)
> > > > I don't see any obvious issue with the css change.
> > > 
> > > Well, by now my question from comment 11 was buried between other
> > > discussions, and was rather about why the rule existed. Matteo suggested it
> > > is a leftover and could just be removed entirely for the badged button, is
> > > this the same for the social button? How can I test it?
> > 
> > I'm guessing (based on testing and vague memory) that max-width is necessary
> > in case a socialapi provider uses a larger sized image for the button (we
> > may not be able to control that).  It cannot be removed for the badged
> > button right now (without some other fix).
> > 
> > I'd suggest making the change in the patch and using a different bug to look
> > at removal.
> How about reducing it to 18px?

Err. Obviously meant increasing.

Comment 30

[Shane Caraveo (:mixedpuppy)]

(In reply to Tim Nguyen [:ntim] from comment #29)

> > > I'd suggest making the change in the patch and using a different bug to look
> > > at removal.
> > How about reducing it to 18px?
> 
> Err. Obviously meant increasing.

That's what the patch does.

Comment 31

[:Paolo Amadini]

Comment on attachment 8523808 [details] [diff] [review]
First attempt

Upgrading feedback to review as per latest comment.

Still waiting for UI updates on issues from comment 14, especially number 2:

1. The menu panel icons do not need inverted versions - they are only used for icons that can open panel subviews, like History. So, I've removed the second row from all variants of the menu panel PNGs on all platforms, as those areas are not referenced in CSS.

2. The inverted toolbar button icons on Windows and Linux look smaller than the normal icon because of the border. This effect does not happen for other toolbar icons. This can be easily verified using about:config, enabling the devtools theme and then switching between the dark and light versions. On Mac however, both normal and HiDPI, the inverted icons have the correct size.

3. There is a slight vertical misalignment on Windows for the menu panel icons between the Aero and normal (Windows 8) versions. You can verify this by overlaying the two images in any image viewer and switching between the two. Such misalignment is not present if you do this with the menuPanel.png and manuPanel-aero.png images in the product. I admit this is a minor issue because the two images are never used together on the same system.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8523808 [details] [diff] [review]
First attempt

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

r+ with one nit:

browser/themes/windows/loop/toolbar-XP.png was created as a new file, and browser/themes/windows/loop/toolbar-XPVista7.png was deleted. Can you do this as a `hg rename` from toolbar-XPVista7.png to toolbar-XP.png and then modify overwrite the binary file? I'd like to keep the `hg log` intact for the file.

I can confirm the issues that you mentioned in your review request comment, but I don't think any of them are major enough to hold back landing this change.

Comment 33

[:Paolo Amadini]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #32)
> I can confirm the issues that you mentioned in your review request comment,
> but I don't think any of them are major enough to hold back landing this
> change.

Thanks! I'll update the patch and land, and file a new bug for the issues.

Comment 34

[:Paolo Amadini]

Attachment: Checked in

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #32)
> this as a `hg rename` from toolbar-XPVista7.png to toolbar-XP.png and then

Done (the actual rename target is toolbar-aero.png, the new image is the XP classic version).

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64b36c92fc80

Comment 36

[Lukas Blakk [:lsblakk] use ?needinfo]

Can we get beta/aurora uplift nomination here?

Comment 37

[:Paolo Amadini]

Comment on attachment 8538039 [details] [diff] [review]
Checked in

(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #36)
> Can we get beta/aurora uplift nomination here?

From what I can tell this is a big improvement in the quality of the icon, so this is probably worth nominating.

Approval Request Comment
[Feature/regressing bug #]: Hello
[User impact if declined]: Blurry (scaled down) toolbar button icon on Windows and Mac, wrong icon style on Windows 8, misaligned non-default states of the icon
[Describe test coverage new/current, TBPL]: Comment 17
[Risks and why]: Chances of external Social Provider icon sizes being scaled down to a larger size (18px) if they are not 16px already (comment 27). The patch has not been explicitly tested on Windows 8 and Mac OS X 10.10 (comment 17). No automated testing possible due to the visual nature of the bug, thus requires extensive QA on all platforms.
[String/UUID change made/needed]: None

Comment 38

[Tim Nguyen :ntim]

This patch works fine on Windows 8.

Comment 39

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/877827bc103a

Comment 40

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/20eceb4c7be4

Comment 41

[u279076]

Dropping qawanted since this is already flagged for verification.

Comment 42

[Paul Silaghi, QA [:pauly]]

(In reply to Tim Nguyen [:ntim] from comment #38)
> This patch works fine on Windows 8.
Everything's also fine on 36b2, 37.0a2 (2015-01-22) OS X 10.9.5, Ubuntu 12.04