1165679 - Hello icon looks too small in customization mode in retina mode

Status: VERIFIED FIXED

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

Description

[Tim Nguyen :ntim]

Screenshot : http://cl.ly/image/1l293c0B3s1c

Comment 1

[:Gijs (he/him)]

Works on Windows, so might be OS X - specific.

Comment 2

[Mark Banner (:standard8)]

This only affects retina mode. Probably an issue in the css somewhere.

Comment 3

[Tim Nguyen :ntim]

I see the same issue on Windows with normal DPI settings.

Comment 4

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] (limited availability) from comment #3)
> I see the same issue on Windows with normal DPI settings.

(Windows 8 more precisely)

Comment 5

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

[Tracking Requested - why for this release]: Noticeable UI glitch for people on HiDPI OSX and normal DPI Windows.

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: show the proper size Hello icon inside the customization palette

I couldn't think of a downside to making the rule here more specifically applying to the icon. And since I introduced this line in the first place, it appears that I caused this regression.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8607436 [details] [diff] [review]
Patch v1: show the proper size Hello icon inside the customization palette

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

Why not also for all the other badge-containers in that rule?

Comment 8

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #7)
> Why not also for all the other badge-containers in that rule?

Because that appears to make to button too large inside the menu-panel as it contains the label there as well.

Comment 9

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #8)
> (In reply to :Gijs Kruitbosch from comment #7)
> > Why not also for all the other badge-containers in that rule?
> 
> Because that appears to make to button too large inside the menu-panel as it
> contains the label there as well.

This doesn't make sense to me. You're changing the width of only the icon. toolbarbutton-icon doesn't have the label in it. What, precisely, do you mean?

Comment 10

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: show the proper size Hello icon inside the customization palette

Gijs, thanks for making me look harder at this! It appears that the rules were correct before, as applying them to the .toolbarbutton-badge-container makes the badge itself be positioned correctly.
Now there was already a rule added for the toolbarbutton-icon inside the menu-panel, but that didn't yet apply to the icon inside the palette. With this patch it does.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8607461 [details] [diff] [review]
Patch v2: show the proper size Hello icon inside the customization palette

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

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +450,5 @@
>  }
>  
>  /* above we treat the container as the icon for the margins, that is so the
>  /* badge itself is positioned correctly. Here we make sure that the icon itself
>  /* has the minum size we want, but no padding/margin. */

Nit: please fix "minum" while we're here.

Comment 12

[jen strickland :jenstrickland]

Tracking enabled for 40 and 41, to ensure the Hello icons are properly sized in all implementations, including retina.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/97ca7501adc7

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/97ca7501adc7

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8607461 [details] [diff] [review]
Patch v2: show the proper size Hello icon inside the customization palette

Approval Request Comment
[Feature/regressing bug #]: (not tracked down yet)
[User impact if declined]: Hello icon is too small in customize mode
[Describe test coverage new/current, TreeHerder]: Fix works fine on m-c
[Risks and why]: Very low, css only fix
[String/UUID change made/needed]: nope

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d937fbca8972

Comment 17

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue using Nightly from 2015-05-18, verified that the issue is fixed on Surface Pro 2, Windows 7 64-bit 125 dpi and MacBook Pro late 2013 using latest Nightly and latest Aurora.