1257672 - Hello button on Mac retina display is 2px larger than the other buttons

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

Attachment: Screen shot - non-retina left, retina right

In bug 1233701 we changed the non-retina buttons to use 16x16 images (from 18x18), and the retina ones to be 32x32 (from 36x36).

However, on our Mac displays, we've noticed that the Hello button now looks slightly bigger than the rest of the buttons - see attached screenshot.

Measuring the retina display toolbar button it comes out as 36x36 and the rest of the buttons are smaller (we're not quite sure exactly what they are meant to be).

Reverting the retina icons back to 34x34 appears to fix the issue. However, we are unsure if this is the right fix due to the unexpected difference with the non-retina display, and if this will adversely affect other platforms or not.

Gijs, can you please help us out here? We're not sure what the right thing to do is.

Comment 1

[:Gijs (out for now; he/him)]

How do I reproduce this?

On regular OS X nightly on hidpi DOM Inspector says all the buttons have a height of 24 css pixels. So, I'm not really sure what you're asking. You also seem to be using device pixels when talking about the size of things, which just makes me more confused...

Comment 2

[Mark Banner (:standard8)]

We took screenshots and measured them using preview. Just loading up latest nightly on a retina display, and then comparing it to a non-retina should highlight the issue. On the retina display the Hello button just "feels" bigger than the others.

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Mark Banner (:standard8) from comment #2)
> We took screenshots and measured them using preview. Just loading up latest
> nightly on a retina display, and then comparing it to a non-retina should
> highlight the issue. On the retina display the Hello button just "feels"
> bigger than the others.

It looks like your CSS right now looks like this:

  :-moz-any(toolbar, .widget-overflow-list) #loop-button > .toolbarbutton-icon,
  :-moz-any(toolbar, .widget-overflow-list) #loop-button > :-moz-any(.toolbarbutton-menubutton-button, .toolbarbutton-badge-stack) > .toolbarbutton-icon {
    max-width: 18px;
    margin: 0;
  }

  #loop-button {
    list-style-image: url(chrome://loop/skin/toolbar.png);
    -moz-image-region: rect(1px, 17px, 17px, 1px);
  }

This is why you're having issues. You're saying the image you want should be 16 by 16 pixels, but the icon should be sized to 18px.

The reason this is only showing up on retina is that the image on retina is not going to be 16 by 16 pixels but 32 by 32 pixels, and so that will get squashed into 18px because of the max width. On non-retina the intrinsic size of the image (16x16px) is being used.

I expect bug 1253973 is effectively the same problem.

Because bug 1246316 is also happening, it seems you can make 1 of 2 decisions...

1) use 16px and redo the inverted icons so they fit within a 16x16 (or 32x32 on hidpi) bounding box. Fix the CSS to set the width of the icon and its padding/margin so the icon gets sized to 16x16 pixels, but there's enough padding/margin on the border box so that the button is the same size as other buttons on both hidpi and lodpi
2) use 18px, basically revert bug 1233701 and instead fix that by forcing the width of the icon and its padding/margin appropriately, also ensuring that the button is the same size as other buttons on both hidpi and lodpi.


This is mostly just complicated because there's assets that make 1 thing easier, and Firefox is set up to make another thing easier (16x16 icons are, after all, more standard than 18x18px ones).

Comment 4

[Mark Banner (:standard8)]

Dave: I think we should go with option 1. For this bug it looks like we can just fix the max-width.

For bug 1246316 we'll need new icons, so I've requested them there.

Comment 5

[GitHub Autolander Bot]

Attachment: [loop] daveccrit:1257672-helloToolbarIcon > mozilla:master

Comment 6

[David Critchley (:dcritch)]

Comment on attachment 8736027 [details] [review]
[loop] daveccrit:1257672-helloToolbarIcon > mozilla:master

Checked inversed and normal icons on Mac and windows 10, both looked good

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8736027 [details] [review]
[loop] daveccrit:1257672-helloToolbarIcon > mozilla:master

As far as I can tell this is good on all counts, and also fixes bug 1246316. So r=me.