Closed Bug 1464568 Opened 2 years ago Closed 2 years ago

Animations are mispositioned during scrolling

Categories

(Core :: DOM: Animation, defect, P1)

61 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 + verified
firefox62 + verified

People

(Reporter: kats, Assigned: hiro)

References

Details

(Keywords: regression)

Attachments

(10 files)

7.72 MB, video/quicktime
Details
24.19 KB, text/plain
Details
393 bytes, text/html
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
froydnj
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
7.58 KB, patch
Details | Diff | Splinter Review
5.54 KB, patch
Details | Diff | Splinter Review
On Firefox 61.0b8, macOS 10.13.4.
- Open a MozReview page such as https://reviewboard.mozilla.org/r/246576/diff/1#index_header
- While the review is loading, the list of files (just below "Diff Revision 1") has a little spinner to the left of the filename. Observe the spinner while scrolling up and down

Expected: spinner stays next to the filename as you scroll
Actual: spinner jumps all over the place

I'm bisecting now, will post more details soon.
Attached video jumping-spinner.mov
Here's a video demonstrating the problem (taken on the May 25 nightly with a clean profile). Note that I often had to reload the page a few times in order to catch a length of time with the spinners present. I didn't try making a reduced version of the page.

I ran mozregression twice and both times I got the same regression range: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b63e4bff951e97ce23e5673cb55a990a1bbb1a3c&tochange=eb34acceb9dcba6faff9eee5e8c9183755ddd37a

Which doesn't really seem to make sense, but that's why I ran it twice.
Karl, any idea how your patches might have caused this?
Blocks: 1455210
Has Regression Range: --- → yes
Component: Graphics → DOM: Workers
Flags: needinfo?(karlt)
Keywords: regression
Version: 62 Branch → 61 Branch
So I'm clear, this happens with a default config? No WR enabled or anything?
Flags: needinfo?(bugmail)
Correct
Flags: needinfo?(bugmail)
For the reasons given in
https://bugzilla.mozilla.org/show_bug.cgi?id=1458665#c2
I can't imagine how either of those changes would cause this.

Regression ranges in bug 1458665 were not certain but they did come down to
the same changesets (or nearby).

I also get a similar regression range (on Linux) when I use mozregression, but
mozregression never ran a build for b63e4bff951e.

It seems that mozregression is doing something odd when switching from
mozilla-central to autoland.

I reproduce the symptoms of this bug with
~/.local/bin/mozregression --launch b63e4bff951e
which uses
https://queue.taskcluster.net/v1/task/JRnJuCnIRoenuepHf6qkFQ/runs/0/artifacts/public%2Fbuild%2Ftarget.tar.bz2
for b63e4bff951e

https://reviewboard.mozilla.org/r/24415/diff/13/
reproduces often.

~/.local/bin/mozregression --launch dc845b3a8cbe
does not reproduce.

That leads to the much more likely looking regression range of

https://hg.mozilla.org/integration/autoland/log?rev=ancestors(b63e4bff951e)-ancestors(dc845b3a8cbe)
Blocks: 1454324
No longer blocks: 1455210
Flags: needinfo?(karlt) → needinfo?(hikezoe)
See Also: → 1458665
Attached file Simple test case
Thank you, kats and karlt.

So on non-WebRender, we need to apply unchanged animated values in any way?  Anyway I will take this.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe)
Component: DOM: Workers → DOM: Animation
Depends on: 1464937
I was wondering why this problem doesn't happen on my local build.  That's because I did set layout.scroll.root-frame-containers true there.
It's possible to simulate APZ behaviour in mochitests by using setAsyncScrollOffset [1], if that helps you write a test for it. Maybe create a page with an animation, do a frame of APZ scroll, and then read back the shadow transform via DWU.getOMTAStyle?

[1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/dom/interfaces/base/nsIDOMWindowUtils.idl#1405
Thanks kats for the info!  getOMTAStyle() returns only animation style, so it won't be used as it is, but now I think I can write a test case with setAsyncScrollOffset and another function returning the transform value regardless of whether the value is composed by animation or not.
The test failed on TV. :/  The reason is that the test sometimes proceeded before the transform value by APZC applied.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ee923cccef9926ff00f56b55620dc7984e34ee2

This try should be green. (I hope)
It turns out that we have to set the transform value in case of opacity animations too.
I guess we ideally have separate variables for transform by animation and transform by APZC. But for now I am going to go with the current setup.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bb893c9cedec3d8f6b8edf5aa5f8ec42dc727a8
Comment on attachment 8982929 [details]
Bug 1464568 - Set the shadow base transform for animation explicitly even if the transform value hasn't changed.

https://reviewboard.mozilla.org/r/248758/#review254922

::: gfx/layers/composite/AsyncCompositionManager.cpp:747
(Diff revision 1)
> +                    // FIXME: Bug 1459775: It seems possible that we somehow try
> +                    // to sample animations and skip it even if the previous value
> +                    // has been discarded from the animation storage when we enable
> +                    // layer tree cache. So for the safety, in the case where we
> +                    // have no previous animation value, we set non-animating value
> +                    // instead.

I should note about bug 1459775.  The tab layer cache has been disabled (bug 1465106), so the assertion (bug 1459775) would no longer happen, but still I don't still quite understand how the assertion happens (see bug 1459775 comment 11), so for the safety I did add a fail-safe instead of crashing.
Comment on attachment 8982926 [details]
Bug 1464568 - Add an argument to ApplyAsyncProperties() to apply the transform by APZC or not.

https://reviewboard.mozilla.org/r/248752/#review254946

::: gfx/layers/ipc/CompositorBridgeParent.h:118
(Diff revision 1)
> +                                    TransformsToSkip aSkip =
> +                                      TransformsToSkip::NoneOfThem) = 0;

I would prefer to remove the default value here, since you're already explicitly specifying the arg at all the call sites of this function

::: gfx/layers/ipc/CompositorBridgeParent.h:246
(Diff revision 1)
> +                            TransformsToSkip aSkip =
> +                              TransformsToSkip::NoneOfThem) override;

Ditto, drop default value

::: gfx/layers/ipc/CrossProcessCompositorBridgeParent.h:92
(Diff revision 1)
> +                            TransformsToSkip aSkip =
> +                              TransformsToSkip::NoneOfThem) override;

Ditto, drop default value
Attachment #8982926 - Flags: review?(bugmail) → review+
Comment on attachment 8982927 [details]
Bug 1464568 - Add an IPC call to get transform value for a given element on the compositor.

https://reviewboard.mozilla.org/r/248754/#review254948

::: gfx/layers/ipc/LayerTransactionParent.cpp:759
(Diff revision 1)
> +  Layer* layer = AsLayer(aLayerHandle);
> +  if (!layer) {
> +    return IPC_FAIL_NO_REASON(this);
> +  }
> +
> +  mCompositorBridge->ApplyAsyncProperties(this);

I guess you'll have to add the NoneOfThem arg here if you make the changes I requested in part 1

::: gfx/layers/ipc/LayerTransactionParent.cpp:772
(Diff revision 1)
> +                        1.0f);
> +  }
> +  float scale = 1;
> +  Point3D scaledOrigin;
> +  Point3D transformOrigin;
> +  for (uint32_t i=0; i < layer->GetAnimations().Length(); i++) {

nit: spaces around =
Attachment #8982927 - Flags: review?(bugmail) → review+
Comment on attachment 8982928 [details]
Bug 1464568 - Add a new function to get transform value on the compositor.

https://reviewboard.mozilla.org/r/248756/#review254952

::: dom/base/nsDOMWindowUtils.cpp:3643
(Diff revision 1)
>    *aRetVal = aTarget->
>      DispatchEvent(*aEvent, CallerType::System, IgnoreErrors());
>    return NS_OK;
>  }
>  
> -NS_IMETHODIMP
> +static Result<nsIFrame*, nsresult>

nice! First time I'm interacting with this C++ Result class, it's pretty handy here :)
Attachment #8982928 - Flags: review?(bugmail) → review+
Comment on attachment 8982929 [details]
Bug 1464568 - Set the shadow base transform for animation explicitly even if the transform value hasn't changed.

https://reviewboard.mozilla.org/r/248758/#review254954

Minusing for the add_task changes. Sorry to be picky about that but I spend a lot of time debugging APZ mochitest failures and really don't want to introduce more complexity in that regard.

::: commit-message-d597c:8
(Diff revision 1)
> +That's because the shadow base transform value might have been changed by APZC.
> +
> +The test case in this patch fail without this fix on non-WebRender and the test
> +case is skipped on WebRender since the bug should never happen on WebRender
> +because WebRender manages animation transform value and APZ transform value
> +serapately.

typo: separately

::: gfx/layers/apz/test/mochitest/helper_bug1464568_transform.html:4
(Diff revision 1)
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test that transform animation is correctly placed during asynchronos scrolling</title>

typo: asynchronous

::: gfx/layers/apz/test/mochitest/helper_bug1464568_transform.html:34
(Diff revision 1)
> +const SimpleTest = opener.SimpleTest;
> +const ok = opener.ok.bind(opener);
> +const is = opener.is.bind(opener);

You shouldn't need to define `SimpleTest`, `ok`, or `is` here. They are already exported into the subtest window by the runSubtestsSeriallyInFreshWindows at https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/gfx/layers/apz/test/mochitest/apz_test_utils.js#242-244

Note in particularly that the `ok` and `is` functions will include the name of the subtest that is running which is a really nice feature, and defining these here yourself will clobber that.

::: gfx/layers/apz/test/mochitest/helper_bug1464568_transform.html:40
(Diff revision 1)
> +const ok = opener.ok.bind(opener);
> +const is = opener.is.bind(opener);
> +const info = opener.info.bind(opener); // For AddTask.js
> +const utils = SpecialPowers.getDOMWindowUtils(window);
> +
> +SimpleTest.finish = subtestDone;

Because of this override of SimpleTest.finish I would prefer to avoid using the add_task machinery in APZ mochitests that use the runSubtestsSerially machinery. It doesn't seem like add_task provides a lot of value for cases like this where there's only one "task" and it introduces additional black-box complexity. Already we have enough trouble with APZ tests being pretty flaky and I don't want to have to go and debug subtle interactions between add_task machinery and runSubtestsSerially machinery.

Can you instead structure the test like so:

async function test_transform() {
   // your test code starting with utils.setDisplayPortForElement
}

if (utils.layerManagerType == 'WebRender') {
  ok(true, ...);
  subtestDone();
} else {
  waitUntilApzStable().then(test_transform).then(subtestDone);
}

You can then also drop the `info` definition and the AddTask.js import

::: gfx/layers/composite/AsyncCompositionManager.cpp:741
(Diff revision 1)
> +                if (layer->GetScrollMetadataCount() > 0) {
> +                  // In the case of transform we have to set the unchanged
> +                  // transform value again becasue APZC might have modified the
> +                  // previous shadow base transform value.

Do we need this check? APZ also modifies the shadow transform on scrollthumb layers, for which GetScrollMetadataCount() returns zero. I guess those layers would never have transform animations on them, but it still seems kind of brittle to have this check here.
Attachment #8982929 - Flags: review?(bugmail) → review-
Comment on attachment 8982930 [details]
Bug 1464568 - Set the shadow base transform value for the case where opacity animations' calculation was skipped.

https://reviewboard.mozilla.org/r/248760/#review254960

Same changes/questions here as in the previous patch: (1) please remove add_task stuff, (2) typo with asynchronous in the test title, (3) do we really need the GetScrollMetadataCount() check. Otherwise this looks fine.
Attachment #8982930 - Flags: review?(bugmail)
Thanks for the review!

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24)
> ::: gfx/layers/composite/AsyncCompositionManager.cpp:741
> (Diff revision 1)
> > +                if (layer->GetScrollMetadataCount() > 0) {
> > +                  // In the case of transform we have to set the unchanged
> > +                  // transform value again becasue APZC might have modified the
> > +                  // previous shadow base transform value.
> 
> Do we need this check? APZ also modifies the shadow transform on scrollthumb
> layers, for which GetScrollMetadataCount() returns zero. I guess those
> layers would never have transform animations on them, but it still seems
> kind of brittle to have this check here.

Phew, I didn't know that.  Moving scrollthumbs by APZC makes quite sense (I don't quite understand why its scroll metadata count is zero though), and it theoretically can have transform animations I believe.  That's fair.

I will drop the GetScrollMetadataCount() check there, and revise test code.
Thanks!
Comment on attachment 8982927 [details]
Bug 1464568 - Add an IPC call to get transform value for a given element on the compositor.

https://reviewboard.mozilla.org/r/248754/#review255034

r=me on the sync-messages.ini changes.
Attachment #8982927 - Flags: review?(nfroyd) → review+
Comment on attachment 8982926 [details]
Bug 1464568 - Add an argument to ApplyAsyncProperties() to apply the transform by APZC or not.

https://reviewboard.mozilla.org/r/248752/#review255050
Comment on attachment 8982929 [details]
Bug 1464568 - Set the shadow base transform for animation explicitly even if the transform value hasn't changed.

https://reviewboard.mozilla.org/r/248758/#review255052
Attachment #8982929 - Flags: review?(bugmail) → review+
Comment on attachment 8982930 [details]
Bug 1464568 - Set the shadow base transform value for the case where opacity animations' calculation was skipped.

https://reviewboard.mozilla.org/r/248760/#review255054

Thanks!
Attachment #8982930 - Flags: review?(bugmail) → review+
Priority: -- → P1
The test case failed on Android;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=151df58aa44347cc8d80f66470f217d5e904e254&selectedJob=181823866

It seem to me that setAsyncScrollOffset doesn't work on Android?
I am going to skip the test on Android for now.
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88bbaa441a37
Add an argument to ApplyAsyncProperties() to apply the transform by APZC or not.  r=kats
https://hg.mozilla.org/integration/autoland/rev/cf6231e7ef0c
Add an IPC call to get transform value for a given element on the compositor. r=froydnj,kats
https://hg.mozilla.org/integration/autoland/rev/a5e8c4b793e6
Add a new function to get transform value on the compositor. r=kats
https://hg.mozilla.org/integration/autoland/rev/b8c1542514b5
Set the shadow base transform for animation explicitly even if the transform value hasn't changed. r=kats
https://hg.mozilla.org/integration/autoland/rev/9ea461e4570b
Set the shadow base transform value for the case where opacity animations' calculation was skipped. r=kats
(In reply to Hiroyuki Ikezoe (:hiro) from comment #36)
> It seem to me that setAsyncScrollOffset doesn't work on Android?

That's odd. I thought it should work everywhere. If you file a follow-up bug for that I can look into it.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #44)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #36)
> > It seem to me that setAsyncScrollOffset doesn't work on Android?
> 
> That's odd. I thought it should work everywhere. If you file a follow-up bug
> for that I can look into it.

Filed bug 1466950 for that.
Comment on attachment 8982926 [details]
Bug 1464568 - Add an argument to ApplyAsyncProperties() to apply the transform by APZC or not.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1454324
[User impact if declined]: Opacity and transform animations with step functions are misplaced during scrolling
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes, though there are two automation test cases, but they are simplified test cases, so it would be nice that someone checks the original case.  To reproduce the problem, open the reviewboard link in comment 0 and scrolling up and down during small spinners are spinning.  (On my network environment, the spinners finish soon, so I can't see the original issue) 
[List of other uplifts needed for the feature/fix]: All patches in this bug
[Is the change risky?]: I don't think so
[Why is the change risky/not risky?]:  Though there are five patches here, but most of them are for the automation tests, so the fix itself is pretty simple, also I added a fail-safe in the fix, see comment 20 
[String changes made/needed]: None
Attachment #8982926 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Comment on attachment 8982926 [details]
Bug 1464568 - Add an argument to ApplyAsyncProperties() to apply the transform by APZC or not.

This one makes me a bit nervous given the size of the patches, but given that we've gotten multiple reports about this issue, it's a new regression in 61, and it includes automated test coverage, I'll approve for 61.0b12. Let's be vigilant for any new regression reports, though.
Attachment #8982926 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Part 4 needs a rebased patch due to conflicts with bug 1458457.
Flags: needinfo?(hikezoe)
Duplicate of this bug: 1458665
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180608100105

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180607135512

Verified as fixed on Nightly build 62.0a1 (2018-06-08) and Beta 61.0b12 on MacOS 10.13.5.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Thank you Cristian!
You need to log in before you can comment on or make changes to this bug.