If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 39

Status

()

Core
Panning and Zooming
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
mozilla39
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

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.
Assignee: nobody → bugmail.mozilla
Created attachment 8578635 [details] [diff] [review]
Part 1 - Make tests reflect reality a bit better
Attachment #8578635 - Flags: review?(botond)
Created attachment 8578636 [details] [diff] [review]
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.
Attachment #8578636 - Flags: review?(botond)
Created attachment 8578637 [details] [diff] [review]
Part 3 - Code simplification

No functional changes here, just a little cleanup
Attachment #8578637 - Flags: review?(botond)
Created attachment 8578638 [details] [diff] [review]
Part 4 - Fix

The fix for the bug
Attachment #8578638 - Flags: review?(botond)
Created attachment 8578642 [details] [diff] [review]
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.
Attachment #8578638 - Attachment is obsolete: true
Attachment #8578638 - Flags: review?(botond)
Attachment #8578642 - Flags: review?(botond)
(Assignee)

Comment 6

3 years ago
greentry
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0397efd25225
Blocks: 1144324
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?
Attachment #8578635 - Flags: review?(botond) → review+
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.

Updated

3 years ago
Attachment #8578636 - Flags: review?(botond) → review+

Updated

3 years ago
Attachment #8578637 - Flags: review?(botond) → review+
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.
Attachment #8578642 - Flags: review?(botond) → review+
(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
It has to be above the StartNewTouchBlock call which will (in the cases we care about) throw away the CurrentTouchBlock().
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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!
(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
No longer blocks: 1144324
(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.
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.