Closed Bug 1394588 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Theme, defect, P1, major)

57 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: Eddwardiq, Assigned: jaws)

References

Details

(Keywords: regression, Whiteboard: [reserve-photon-animation])

Attachments

(3 files)

Attached image 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.
Blocks: 1384953
Severity: normal → major
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Theme
Keywords: regression
Whiteboard: [photon-animation] [triage]
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Flags: qe-verify+
Priority: -- → P3
QA Contact: stefan.georgiev
Whiteboard: [photon-animation] [triage] → [reserve-photon-animation]
(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)
(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?
(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?
(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
Attached image max vs window.png
(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: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 57.3 - Sep 19
Priority: P3 → P1
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)
Duplicate of this bug: 1394675
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 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 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+
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
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.