1403550 - Library button is out of position when maximizing window during animation

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Edo Karabas]

Attachment: Library animation bug.png

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

Steps to reproduce:

1. Open Firefox in the windowed mode
2. Make sure you have a Library button on the toolbar
3. Open some webpage and hit "Star" button 
4. Click Done and right after that maximize window
5. See that Library button will stay in its position (in this case in URL bar) until animation is finished.

Severity of this bug depends on the point of view. In reality it is probably edge case, but if it is fixable then should be targetting Firefox 57.

I'm attaching also a screenshot when Library button is showed during animation right after I maximized window.
If users will hit this bug, it will definitely not look very nice.

Comment 1

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

[Tracking Requested - why for this release]: noticeable visual glitch while resizing the window after a bookmark or pocket of a page.

To fix this we could probably do something onresize of the window.

Comment 2

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

Attachment: Bug 1403550 - Update the X offset of the library button when the window is resized.

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

Comment 3

[Sam Foster [:sfoster] (pto until Nov 3) (he/him)]

Comment on attachment 8912784 [details]
Bug 1403550 - Update the X offset of the library button when the window is resized.

https://reviewboard.mozilla.org/r/184100/#review189410

::: browser/base/content/browser-places.js:1337
(Diff revision 1)
>      }
>    },
> +
> +  _windowResizeRunning: false,
> +  _onWindowResize(aEvent) {
> +    if (LibraryUI._windowResizeRunning) {

Don't we expect multiple resize events? If we ignore all but the first, is there a risk we'll still end up with the animation playing at the wrong position? 

I've not seen this in practice, so perhaps its no a concern.

::: browser/base/content/browser-places.js:1344
(Diff revision 1)
> +    }
> +    LibraryUI._windowResizeRunning = true;
> +
> +    requestAnimationFrame(() => {
> +      let libraryButton = document.getElementById("library-button");
> +      if (!libraryButton ||

Nit: a comment here explaining what these conditions mean would be good - why can we early return here?

Comment 4

[Sam Foster [:sfoster] (pto until Nov 3) (he/him)]

Comment on attachment 8912784 [details]
Bug 1403550 - Update the X offset of the library button when the window is resized.

https://reviewboard.mozilla.org/r/184100/#review189410

Looks good in general, r+ with nits.

Comment 5

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

Comment on attachment 8912784 [details]
Bug 1403550 - Update the X offset of the library button when the window is resized.

https://reviewboard.mozilla.org/r/184100/#review189410

> Don't we expect multiple resize events? If we ignore all but the first, is there a risk we'll still end up with the animation playing at the wrong position? 
> 
> I've not seen this in practice, so perhaps its no a concern.

Yes, in fact we receive *many* resize events. So this code uses requestAnimationFrame to coalesce the resize events. If multiple resize events come in before a rAF call, we will only act on it once. After rAF has finished, then we will reopen the ability to update the position for future resize events.

Comment 6

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

Comment on attachment 8912784 [details]
Bug 1403550 - Update the X offset of the library button when the window is resized.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6a7f900b908
Update the X offset of the library button when the window is resized. r=sfoster

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/d6a7f900b908

Comment 9

[Edo Karabas]

Thanks for a quick fix Jared. Looks good. I can no longer reproduce the issue using latest m-c build.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jared, could you please nominate a patch for uplift to Beta57? Thanks

Comment 11

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

Comment on attachment 8912784 [details]
Bug 1403550 - Update the X offset of the library button when the window is resized.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1384953 
[User impact if declined]: resizing the window after making a bookmark or pocketing can cause the animation to get out of place
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: updates our manual positioning code during resize, easy to disable if needed and only affects this one button.
[String changes made/needed]: none

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8912784 [details]
Bug 1403550 - Update the X offset of the library button when the window is resized.

Polish photon, taking it.
Should be in 57b5

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f8ac35ccd700

Comment 14

[Stefan [:StefanG_QA]]

I was able to reproduce issue in Firefox 57.0b4 and Nightly build (20170926100259).

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

The issue is not reproducible with Firefox 57.0b5 on Windows 10 x64, Ubuntu 17.04 x64 and Mac OS X 10.12.

The issue is not reproducible with Latest Nightly build (20171003100226) on Windows 10 x64, Ubuntu 17.04 x64 and Mac OS X 10.12.