1107716 - Call SetState(PANNING) / SetState(NOTHING) at the right times for pan events

Status: ASSIGNED

(Core :: Panning and Zooming, defect, P3)

Description

[Markus Stange [:mstange]]

Attachment: v1

TYLin noticed this when testing the touch copy/paste panel with APZ on Mac: SetState is not correctly reset after a pan, and thus nsIScrollObserver::AsyncPanZoomStarted/Stopped are called at the wrong times.

Comment 1

[Botond Ballo [:botond]]

Comment on attachment 8532258 [details] [diff] [review]
v1

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1635,5 @@
>      CancelAnimation();
>    }
>  
>    mPanGestureState = MakeUnique<InputBlockState>(this, true);
> +  SetState(PANNING);

My understanding is that a series of pan-momentum events are more like a fling animation than a pan. Would it make sense to use FLING here, or to introduce a new PanZoomState specifically for this, rather than to use PANNING?

Comment 2

[Markus Stange [:mstange]]

FLING seems to imply that there's an animation running that you can cancel, though the only real consumers of the FLING state are touch event handlers, which won't fire on platforms where we use pan events, afaik.
A new state would probably better. Do you have good ideas for a name? EVENT_CONTROLLED_FLING, PAN_MOMENTUM, ...?

Comment 3

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

Tangential question: what happens if you get a PanMomentumStart, followed by a bunch of PanMomentum events, except there's latency somewhere in the event delivery code and so the PanMomentum events large gaps in between. I think that based on the current code that will result in user-visible jank. Is that ok? Should we in fact kick off a smooth-scroll animation instead on PanMomentumStart and then "update" it as we get more PanMomentum events? That might smooth over the jank and result in a better user experience. If that is something that sounds conceptually correct (even if it's something we never do) I would suggest SMOOTH_SCROLL or FLING would in fact be appropriate. I'm not opposed to adding a new state though if that makes things simpler/more explicit.

Comment 4

[Markus Stange [:mstange]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> except there's latency somewhere in the event delivery code

I don't think we need to plan for this. I'm assuming that we won't ever need pan momentum events on platforms other than Mac, because Mac is the only platform with a native fling animation that we want to match. Since APZ on Mac has its own thread that does nothing except process mouse wheel events, event processing should never be the bottleneck of the APZ pipeline. If the system is so busy that we need more than 8ms to receive or process an event, there probably won't be any resources to render a smooth animation either.

Comment 5

[Botond Ballo [:botond]]

(In reply to Markus Stange [:mstange] from comment #2)
> FLING seems to imply that there's an animation running that you can cancel,
> though the only real consumers of the FLING state are touch event handlers,
> which won't fire on platforms where we use pan events, afaik.
> A new state would probably better. Do you have good ideas for a name?
> EVENT_CONTROLLED_FLING, PAN_MOMENTUM, ...?

I like PAN_MOMENTUM.

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8532258 [details] [diff] [review]
v1

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

Dropping review flag until the patch is updated to introduce a new state.