Hello icon looks too small in customization mode in retina mode

VERIFIED FIXED in Firefox 40

Status

Hello (Loop)
Client
P1
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: ntim, Assigned: mikedeboer)

Tracking

unspecified
mozilla41
x86
All
Points:
1
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Screenshot : http://cl.ly/image/1l293c0B3s1c
(Reporter)

Updated

3 years ago
Whiteboard: regression, regression-windowwanted
(Reporter)

Updated

3 years ago
Keywords: regression, regressionwindow-wanted
Whiteboard: regression, regression-windowwanted
(Reporter)

Updated

3 years ago
status-firefox40: --- → ?
status-firefox41: --- → ?
(Reporter)

Updated

3 years ago
status-firefox40: ? → affected
status-firefox41: ? → affected

Updated

3 years ago
Component: Theme → Client
Product: Firefox → Loop
Comment hidden (obsolete)
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
(Reporter)

Comment 3

3 years ago
I see the same issue on Windows with normal DPI settings.
(Reporter)

Updated

3 years ago
OS: Mac OS X → All
(Reporter)

Comment 4

3 years ago
(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.
tracking-firefox40: --- → ?
(Assignee)

Updated

3 years ago
Assignee: nobody → mdeboer
Blocks: 1048144
Status: NEW → ASSIGNED
Keywords: regression, regressionwindow-wanted
Priority: -- → P1
(Assignee)

Comment 6

3 years ago
Created attachment 8607436 [details] [diff] [review]
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.
Attachment #8607436 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

3 years ago
Iteration: --- → 41.1 - May 25
Points: --- → 1
Flags: qe-verify+
Flags: firefox-backlog+

Comment 7

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

Comment 8

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

Updated

3 years ago
Attachment #8607436 - Flags: review?(gijskruitbosch+bugs)

Comment 9

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

Comment 10

3 years ago
Created attachment 8607461 [details] [diff] [review]
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.
Attachment #8607436 - Attachment is obsolete: true
Attachment #8607436 - Flags: review?(gijskruitbosch+bugs)
Flags: needinfo?(mdeboer)
Attachment #8607461 - Flags: review?(gijskruitbosch+bugs)

Comment 11

3 years ago
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.
status-firefox39: --- → ?
tracking-firefox40: ? → +
tracking-firefox41: --- → +
https://hg.mozilla.org/mozilla-central/rev/97ca7501adc7
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Reporter)

Comment 15

3 years ago
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
status-firefox40: fixed → verified
status-firefox41: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.