1039979 - On touchstart animations do not abort until content listeners respond

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Attachment: Test page

Load the attached page in the B2G browser. Fling the page and then while it is in kinetic scroll, put your finger down.

Expected results:
- The page stops moving immediately when the finger goes down

Actual results:
- The page continues moving for 300ms before it stops

Comment 1

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

Attachment: Test page #2

With this second case things are even worse. If you fling the top-level page and then put your finger down on a subframe the top-level page just keeps going. (See instructions at the top of the test page for precise STR).

Comment 2

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

Attachment: Part 1 - Fix the basic test case

Comment 3

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

Attachment: Part 2 - Fix the second test case

Comment 4

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

Attachment: Part 2 - Fix the second test case

Comment 5

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

Attachment: Part 1.5 - Fix a bunch of new gtest warnings

Because we now want the overscroll handoff chain anytime we get a touchstart, we need it in ApzcTap, ApzcPinchWithTouchInput, ApzcDoubleTap, etc. I figured it was easier to just move it into the tester setup/teardown. If we don't have the handoff chain the NS_WARNING from part 2 gets printed.

Comment 6

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

Attachment: Part 3 - New gtests to exercise part 1

Comment 7

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

Attachment: Part 4 - Some more gtest cleanup/robustification

Comment 8

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

Attachment: Part 5 - Move the stop-fast-fling code to deal with above changes

I think this is a cleaner solution to the stop-fast-fling problem (bug 1022956) since it doesn't reach backwards into the GEL to throw away an already-processed touchstart.

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8457868 [details] [diff] [review]
Part 2 - Fix the second test case

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +905,5 @@
> +{
> +  MonitorAutoLock lock(mTreeLock);  // to access mOverscrollHandoffChain
> +  if (mOverscrollHandoffChain.length() == 0) {
> +    return false;
> +  }

We could omit this 'if' block and return 'mOverscrollHandoffChain.length() > 0' instead.

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8457943 [details] [diff] [review]
Part 5 - Move the stop-fast-fling code to deal with above changes

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +837,5 @@
>    TouchBlockState* block = nullptr;
>    if (aEvent.AsMultiTouchInput().mType == MultiTouchInput::MULTITOUCH_START) {
>      block = StartNewTouchBlock(false);
>      APZC_LOG("%p started new touch block %p\n", this, block);
> +    if (block == CurrentTouchBlock()) {

Please add a comment explaining this condition. (For reference, on IRC you said "so that future events don't interfere with animations from a previous block and cause some quantum wormhole" :-)).

Comment 12

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

(In reply to Botond Ballo [:botond] from comment #10)
> 
> We could omit this 'if' block and return 'mOverscrollHandoffChain.length() >
> 0' instead.

Good point. I'll do that, and update the corresponding flush-repaint code that I copied as well :)

(In reply to Botond Ballo [:botond] from comment #11)
> Please add a comment explaining this condition. (For reference, on IRC you
> said "so that future events don't interfere with animations from a previous
> block and cause some quantum wormhole" :-)).

Will do.

Also I did some try pushes with these patches:
https://tbpl.mozilla.org/?tree=Try&rev=4ad71eff0b94
https://tbpl.mozilla.org/?tree=Try&rev=3c760d184c0d

looks like we're getting a lot of infra errors right now but things that are running look green.

Comment 13

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/71c26f7faa40
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/46964bbe3c3f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7d3bd5fccafd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/086886096791
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/74cd20833f9d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1709c5fdd182

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/71c26f7faa40
https://hg.mozilla.org/mozilla-central/rev/46964bbe3c3f
https://hg.mozilla.org/mozilla-central/rev/7d3bd5fccafd
https://hg.mozilla.org/mozilla-central/rev/086886096791
https://hg.mozilla.org/mozilla-central/rev/74cd20833f9d
https://hg.mozilla.org/mozilla-central/rev/1709c5fdd182