Closed Bug 1165679 Opened 5 years ago Closed 5 years ago

Hello icon looks too small in customization mode in retina mode

Categories

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

x86
All
defect
Points:
1

Tracking

(firefox39 ?, firefox40+ verified, firefox41+ verified)

VERIFIED FIXED
mozilla41
Iteration:
41.1 - May 25
Tracking Status
firefox39 --- ?
firefox40 + verified
firefox41 + verified

People

(Reporter: ntim, Assigned: mikedeboer)

References

Details

Attachments

(1 file, 1 obsolete file)

Screenshot : http://cl.ly/image/1l293c0B3s1c
Whiteboard: regression, regression-windowwanted
Whiteboard: regression, regression-windowwanted
Component: Theme → Client
Product: Firefox → Loop
This only affects retina mode. Probably an issue in the css somewhere.
Summary: Hello icon looks too small in customization mode → Hello icon looks too small in customization mode in retina mode
I see the same issue on Windows with normal DPI settings.
OS: Mac OS X → All
(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)
[Tracking Requested - why for this release]: Noticeable UI glitch for people on HiDPI OSX and normal DPI Windows.
Assignee: nobody → mdeboer
Blocks: 1048144
Status: NEW → ASSIGNED
Priority: -- → P1
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.
Attachment #8607436 - Flags: review?(gijskruitbosch+bugs)
Iteration: --- → 41.1 - May 25
Points: --- → 1
Flags: qe-verify+
Flags: firefox-backlog+
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?
Attachment #8607436 - Flags: review?(gijskruitbosch+bugs)
(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.
Attachment #8607436 - Flags: review?(gijskruitbosch+bugs)
(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?
Flags: needinfo?(mdeboer)
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.
Attachment #8607436 - Attachment is obsolete: true
Attachment #8607436 - Flags: review?(gijskruitbosch+bugs)
Flags: needinfo?(mdeboer)
Attachment #8607461 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8607461 - Flags: review?(gijskruitbosch+bugs) → review+
Tracking enabled for 40 and 41, to ensure the Hello icons are properly sized in all implementations, including retina.
https://hg.mozilla.org/mozilla-central/rev/97ca7501adc7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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
Attachment #8607461 - Flags: approval-mozilla-aurora?
Attachment #8607461 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: bogdan.maris
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.