1233701 - Hello button has increased height (so navigation toolbar is 42px in height instead of 40px)

Status: VERIFIED FIXED

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

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20151217030207
STR:
1. Create new profile, launch it
2. Right-click Hello button on navigation toolbar, click "Move to menu"

Result:       Navigation toolbar height reduced by 2 pixels
Expectations: It should always have height == 40px (content-box)

Regressed by bug 1223573
> pushlog:   https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=f48352b311eb120a09347efce60ae9045401858c&tochange=19876a153a009b457900c82f27efbc398ad19413

Comment 1

[arni2033]

I'm just VERY nervous about that insanely large toolbar. I mean, "isn't it thick enough already?!"
I was close to think that it's made intentionally! Dear G-d... Firefox has been completely regressed. Even on devtools theme navigation toolbox is several pixels thicker than one in GoogleChrome

Comment 2

[David Critchley (:dcritch)]

I believe I found the culprit here, some css, in the browser.css for windows, that was fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=1179295 added this:
#nav-bar .toolbarbutton-1:not(:-moz-any(@primaryToolbarButtons@)) > .toolbarbutton-icon,
#nav-bar .toolbarbutton-1:not(:-moz-any(@primaryToolbarButtons@)) > .toolbarbutton-badge-stack,
#nav-bar .toolbarbutton-1:not(:-moz-any(@primaryToolbarButtons@)) > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
  padding: calc(var(--toolbarbutton-vertical-inner-padding) + 1px) 7px;
}
The "+ 1px" padding is adding to the height of the hello button, whereas, other buttons in the toolbar only have "padding: calc(var(--toolbarbutton-vertical-inner-padding)) 7px;"

Should we fix this here or create an overriding class to remove the "+ 1px"?

Comment 3

[Mark Banner (:standard8)]

Yes, I think removing the 1px is reasonable.

Comment 4

[:Gijs (he/him)]

This is your button being treated as a non-default button. Non-default buttons being treated differently is correct. Please don't remove the "+ 1px". If you want to continue to use an 18x18px icon for Hello (not necessary, I believe, as the actual non-transparent content fit in a 16x16px box, last I checked), you should override the rule just for Hello.

When fixing this, please check what's happening on non-Windows, too.

Comment 5

[David Critchley (:dcritch)]

That appears to be what needs to be done. Since Hello was removed from the default toolbar list, the icons need to be resized down to 16x16 from 18x18.
Michael, can we resize these images down?
If not, I can fall back to overriding the CSS for the Loop icon.

Comment 6

[:Gijs (he/him)]

(In reply to David Critchley (:dcritch) from comment #5)
> That appears to be what needs to be done. Since Hello was removed from the
> default toolbar list, the icons need to be resized down to 16x16 from 18x18.
> Michael, can we resize these images down?
> If not, I can fall back to overriding the CSS for the Loop icon.

FWIW, I think in the existing spritesheet, a 16x16 box can capture the entirety of the icon as it is. The outside 1px all around it (making a 2px size difference in both directions) is transparent. So technically, you don't need a new sprite, you would just need to update the rect/region we use from the icon. So instead of:

    -moz-image-region: rect(0, 18px, 18px, 0);

from: https://dxr.mozilla.org/mozilla-central/rev/9d6ffc7a08b6b47056eefe1e652710a3849adbf7/browser/extensions/loop/skin/shared/loop.css#21

you want:

    -moz-image-region: rect(1px, 17px, 17px, 1px);

Of course, it's probably neater to update the raw image files, so you can use 0,16,16,0 instead -- but at least the icons don't need actual resizing.

Comment 7

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

Hey David, can you use the steps that Gijs has provided in comment #6 to land a fix for this without new icons? We shouldn't be shipping this to users in this state, and we are getting close to this merging in to Beta at which point it will be harder to fix.

Comment 8

[David Critchley (:dcritch)]

Will hopefully have this fixed within the next couple days.

Comment 9

[GitHub Autolander Bot]

Attachment: [loop] daveccrit:1233701-HelloIcon > mozilla:master

Comment 10

[David Critchley (:dcritch)]

Attachment: Link to Github pull-request: https://github.com/mozilla/loop/pull/93

Comment 11

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

(In reply to :Gijs Kruitbosch from comment #4)
> When fixing this, please check what's happening on non-Windows, too.

Thanks for the quick patch David. Were you able to test on non-Windows?

Comment 12

[David Critchley (:dcritch)]

Yes, I tested both on a Windows box and my Mac, also Chris Rafuse tested on Linux for me.

Comment 13

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

Thanks!

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8709763 [details] [review]
Link to Github pull-request: https://github.com/mozilla/loop/pull/93

I've done a few extra checks on Mac/Windows and it looks good. r=Standard8

Comment 16

[:Gijs (he/him)]

Mark, is this on track to make Firefox 45 ?

Comment 17

[Mark Banner (:standard8)]

(In reply to :Gijs Kruitbosch from comment #16)
> Mark, is this on track to make Firefox 45 ?

This is already fixed in development versions of the add-on. We're preparing to land an updated version in trees today, and get it uplifted soon after.

Comment 18

[Mark Banner (:standard8)]

Moving the icons request over to bug 1257672

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Mark was this already fixed in 46?

Comment 20

[Mark Banner (:standard8)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #19)
> Mark was this already fixed in 46?

No, we're going to be shipping it in the 1.3 update that we're planning to release just after 46.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Tracking for 46.  we'll mark it fixed for 46 once Hello 1.3 ships.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

This is rolling out to release now, marking fixed.