1381991 - Pin to Overflow animation is placed in the wrong position when using a non-default font-size

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

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

STR:
Open the Browser Toolbox
Inspect the <window>
Set the font-size on the window to font-size:1.4em;
Pin an item to the overflow using the context menu

Expected result:
The overflow animation should run, vertically centered and aligned with the other toolbar icons

Actual result:
The overflow animation is shifted down and not in line with the other toolbar icons.

Comment 1

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

Hey Eric, can you provide a version of the Pin to Overflow animation that has equal blank space above and below the icon? We will need that to fix this bug.

Comment 2

[Eric Pang [:epang] UX]

Attachment: SVG + JSON.zip

Hey Jared, I updated the icon so there's equal space above and below the icon's movement area. Let me know if you need anything else.

Comment 3

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

Hey Eric, I downloaded the attachment but the icon is still only 26px tall and there doesn't appear to be any extra blank space above the icon.

Comment 4

[Amy Lee [:amylee] UX]

Attachment: SVG + JSON.zip

Hey Jared, 

Here's the updated overflow icon animation. It extra space should be in there now.

Comment 5

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

Thanks!

Comment 6

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

Attachment: Bug 1381991 - Pin to Overflow animation is placed in the wrong position when using a non-default font-size.

Review commit: https://reviewboard.mozilla.org/r/159364/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159364/

Comment 7

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

Comment on attachment 8888425 [details]
Bug 1381991 - Pin to Overflow animation is placed in the wrong position when using a non-default font-size.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159364/diff/1-2/

Comment 8

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8888425 [details]
Bug 1381991 - Pin to Overflow animation is placed in the wrong position when using a non-default font-size.

https://reviewboard.mozilla.org/r/159364/#review165012

I confess I'm lukewarm on this approach, but as it works I'm in favor of landing it and refining later if a better solution emerges. Sorry my head is broken by this time of night so I don't have any constructive alternative to offer.

::: browser/themes/shared/toolbarbutton-icons.inc.css:365
(Diff revision 2)
>  }
>  
>  #nav-bar-overflow-button > .toolbarbutton-animatable-box {
> -  position: fixed;
> +  position: absolute;
>    overflow: hidden;
> -  /* The height of the sprite is 24px, which is 8px taller than
> +  top: calc(50% - 18px); /* Vertically center the 36px tall animatable image */

Just a comment: there's really no xul flex-box way of doing this? All this top: 50% - 1/2 height vertical alignment stuff was why we came up with flexbox in the first place.

::: browser/themes/shared/toolbarbutton-icons.inc.css:374
(Diff revision 2)
>    width: 18px; /* Width of each frame within the SVG sprite */
> -  height: 24px; /* Height of each frame within the SVG sprite */
> +  height: 36px; /* Height of each frame within the SVG sprite */
>  }
>  
>  #nav-bar-overflow-button > .toolbarbutton-animatable-box > .toolbarbutton-animatable-image {
> -  height: 24px; /* Height of each frame within the SVG sprite */
> +  height: var(--toolbarbutton-height); /* Height must be equal to height of toolbarbutton padding-box */

As you wont find --toolbarbutton-height in any stylesheet, lets add a comment pointing out it is defined at runtime by setToolbarButtonHeightProperty

Comment 9

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

Comment on attachment 8888425 [details]
Bug 1381991 - Pin to Overflow animation is placed in the wrong position when using a non-default font-size.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159364/diff/2-3/

Comment 10

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

Comment on attachment 8888425 [details]
Bug 1381991 - Pin to Overflow animation is placed in the wrong position when using a non-default font-size.

https://reviewboard.mozilla.org/r/159364/#review165012

> Just a comment: there's really no xul flex-box way of doing this? All this top: 50% - 1/2 height vertical alignment stuff was why we came up with flexbox in the first place.

There isn't a way using flex-box to take an element out of document flow and place it on top of another element. position:fixed or position:absolute are our only two ways, and absolute works best for what we want.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/320f9642bcef
Pin to Overflow animation is placed in the wrong position when using a non-default font-size. r=sfoster

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out because it was blocking me from backing out bug 1355922.

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88bdf54bbfeb
Backed out changeset 320f9642bcef because it blocks backing out 1355922 a=backout

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ca1d6af381a
Pin to Overflow animation is placed in the wrong position when using a non-default font-size. r=sfoster

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0ca1d6af381a

Comment 16

[Stefan [:StefanG_QA]]

I was able to reproduce this issue on Nightly build 56.0a1 from 20170720030203.

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 (20170925150345)

This issue is verified as fixed with the latest 57.0b3 build on 9/26/2017.