1145084 - When touch-action is enabled, ensure the APZ doesn't wait forever for the SetAllowedTouchBehavior notification

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Spinoff from bug 1144112 comment 13.

The APZ code waits until it has necessary information about an input block before processing that input block. When the touch-action pref is enabled, one of the things it waits for is the allowed touch behaviors for the block. Unlike the preventDefault status, there is no timeout on the wait for allowed touch behaviors, which means that APZ is effectively blocked on the main thread in this scenario. We should make this wait subject to the same timeout that we use for preventDefault status, and process the input block after that timeout even if we don't have the information.

Comment 1

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

This is more important to do now that bug 1166347 is turning on touch-action on Nightly.

Comment 2

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

Attachment: WIP

It took a bunch of hacking to get all the gtests passing with the touch-action pref defaulting to true and to false (which is the current state, and that I'd like to maintain). I want to try to clean this up further by reducing the number of places we explicitly have to pump the delayed tasks.

Comment 3

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

Attachment: Patch

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8613576 [details] [diff] [review]
Patch

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

If I'm understanding this patch correctly, it's re-using the timeout used for the ContentReceivedInputBlock response, for SetAllowedTouchBehavior as well.

  - Perhaps this is worth a mention in the APZ docs?

  - We seem to be leaving mAllowedTouchBehaviors empty if the timeout is triggered.
    Based on how the TouchActionAllows*() methods are written, this will allow
    panning but nor pinch-zoom or double-tap-zoom. Is this what we want?

::: gfx/layers/apz/src/InputBlockState.cpp
@@ -459,5 @@
>    if (!CancelableBlockState::IsReadyForHandling()) {
>      return false;
>    }
>  
> -  // TODO: for long-tap blocks we probably don't need the touch behaviour?

Have we decided that we do need the touch behaviour for long-tap blocks?

Comment 5

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

(In reply to Botond Ballo [:botond] from comment #4)
> Comment on attachment 8613576 [details] [diff] [review]
> Patch
> 
> Review of attachment 8613576 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If I'm understanding this patch correctly, it's re-using the timeout used
> for the ContentReceivedInputBlock response, for SetAllowedTouchBehavior as
> well.

Yes.

> 
>   - Perhaps this is worth a mention in the APZ docs?

Good point, I will update those.

>   - We seem to be leaving mAllowedTouchBehaviors empty if the timeout is
> triggered.
>     Based on how the TouchActionAllows*() methods are written, this will
> allow
>     panning but nor pinch-zoom or double-tap-zoom. Is this what we want?

It should allow all the behaviours if mAllowedTouchBehaviors is empty. TouchActionAllowsPinchZoom and TouchActionAllowsDoubleTapZoom default to returning true if mAllowedTouchBehaviors is empty.

> ::: gfx/layers/apz/src/InputBlockState.cpp
> > -  // TODO: for long-tap blocks we probably don't need the touch behaviour?
> 
> Have we decided that we do need the touch behaviour for long-tap blocks?

The code should be copying the touch behaviors from the "current block" at the time it injects the long-tap block, so it'll just inherit those touch behaviors. As far as I could tell we don't need to do anything further for this "TODO", so I removed it.

Comment 6

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

Attachment: Patch v2

Docs slightly updated. Turns out the existing description of the handling of input events near the bottom of the doc already reflected the changes I'm making in this bug (i.e. the docs were wrong before with respect to touch-action, but with the changes in this bug they are correct).

Comment 7

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> >   - We seem to be leaving mAllowedTouchBehaviors empty if the timeout is
> > triggered.
> >     Based on how the TouchActionAllows*() methods are written, this will
> > allow
> >     panning but nor pinch-zoom or double-tap-zoom. Is this what we want?
> 
> It should allow all the behaviours if mAllowedTouchBehaviors is empty.
> TouchActionAllowsPinchZoom and TouchActionAllowsDoubleTapZoom default to
> returning true if mAllowedTouchBehaviors is empty.

Doh! Misread that, apologies.

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8613691 [details] [diff] [review]
Patch v2

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

Thanks!

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7f1e7b7e095

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e7f1e7b7e095