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

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Theme
P1
normal
RESOLVED FIXED
a month ago
26 days ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

unspecified
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [reserve-photon-animation])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

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.
See Also: → bug 1381993
Whiteboard: [photon-animation][triage]
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.
Flags: needinfo?(epang)

Updated

a month ago
Flags: qe-verify+
Priority: -- → P3
QA Contact: jwilliams
Whiteboard: [photon-animation][triage] → [reserve-photon-animation]
Created attachment 8888209 [details]
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.
Flags: needinfo?(epang) → needinfo?(jaws)
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.
Flags: needinfo?(jaws) → needinfo?(epang)

Comment 4

29 days ago
Created attachment 8888406 [details]
SVG + JSON.zip

Hey Jared, 

Here's the updated overflow icon animation. It extra space should be in there now.
Flags: needinfo?(epang)

Updated

29 days ago
Attachment #8888209 - Attachment is obsolete: true
Thanks!
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Updated

29 days ago
Iteration: --- → 56.3 - Jul 24
Priority: P3 → P1
Comment hidden (mozreview-request)

Comment 8

29 days ago
mozreview-review
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
Attachment #8888425 - Flags: review?(sfoster) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 10

29 days ago
mozreview-review-reply
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

29 days ago
Pushed by jwein@mozilla.com:
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
I had to back this out because it was blocking me from backing out bug 1355922.
Flags: needinfo?(jaws)

Comment 13

29 days ago
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/88bdf54bbfeb
Backed out changeset 320f9642bcef because it blocks backing out 1355922 a=backout
Flags: needinfo?(jaws)

Comment 14

28 days ago
Pushed by jwein@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/0ca1d6af381a
Status: ASSIGNED → RESOLVED
Last Resolved: 27 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1383689
You need to log in before you can comment on or make changes to this bug.