1144112 - Input events stop flowing with a fast-motion block when touch action is enabled

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

As per bug 736048 comment 79. When we start a new touch block while an APZC is in the "fast motion" state, we consume that block in the APZ and return an nsEventStatus_eConsumeNoDefault back to the caller. That block is intended to be processed immediately inside the APZ. With touch-action disabled, this works fine. However with touch-action enabled, we never set the allowed touch behaviors on the block, and so it always returns false from IsReadyForHandling(), and never gets processed.

Our tests also don't really exercise this code well. I have patches to update the tests and fix the code. At some point we should probably also expand the content-response timeout to cover this, so that if we don't get the allowed touch-behaviour information within the timeout we process the block anyway. I'll leave that for a follow-up bug though.

Comment 1

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

Attachment: Part 1 - Make tests reflect reality a bit better

Comment 2

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

Attachment: Part 2 - Update FlingStop tests to exercise the bug

With these two patches in place and touch-action enabled, the FlingStop test fails because of the bug described in comment 0 because the block for the second tap just sits in the input queue behind the block for the first tap which doesn't have allowed behaviors.

Comment 3

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

Attachment: Part 3 - Code simplification

No functional changes here, just a little cleanup

Comment 4

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

Attachment: Part 4 - Fix

The fix for the bug

Comment 5

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

Attachment: Part 4 - fix

Slightly cleaner but equivalent fix.

One thing I would like to note is that if the CurrentTouchBlock() doesn't have any allowed touch behaviors set yet, and so haveBehavior ends up false, we don't go into the code that sets the fast-motion flag. However, in this scenario, StartNewTouchBlock() would not have been able to sweep away the CurrentTouchBlock() as depleted anyway (because it doesn't have allowed touch behaviours), so after calling StartNewTouchBlock(), block != CurrentBlock() and therefore we wouldn't have gone into the fast-motion code anyway.

Comment 6

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0397efd25225

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8578635 [details] [diff] [review]
Part 1 - Make tests reflect reality a bit better

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

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +439,5 @@
>  
>    aTime += TIME_BETWEEN_TOUCH_EVENT;
>  
>    // Allowed touch behaviours must be set after sending touch-start.
> +  if (status != nsEventStatus_eConsumeNoDefault) {

Unrelated, but in the description of the eConsumeNoDefault return value in APZCTreeManager.h:

   * nsEventStatus_eConsumeNoDefault is returned to indicate the
   *   caller should discard the event with extreme prejudice.
   *   Currently this is only returned if the APZ determines that something is
   *   in overscroll and the event should be ignored entirely, or if the input
   *   event is part of a extended gesture like flywheel scrolling, and gets
   *   consumed within the APZ code. There may be other scenarios where this
   *   return code might be used in the future.

could we remove the bit about overscroll, as it's no longer the case since bug 1042103?

Comment 8

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

Yeah. In fact I'd rather not specify any of the exact conditions under which it is returned; I don't see a need to expose that information as part of the API.

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8578642 [details] [diff] [review]
Part 4 - fix

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

::: gfx/layers/apz/src/InputQueue.cpp
@@ +82,5 @@
> +    if (!gfxPrefs::TouchActionEnabled()) {
> +      haveBehaviors = true;
> +    } else if (!mInputBlockQueue.IsEmpty() && CurrentBlock()->AsTouchBlock()) {
> +      haveBehaviors = CurrentTouchBlock()->GetAllowedTouchBehaviors(currentBehaviors);
> +    }

Might it make sense to move this computation into the block conditioned on HasFastMovingApzc() below (and make the condition on 'haveBehaviors' be a nested condition after that)? In addition to avoiding unnecessary work in the non-fast-moving scenario, I feel like this would make the code more readable, as we only have about haveBehaviors in the fast-moving case.

Comment 10

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #9)
> I feel like this would make the code more
> readable, as we only have about haveBehaviors in the fast-moving case.

s/have/care

Comment 11

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

It has to be above the StartNewTouchBlock call which will (in the cases we care about) throw away the CurrentTouchBlock().

Comment 12

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> It has to be above the StartNewTouchBlock call which will (in the cases we
> care about) throw away the CurrentTouchBlock().

Good point - fine as it is then.

Comment 13

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #0)
> At some point we should probably also
> expand the content-response timeout to cover this, so that if we don't get
> the allowed touch-behaviour information within the timeout we process the
> block anyway. I'll leave that for a follow-up bug though.

Could you file the follow-up so we don't forget? Thanks!

Comment 14

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

(In reply to Botond Ballo [:botond] from comment #13)
> Could you file the follow-up so we don't forget? Thanks!

Filed bug 1145084 for this.

Landed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ea078640b43e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4a4e2ae41f4e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/539b2af2e3ac
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2af030f451eb

Comment 15

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

(In reply to Botond Ballo [:botond] from comment #7)
> Unrelated, but in the description of the eConsumeNoDefault return value in
> APZCTreeManager.h:

Also filed bug 1145089 for this.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ea078640b43e
https://hg.mozilla.org/mozilla-central/rev/4a4e2ae41f4e
https://hg.mozilla.org/mozilla-central/rev/539b2af2e3ac
https://hg.mozilla.org/mozilla-central/rev/2af030f451eb