bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Update Loop button icon images and rendered size

VERIFIED FIXED in Firefox 35

Status

Hello (Loop)
Client
P2
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: pauly, Assigned: Paolo)

Tracking

({regression})

unspecified
mozilla37
regression
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox33 unaffected, firefox34+ wontfix, firefox35+ fixed, firefox36+ verified, firefox37 verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8510854 [details]
Screenshot
(Reporter)

Updated

4 years ago
Blocks: 1083396
The shade of gray will also be off on Windows depending on the OS theme.

Why is this icon not part of Toolbar.png?
Flags: needinfo?(mdeboer)
Flags: needinfo?(jaws)
[Tracking Requested - why for this release]:
This button is visible in the default primary UI. Shouldn't ship like this.
tracking-firefox34: --- → ?
tracking-firefox35: --- → ?
tracking-firefox36: --- → ?

Updated

4 years ago
Keywords: regression
OS: Windows 7 → All
Hardware: x86_64 → All
Jared -- Can you take this one when you're back on Monday?
status-firefox33: --- → unaffected
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → affected
tracking-firefox34: ? → +
tracking-firefox35: ? → ---
tracking-firefox36: ? → ---
Flags: needinfo?(mdeboer)
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.
backlog: --- → Fx35?
Flags: needinfo?(mmaslaney)
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.
Flags: needinfo?(mmaslaney)
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?
Flags: needinfo?(mmaslaney)
Sure, I'll try to get those assets updated, today.
Flags: needinfo?(mmaslaney)
backlog: Fx35? → Fx35+
Priority: -- → P2
(Assignee)

Updated

4 years ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED

Updated

4 years ago
Flags: qe-verify?
Flags: firefox-backlog+
(Assignee)

Updated

4 years ago
Flags: qe-verify? → qe-verify+

Updated

4 years ago
Iteration: --- → 36.3
Points: --- → 2

Comment 9

4 years ago
This bug also affects Windows XP.

Updated

4 years ago
Summary: The new Loop icon is blue grey on Linux → The new Loop icon is blue grey on Linux and Windows XP

Comment 10

4 years ago
Note that all loop assets need to be updated since the assets from Bug 1088021 fixed some alignment and inconsistency issues as well.
(Assignee)

Updated

4 years ago
Summary: The new Loop icon is blue grey on Linux and Windows XP → Update Loop button icon images and rendered size
(Assignee)

Comment 11

4 years ago
Created attachment 8523808 [details] [diff] [review]
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.
Attachment #8523808 - Flags: feedback?(zer0)
Attachment #8523808 - Flags: feedback?(mixedpuppy)
Attachment #8523808 - Flags: feedback?(jaws)
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.
(Assignee)

Updated

4 years ago
Blocks: 1100308
(Assignee)

Comment 13

4 years ago
(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.
Flags: needinfo?(jaws)
(Assignee)

Comment 14

4 years ago
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.
Flags: needinfo?(mmaslaney)
(Assignee)

Comment 15

4 years ago
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.
Flags: needinfo?(ntim007)
(Assignee)

Comment 16

4 years ago
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.
(Assignee)

Comment 17

4 years ago
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

4 years ago
(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.
Flags: needinfo?(ntim007)
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?
Flags: needinfo?(mreavy)
[Tracking Requested - why for this release]:

Let's re-target this for Fx35.  We don't need to hold Fx34 for this.
status-firefox34: affected → wontfix
tracking-firefox34: + → ---
tracking-firefox35: --- → ?
Flags: needinfo?(mreavy)
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.
tracking-firefox34: --- → +
Hi Jared  -- Can you work with Paolo to get this landed as soon as we can?  Thanks!
Flags: needinfo?(jaws)
tracking-firefox35: ? → +
tracking-firefox36: --- → +

Updated

4 years ago
Iteration: 36.3 → 37.1

Updated

4 years ago
backlog: Fx35+ → Fx36+

Updated

4 years ago
Iteration: 37.1 → 37.2
Hi Paolo -- is there any way we can move this forward?  Or are you stuck waiting for feedback?
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 24

4 years ago
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.
Flags: needinfo?(paolo.mozmail)
Attachment #8523808 - Flags: feedback?(zer0)
Comment on attachment 8523808 [details] [diff] [review]
First attempt

I don't see any obvious issue with the css change.
Attachment #8523808 - Flags: feedback?(mixedpuppy) → feedback+
(Assignee)

Comment 26

4 years ago
(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?
Flags: needinfo?(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.
Flags: needinfo?(mixedpuppy)

Comment 28

4 years ago
(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

4 years ago
(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.
(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.
(Assignee)

Comment 31

4 years ago
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.
Attachment #8523808 - Flags: ui-review?(mmaslaney)
Attachment #8523808 - Flags: review?(jaws)
Attachment #8523808 - Flags: feedback?(jaws)
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.
Attachment #8523808 - Flags: review?(jaws) → review+
(Assignee)

Comment 33

4 years ago
(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.
Flags: needinfo?(jaws)
(Assignee)

Updated

4 years ago
Blocks: 1112739
(Assignee)

Comment 34

4 years ago
Created attachment 8538039 [details] [diff] [review]
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).
Attachment #8523808 - Attachment is obsolete: true
Attachment #8523808 - Flags: ui-review?(mmaslaney)
Flags: needinfo?(mmaslaney)
https://hg.mozilla.org/mozilla-central/rev/64b36c92fc80
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Can we get beta/aurora uplift nomination here?
Flags: needinfo?(paolo.mozmail)
status-firefox37: --- → fixed
(Assignee)

Comment 37

4 years ago
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
Flags: needinfo?(paolo.mozmail)
Attachment #8538039 - Flags: approval-mozilla-beta?
Attachment #8538039 - Flags: approval-mozilla-aurora?

Comment 38

4 years ago
This patch works fine on Windows 8.
Keywords: qawanted
Attachment #8538039 - Flags: approval-mozilla-beta?
Attachment #8538039 - Flags: approval-mozilla-beta+
Attachment #8538039 - Flags: approval-mozilla-aurora?
Attachment #8538039 - Flags: approval-mozilla-aurora+

Updated

4 years ago
Blocks: 1097062
Dropping qawanted since this is already flagged for verification.
Keywords: qawanted

Updated

4 years ago
Iteration: 37.2 → 37.1
(Reporter)

Comment 42

4 years ago
(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
Status: RESOLVED → VERIFIED
status-firefox36: fixed → verified
status-firefox37: fixed → verified
You need to log in before you can comment on or make changes to this bug.