Closed Bug 1039979 Opened 5 years ago Closed 5 years ago

On touchstart animations do not abort until content listeners respond

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(8 files, 1 obsolete file)

11.83 KB, text/html
Details
23.43 KB, text/html
Details
1.59 KB, patch
botond
: review+
Details | Diff | Splinter Review
6.97 KB, patch
botond
: review+
Details | Diff | Splinter Review
12.19 KB, patch
botond
: review+
Details | Diff | Splinter Review
3.09 KB, patch
botond
: review+
Details | Diff | Splinter Review
5.72 KB, patch
botond
: review+
Details | Diff | Splinter Review
14.93 KB, patch
botond
: review+
Details | Diff | Splinter Review
Attached file 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
Attached file 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).
Attachment #8457866 - Attachment is obsolete: true
Attachment #8457866 - Flags: review?(botond)
Attachment #8457868 - Flags: review?(botond)
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.
Attachment #8457904 - Flags: review?(botond)
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.
Attachment #8457943 - Flags: review?(botond)
Attachment #8457865 - Flags: review?(botond) → review+
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.
Attachment #8457868 - Flags: review?(botond) → review+
Attachment #8457904 - Flags: review?(botond) → review+
Attachment #8457905 - Flags: review?(botond) → review+
Attachment #8457940 - Flags: review?(botond) → review+
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" :-)).
Attachment #8457943 - Flags: review?(botond) → review+
(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.
Blocks: 1040226
See Also: → 1085404
You need to log in before you can comment on or make changes to this bug.