Library icon animation is heavily out of position when adding a bookmark

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Theme
P1
major
VERIFIED FIXED
11 months ago
10 months ago

People

(Reporter: Eddward, Assigned: jaws)

Tracking

({regression})

57 Branch
Firefox 57
regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 fixed)

Details

(Whiteboard: [reserve-photon-animation])

MozReview Requests

()

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

Attachments

(3 attachments)

(Reporter)

Description

11 months ago
Created attachment 8902011 [details]
animation.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170828100127

Steps to reproduce:

1. put Library button on the toolbar (if not yet)
2. open random web page
3. hit star button and click done
4. see broken Library button animation - it shifts down and then up on the navigation toolbar
Looks like it started with https://hg.mozilla.org/integration/autoland/rev/b5a5c38a0add
So regressed by Bug 1384953.
I didn't test animation when saving to Pocket.
(Reporter)

Updated

11 months ago
Blocks: 1384953
Severity: normal → major
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Theme
Keywords: regression
Whiteboard: [photon-animation] [triage]

Updated

11 months ago
Status: UNCONFIRMED → NEW
status-firefox56: --- → unaffected
status-firefox57: --- → affected
Ever confirmed: true

Comment 1

11 months ago
As stated in http://forums.mozillazine.org/viewtopic.php?p=14763709#p14763709, on Windows 10 this happens on maximized window without title bar; otherwise the library button animation (i.e. after clicking "Done") is fine.

Updated

11 months ago
Flags: qe-verify+
Priority: -- → P3
QA Contact: stefan.georgiev
Whiteboard: [photon-animation] [triage] → [reserve-photon-animation]
(Reporter)

Comment 2

11 months ago
(In reply to Mauro Sanabria from comment #1)
> As stated in
> http://forums.mozillazine.org/viewtopic.php?p=14763709#p14763709, on Windows
> 10 this happens on maximized window without title bar; otherwise the library
> button animation (i.e. after clicking "Done") is fine.

I can not confirm this.
Animation works fine only on windowed mode with enabled title bar. In every other case it is broken.
The most important thing is that it is broken with default Windows 10 configuration.
In bug 1384953 we use the bounding client rect of the library button icon to position the animatable box for the library animation in the same place.

This works well for restored windows, but it is showing this bug in maximized windows.

The calculation for the bounding client rect is off by 6.5px in maximized windows, which matches the padding-top on #titlebar for maximized windows. I can't see where this 6.5px comes from, especially because there is no mention of `paddingTop` in http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/browser/base/content/browser-tabsintitlebar.js.

To fix this bug I *could* subtract the #titlebar's padding-top from the library icon's bounding client rect Y value, but I would like to understand this bug better. @Dao, do you know where this padding-top is coming from and what I can do to work-around-it or incorporate it into the calc()?
Flags: needinfo?(dao+bmo)

Comment 4

11 months ago
(In reply to Eddward from comment #2)
> (In reply to Mauro Sanabria from comment #1)
> > As stated in
> > http://forums.mozillazine.org/viewtopic.php?p=14763709#p14763709, on Windows
> > 10 this happens on maximized window without title bar; otherwise the library
> > button animation (i.e. after clicking "Done") is fine.
> 
> I can not confirm this.
> Animation works fine only on windowed mode with enabled title bar. In every
> other case it is broken.

Are you saying it's also off in windowed mode with the title bar disabled? Because that doesn't match comment #3... If so, can you add a screencast and/or more details about how that case is broken?

Comment 5

11 months ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> In bug 1384953 we use the bounding client rect of the library button icon to
> position the animatable box for the library animation in the same place.
> 
> This works well for restored windows, but it is showing this bug in
> maximized windows.
> 
> The calculation for the bounding client rect is off by 6.5px in maximized
> windows, which matches the padding-top on #titlebar for maximized windows. I
> can't see where this 6.5px comes from, especially because there is no
> mention of `paddingTop` in
> http://searchfox.org/mozilla-central/rev/
> cd82cacec2cf734768827ff85ba2dba90a534c5e/browser/base/content/browser-
> tabsintitlebar.js.
> 
> To fix this bug I *could* subtract the #titlebar's padding-top from the
> library icon's bounding client rect Y value, but I would like to understand
> this bug better. @Dao, do you know where this padding-top is coming from and
> what I can do to work-around-it or incorporate it into the calc()?

It's 6.8px for me, and AFAICT it's coming from the -moz-appearance setting, because there are no other rules that show any padding at all.

Rather than doing something specific, couldn't you get the lazy bounds of the element against which everything is being positioned (which I think is the navbar?) and subtract the two?
(Reporter)

Comment 6

11 months ago
(In reply to :Gijs (queue backed up, slow) from comment #4)
> (In reply to Eddward from comment #2)
> > (In reply to Mauro Sanabria from comment #1)
> > > As stated in
> > > http://forums.mozillazine.org/viewtopic.php?p=14763709#p14763709, on Windows
> > > 10 this happens on maximized window without title bar; otherwise the library
> > > button animation (i.e. after clicking "Done") is fine.
> > 
> > I can not confirm this.
> > Animation works fine only on windowed mode with enabled title bar. In every
> > other case it is broken.
> 
> Are you saying it's also off in windowed mode with the title bar disabled?
> Because that doesn't match comment #3... If so, can you add a screencast
> and/or more details about how that case is broken?

yes it is, looks like 2px for me,
and in maximized window it is 8px as far as I can see...
the important thing here is that I'm using compact theme mode... in normal mode it's fine in restored window as per above comments
(Reporter)

Comment 7

11 months ago
Created attachment 8903689 [details]
max vs window.png

Comment 8

11 months ago
(In reply to :Gijs (queue backed up, slow) from comment #5)
> > To fix this bug I *could* subtract the #titlebar's padding-top from the
> > library icon's bounding client rect Y value, but I would like to understand
> > this bug better. @Dao, do you know where this padding-top is coming from and
> > what I can do to work-around-it or incorporate it into the calc()?
> 
> It's 6.8px for me, and AFAICT it's coming from the -moz-appearance setting,
> because there are no other rules that show any padding at all.

Yep, it's coming from -moz-appearance.

Are you sure you found the right solution in bug 1384953? This whole setup feels fragile, and this bug seems worse than what bug 1384953 attempted to fix.
Flags: needinfo?(dao+bmo)
I'm not sure what else we can do if we want to fix bug 1384953. At least with what we know now I can adjust the calculation to take into effect the padding from the #titlebar. I'll update the patch here with that.
(Assignee)

Updated

11 months ago
Assignee: nobody → jaws
Status: NEW → ASSIGNED

Updated

11 months ago
Iteration: --- → 57.3 - Sep 19
Priority: P3 → P1
Comment hidden (mozreview-request)

Comment 11

11 months ago
mozreview-review
Comment on attachment 8904677 [details]
Bug 1394588 - Include the gNavToolbox offset in the calculation of the library drop animation.

https://reviewboard.mozilla.org/r/176482/#review181510

::: commit-message-0e73a:1
(Diff revision 1)
> +Bug 1394588 - Include the padding-top of the titlebar in the calcuation of the library drop animation. r?gijs

Nit: spelling.

::: browser/base/content/browser-places.js:1534
(Diff revision 1)
> -    let libraryIcon = document.getAnonymousElementByAttribute(libraryButton, "class", "toolbarbutton-icon");
> +      let libraryIcon = document.getAnonymousElementByAttribute(libraryButton, "class", "toolbarbutton-icon");
> +      let titlebar = document.getElementById("titlebar");
> -    let dwu = window.getInterface(Ci.nsIDOMWindowUtils);
> +      let dwu = window.getInterface(Ci.nsIDOMWindowUtils);
> -    let iconBounds = dwu.getBoundsWithoutFlushing(libraryIcon);
> +      let iconBounds = dwu.getBoundsWithoutFlushing(libraryIcon);
> -    let libraryBounds = dwu.getBoundsWithoutFlushing(libraryButton);
> +      let libraryBounds = dwu.getBoundsWithoutFlushing(libraryButton);
> +      let titlebarPaddingTop = window.getComputedStyle(titlebar).paddingTop;

I'm probably missing something, but why couldn't we use the bounds of gNavToolbox, also getting that with dwu? That would also help if other titlebar properties ever get in the way here. Or does the `promiseLayoutFlushed` stuff here fix something else?

Making this async, I wonder if we need to check `window.closed` before doing all the actual work in the `requestAnimationFrame` callback.
Attachment #8904677 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1394675
(Assignee)

Comment 13

11 months ago
mozreview-review-reply
Comment on attachment 8904677 [details]
Bug 1394588 - Include the gNavToolbox offset in the calculation of the library drop animation.

https://reviewboard.mozilla.org/r/176482/#review181510

> I'm probably missing something, but why couldn't we use the bounds of gNavToolbox, also getting that with dwu? That would also help if other titlebar properties ever get in the way here. Or does the `promiseLayoutFlushed` stuff here fix something else?
> 
> Making this async, I wonder if we need to check `window.closed` before doing all the actual work in the `requestAnimationFrame` callback.

We can't use the bounds of gNavToolbox because gNavToolbox includes all of the toolbars. We would have to base our animation off of the bottom of gNavToolbox, and then we would have to calculate the height of the bookmarks toolbar and change the way we calculate the positioning. I'm not sure if it would have its own issues.

The promiseLayoutFlushed is only here to prevent a sync style flush when calling getComputedStyle. As far as other properties, I agree that this approach isn't perfect but I didn't want to add in extra calculations for paddingBottom, height, etc if they aren't necessary.

Good call about window.closed, I'll add that in.
Comment hidden (mozreview-request)

Comment 15

11 months ago
mozreview-review
Comment on attachment 8904677 [details]
Bug 1394588 - Include the gNavToolbox offset in the calculation of the library drop animation.

https://reviewboard.mozilla.org/r/176482/#review182398

Clearing this per discussion on IRC. :-)
Attachment #8904677 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)

Comment 17

11 months ago
mozreview-review
Comment on attachment 8904677 [details]
Bug 1394588 - Include the gNavToolbox offset in the calculation of the library drop animation.

https://reviewboard.mozilla.org/r/176482/#review182546

Nice and simple. Thanks!
Attachment #8904677 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 18

11 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1dca70dfe391
Include the gNavToolbox offset in the calculation of the library drop animation. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/1dca70dfe391
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Comment 20

10 months ago
I have reproduced this issue with the Nightly build: 20170905100117 

User Agent    Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build   ID    20170908100218

This bug fix is Verified with latest Nightly 57.0a1. (20170908100218)
Status: RESOLVED → VERIFIED

Updated

10 months ago
Flags: qe-verify+
status-firefox55: --- → unaffected
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.