1053766 - Multiple TransformBegin/TransformEnd notifications issued by APZ during a single conceptual operation

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

According to the discussion in bug 1020801 comment 14, we fire two sets of TransformBegin/TransformEnd notifications from APZ in the case where we pan into overscroll and do a snapback. We should avoid doing this since conceptually it is a single user operation and there should be a single TransformBegin/TransformEnd that encompasses the whole thing.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

This probably happens when we switch from the fling to the snapback animation. As the fling animation terminates, it schedules the snabpack as a deferred task and returns false for the continueAnimation return value. This will result in the state getting to NOTHING temporarily at [1] before it is set to SNAP_BACK at [2]. This state glitch is likely what results in the TransformEnd/TransformBegin notifications.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=70b5978c6fb6#2124
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=70b5978c6fb6#1807

Comment 2

[Botond Ballo [:botond]]

A possible approach is to place Transform[Begin|End] notifications into a "queue" of size 1 (with a newer one replacing an older one), and flush the "queue" at the end of conceptual operations.

Looking at the code, flushing them at the end of the following functions should be sufficient:

  SampleContentTransformForFrame
  HandleInputEvent
  CancelAnimation
  Destroy
  ZoomToRect

Kats, does this approach sound reasonable?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

While reviewing this part of your patch on bug 1039992:

+      // Note:
+      //   This needs to be a deferred task even though it can safely run
+      //   while holding mMonitor, because otherwise, if the overscrolled APZC
+      //   is this one, then the SetState(NOTHING) in UpdateAnimation will
+      //   stomp on the SetState(SNAP_BACK) it does.
+      mDeferredTasks.append(NewRunnableMethod(mOverscrollHandoffChain.get(),
+                                              &OverscrollHandoffChain::SnapBackOverscrolledApzc));
       return false;

I was thinking it be simpler to just do what you said in the note (i.e. run while holding mMonitor instead of posting a deferred task) and then return true instead of returning false. The return true will indicate that there's still an animation going, and we won't flip into state NONE. Would that work?

Comment 4

[Botond Ballo [:botond]]

Bug 1046159 contains a similar case of going to the NOTHING state and then back into the transforming state.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I believe your latest patches for flinging will also introduce a NOTHING state after the pan but before the fling kicks in. (Because of how we need to find the scrollgrabbing APZC to start the fling).

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Introduce the state change notification blocker

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Use the state change notification blocker

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Add a test

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Try push https://tbpl.mozilla.org/?tree=Try&rev=d797b92aebb0

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8488290 [details] [diff] [review]
Part 1 - Introduce the state change notification blocker

Review of attachment 8488290 [details] [diff] [review]:
-----------------------------------------------------------------

Nice approach :) More concise than what I suggested in comment 2.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Try push has some build failures. These I'm definitely blaming on nested enum class thingy :p

Comment 12

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> Try push has some build failures. These I'm definitely blaming on nested
> enum class thingy :p

I'm not opposed to changing APZStateChange to be a regular enum if that makes things easier.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Made the change to a regular enum in bug 1066421. New try push is clean:

https://tbpl.mozilla.org/?tree=Try&rev=6768aa6af709

Landed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6f015a84b5e6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d05b3f7f2c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e208feba9fd1

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6f015a84b5e6
https://hg.mozilla.org/mozilla-central/rev/e0d05b3f7f2c
https://hg.mozilla.org/mozilla-central/rev/e208feba9fd1