1141127 - Add deadzone to filter out initial touch moves in APZ

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Michael Wu [:mwu]]

HERE maps refuses to recognize clicks in map layer selection when there's a touch move event fired IIRC. Adding a dead zone to filter out the initial touch moves could help and avoid the need for vendors to add similar hacks.

Comment 1

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

Note that one possible fallout is that it might make things like bug 1058255 worse.

Comment 2

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

There is discussion directly related to this bug at https://bugzilla.mozilla.org/show_bug.cgi?id=1219898#c10 and the next few comments.

Comment 3

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

After giving it a bunch of thought I think adding a touchmove dead zone is probably the right way to go. Ideally we would only drop the touchmove events for scrollable frames but because of the way our event handling works we will probably need to do it for non-scrollable frames as well, at least at first.

The specific reason for that is once the input event arrives at the APZ, we do not know for sure which APZC it is targetting, and therefore do not know if the target frame is scrollable or not. In order to know this we have to wait for the response from the main thread, but we might get touchmove events before that response arrives and have to decide whether or not to drop them. I can think of a few solutions to this catch-22, none of which are particularly nice:
1) Queue up the touchmove events so that they don't get delivered to the main thread until we know the confirmed target from the touchstart, and can decide if the target frame is scrollable or not
2) Do the dropping of the slop touchmove events on the main thread, but we would need to make sure the slop distance calculation is exactly the same as in APZC
3) Split the d-t-c region into two, one for "ambigious target scrollframe" and one for "has listeners" so that in the majority of cases we will have a confirmed target upon receipt of the touchstart in the APZC code. Won't fix it for the ambiguous cases though.

There's probably other solutions I haven't thought of yet either. I'll think about this a bit more today.

Comment 4

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

Also for completeness note that the *opposite* of this was tried in bug 1037066, but it was backed out for causing bug 1049250, bug 1055203, and bug 1055214. If I'm understanding the situation correctly, those bugs probably still manifest on the Aries device (unless their code has changed) because Aries automatically does what was attempted in bug 1037066 - sending a touchmove event immediately after a touchstart. So adding a touchmove dead zone should help address those cases as well - in fact, note that bug 1055214 is actually the same issue as this bug was filed for, which validates this theory.

Comment 5

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

I spent a while thinking about this today and the best implementation idea I have so far involves adding a slop state to the TouchBlockState and updating it as input events arrive at the InputQueue. I initially thought this was bad because we would use the tentative target rather than the confirmed target, but I realized that most of the time the tentative target is the same as the confirmed target, and in the edge cases where it's not, the incorrect behaviour wouldn't really be that bad. In other words, the incorrectness would manifest as a few missing touchmove events when the user puts their finger down on an area that we hit-test as scrollable but is really non-scrollable, and then moves their finger within the slop zone. I consider this negligible.

The other main concern I had was about latency - by skipping touchmove events going to content but leaving our content response timeout the same, we might delay the point at which the input block is ready for processing. To analyze this case I sliced the possibility space based on what the content is doing:

1) Content is calling preventDefault on the touchstart. This situation is no different with or without the touchmoves; as soon as the touchstart is done dispatching the APZEventState will send the content response to the APZ.

2) Content is calling preventDefault on the touchmove but NOT the touchstart. This situation is intended to prevent panning while allowing clicking - that's what this bug is about. If the user is tapping, there's no move events to speak of and everything is fine. If the user is panning they can exit the slop zone within the content response timeout (in which case the preventDefault will take effect) or not (in which case the gesture will likely get detected as a long-press instead). So those scenarios are all ok too.

3) Content is not calling preventDefault at all. In this case if the user pans the first touchmove event arriving on the main thread is delayed, so the content response message is delayed, so the input block processing is delayed. The only meaningful consequence here is that the StartTouch notification from APZ to ActiveElementManager will be delayed. Looking at ActiveElementManager::TriggerElementActivation, this would be most noticeable if the element was not pannable, but if that's the case then we wouldn't be dropping touchmove events in the first place. If the element is pannable there's already a delay with the activation so adding to that delay is probably not going to be that bad. We can improve this later if needed by using information from the touchstart event on the main-thread rather than the StartTouch message from APZ.

So net result is I'm fairly confident doing this won't introduce major issues, and we should be able to deal with the ones it does introduce with follow-up fixes.

Comment 6

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

Attachment: Prevent sending touchmove events while in slop area

Comment 7

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> If the user is panning they
> can exit the slop zone within the content response timeout (in which case
> the preventDefault will take effect) or not (in which case the gesture will
> likely get detected as a long-press instead). So those scenarios are all ok
> too.

In practice I find I can still pan on the test page in attachment 8641082 [details] if I move my finger at just the right speed. It seems to fall between these two cases, where the finger exits the slop zone after the content response timeout expires but before it gets detected as a long-press. I guess there's still a 200ms gap between the two. I'm wondering if there's a simple way to delay the content response timeout or something to deal with this problem.

Comment 8

[Botond Ballo [:botond]]

Summarizing a few points that came up during IRC discussion about this bug:

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> If the element is
> pannable there's already a delay with the activation so adding to that delay
> is probably not going to be that bad. We can improve this later if needed by
> using information from the touchstart event on the main-thread rather than
> the StartTouch message from APZ.

We can't do this, because we wait for the StartTouch message not only to determine whether the target is pannable (which we indeed could also determine from the touch-start event), but also to know that the touch block hasn't been prevent-defaulted.

Therefore, for pannable elements, we don't have much of a way of mitigating the increased delay to activation. As things stand right now, the delay would increase from 100 ms (just the activation delay) to 400 ms (300 ms content response timeout + 100 ms activation delay), although it's possible to reduce that to 300 ms (take the max of the content response timeout and the activation delay).

That said, it's important to keep in mind that the delay is only actually that long for tap-and-hold. For tap-and-release, the touch-end of the release will trigger a content response so we don't need to wait for that timeout.

I'm not opposed to us giving this a try and seeing if the increased delay is acceptable in practice.

--------------

We also realized that, for non-pannable elements, since we're not dropping touch-move events for them, we could consider doing the optimization mentioned in bug 1058255 (synthesizing an early touch-move event, to trigger a content response sooner) just for non-pannable elements. I think this is worth exploring.

Comment 9

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

Attachment: Prevent sending touchmove events while in slop area (v2)

Updated so that a content response that arrives after the timer expires is still taken into account, as you suggested on IRC. This addresses comment 7 quite well, I find.

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8683333 [details] [diff] [review]
Prevent sending touchmove events while in slop area (v2)

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

I'd like to see two follow-ups filed:

  1) To explore the "synthesize an early touch-move" optimization for
     non-pannable elements. This could be a reopen of bug 1058255.

  2) To mitigate the increased delay for pannable elements, such as
     by using the max of the content timeout and the activation delay
     instead of both in sequence.

I don't think we need to implement either of these right away, but I'd like the bugs to be in place so we have something to pick up if we do need to mitigate any fallout.

Comment 11

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

Filed bug 1221775 and bug 1221779 as per comment 10.

Comment 12

[Pulsebot]

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

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Chris Lord [:cwiiis]]

Do we want to uplift this to 2.5? (I vote yes) Not sure who makes the call on that, kats, what do you think?

Comment 15

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

Yes, I will request uplift in a few days.

Comment 16

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

Comment on attachment 8683333 [details] [diff] [review]
Prevent sending touchmove events while in slop area (v2)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): support for high-density display devices like Aries which send touchmove events with very small finger movements
User impact if declined: on certain web content attempting to tap on stuff fails because we send a touchmove event to the content between the touchdown and touchup. See bug 1174532 (which is 2.5+) and bug 1196921 for visible examples
Testing completed: locally, on m-c
Risk to taking this patch (and alternatives if risky): low-medium risk but we don't have much in the way of alternatives. this is the right solution for the long-term but we need to be diligent about catching regressions and fixing them.
String or UUID changes made by this patch: none

Comment 17

[Mahendra Potharaju [:mahe]]

Comment on attachment 8683333 [details] [diff] [review]
Prevent sending touchmove events while in slop area (v2)

Approved for 2.5

Thanks

Comment 18

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

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/a20ed9df8859