Stop sending an async touchmove for the app slop region

Stop sending an async touchmove for the app slop region

With throttled, async touch dispatch, an uncancelable touchmove is sent
as soon as the touch sequence exceeds an arbitrarily defined
application slop region, separate from the platform slop region. The
intent in doing so was to allow web apps to clear any tap/press-related
state after a scroll has begun. However, the sending of this touchmove
at the start of a scroll sequence can be problematic; the early part of
a scroll sequence is often sensitive to minor bottlenecks, and the
dispatch of an additional touchmove can conceivably push the app into
high latency mode by delaying frame production.

Stop sending this slop-exceeding touchmove, instead relying on the
existing timer-based async touchmove dispatch.

Note that while this may regress touch behavior on certain sites (e.g.,
causing faulty tap interpretation), it's likely that behavior on such
sites was already racy and fragile, subject to the arbitrary value of
the fixed "application" slop region.

BUG=449275
Committed: https://crrev.com/635c6ec3ee861ee290cecf66e81b98a000ad23cd
Cr-Commit-Position: refs/heads/master@{#313588}

[jdduke (slow), 2015-01-23 22:34:00]

jdduke@chromium.org changed reviewers:
+ rbyers@chromium.org

[jdduke (slow), 2015-01-23 22:34:01]

rbyers@: PTAL. I played with some fixed time delays, but this approach is both
simpler and avoids (at least some of) the arbitrariness.

[jdduke (slow), 2015-01-27 17:20:58]

On 2015/01/23 22:34:01, jdduke wrote:
> rbyers@: PTAL. I played with some fixed time delays, but this approach is both
> simpler and avoids (at least some of) the arbitrariness.

Or do you think we should build in an additional minimum timeout? Perhaps start
with this?

[Rick Byers, 2015-01-27 17:59:15]

Sorry for the delay.  This definitely seems like a hack (kind of weird to have
pending async touchmove with no outstanding ACK).

How different is this behavior really than just removing the application slop
region altogether?  If I understand the impact of this correctly, it means that
the application-slop touchmove may be replaced (coalesced) by the next throttled
touchmove 100ms later.  So if the page is fast enough to get an ACK back within
the frame, it'll see the extra application slop touchmove, otherwise it won't. 
Is that right?

If so, then I think we need to figure out if we need the application slop region
for page compat or not.  If we do, then we probably can't get away with this
change without breaking stuff.  If we don't, then we can simplify things further
by just removing it.

I forget the details surrounding our decision to add the application slop
touchmove.  We had some specific sites that were broken otherwise, right?  I
think they were probably only broken when we were 'absorbing' ALL touchmoves. 
I.e did we decided we needed the application slop touchmove before or after we
decided we needed throttled async touchmoves?  If before, then perhaps we can
just eliminate it without breaking any sites in practice (100ms is relatively
short in terms of user experience time)?  Sites should ideally be using the
first touchmove to trigger their effect anyway, so in the rare case someone
would be impacted there is a work-around for them.

WDYT?
  Rick

https://codereview.chromium.org/872633005/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/872633005/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:1970: // A
subsequent touchmove will flush the deferred async touchmove,
coalescing means it's more like 'replace' than 'flush', right?  Perhaps it's
worth having the test check the co-ordinate of the event here, verifying
coalescing occurred?

[jdduke (slow), 2015-01-27 20:58:59]

On 2015/01/27 17:59:15, Rick Byers wrote:
> Sorry for the delay.  This definitely seems like a hack (kind of weird to have
> pending async touchmove with no outstanding ACK).
> 
> How different is this behavior really than just removing the application slop
> region altogether?  If I understand the impact of this correctly, it means
that
> the application-slop touchmove may be replaced (coalesced) by the next
throttled
> touchmove 100ms later.  So if the page is fast enough to get an ACK back
within
> the frame, it'll see the extra application slop touchmove, otherwise it won't.

> Is that right?
> 
> If so, then I think we need to figure out if we need the application slop
region
> for page compat or not.  If we do, then we probably can't get away with this
> change without breaking stuff.  If we don't, then we can simplify things
further
> by just removing it.
> 
> I forget the details surrounding our decision to add the application slop
> touchmove.  We had some specific sites that were broken otherwise, right?  I
> think they were probably only broken when we were 'absorbing' ALL touchmoves. 
> I.e did we decided we needed the application slop touchmove before or after we
> decided we needed throttled async touchmoves?  If before, then perhaps we can
> just eliminate it without breaking any sites in practice (100ms is relatively
> short in terms of user experience time)?  Sites should ideally be using the
> first touchmove to trigger their effect anyway, so in the rare case someone
> would be impacted there is a work-around for them.
> 
> WDYT?
>   Rick

Hah, this is what I wanted all along (I never like the arbitrary app slop region
=/). We added it after we decided on async throttled touchmoves, for the compat
reasons you mentioned. Even then, it's not foolproof, as there could very well
be delays in the pipeline that affect timing-based touch response, or apps that
have a slop region between the platform and the one we defined in
TouchEventQueue.

I'd be quite content to get rid of it entirely, though we should probably test a
few of the problem cases we first saw (I think Yahoo mail might have been one?).
I'll see if I can dig up the bug.

[Rick Byers, 2015-01-27 21:59:01]

On 2015/01/27 20:58:59, jdduke wrote:
> On 2015/01/27 17:59:15, Rick Byers wrote:
> > Sorry for the delay.  This definitely seems like a hack (kind of weird to
have
> > pending async touchmove with no outstanding ACK).
> > 
> > How different is this behavior really than just removing the application
slop
> > region altogether?  If I understand the impact of this correctly, it means
> that
> > the application-slop touchmove may be replaced (coalesced) by the next
> throttled
> > touchmove 100ms later.  So if the page is fast enough to get an ACK back
> within
> > the frame, it'll see the extra application slop touchmove, otherwise it
won't.
> 
> > Is that right?
> > 
> > If so, then I think we need to figure out if we need the application slop
> region
> > for page compat or not.  If we do, then we probably can't get away with this
> > change without breaking stuff.  If we don't, then we can simplify things
> further
> > by just removing it.
> > 
> > I forget the details surrounding our decision to add the application slop
> > touchmove.  We had some specific sites that were broken otherwise, right?  I
> > think they were probably only broken when we were 'absorbing' ALL
touchmoves. 
> > I.e did we decided we needed the application slop touchmove before or after
we
> > decided we needed throttled async touchmoves?  If before, then perhaps we
can
> > just eliminate it without breaking any sites in practice (100ms is
relatively
> > short in terms of user experience time)?  Sites should ideally be using the
> > first touchmove to trigger their effect anyway, so in the rare case someone
> > would be impacted there is a work-around for them.
> > 
> > WDYT?
> >   Rick
> 
> Hah, this is what I wanted all along (I never like the arbitrary app slop
region
> =/). We added it after we decided on async throttled touchmoves, for the
compat
> reasons you mentioned. Even then, it's not foolproof, as there could very well
> be delays in the pipeline that affect timing-based touch response, or apps
that
> have a slop region between the platform and the one we defined in
> TouchEventQueue.
> 
> I'd be quite content to get rid of it entirely, though we should probably test
a
> few of the problem cases we first saw (I think Yahoo mail might have been
one?).
> I'll see if I can dig up the bug.

In the 'touchmove absorption' days I remember seeing sites where button pressed
states would get stuck down, and on touchend the button would get
unintentionally activated.  Maybe the biggest risk here is for short (<100ms)
flicks where no async touchmove would be generated at all.  We need to make sure
those don't trigger button activations.  I forget, do we reliably send a final
touchmove at the lift location (i.e. flush the pending async touchmove on
touchend)?  If we did, that would probably be sufficient to avoid this concern. 
If we verify a couple of the problematic sites work reliably with that model
then I'm OK eliminating the application slop logic.

Sorry I couldn't be convinced to do this from the start :-).  At the time I
don't think we had evidence that it might be a performance problem, now that we
have concrete traces showing it is the equation is different.

[jdduke (slow), 2015-01-28 19:22:58]

OK, with the latest version I've removed the app slop region.

I tested this against m.trulia.com, gmail.com and washingtonpost.com (per
crbug.com/360614 and crbug.com/353512).

Washingtpost is the only one where you can trigger a tap if you *very* quickly
scroll up then back to the original offset (you have to be quite explicit about
it). You can also trigger a tap if you scroll a very small amount and quickly
release your finger, but I'm also able to repro that on Canary (though
admittedly with Canary it's slightly harder). We still send the final async
touhchmove, and it's uncancelable, so the site at least has a signal that
something has occurred. I'm inclined to think this behavior is acceptable,
particularly as it's partially broken on trunk anyway. Thoughts?

[Rick Byers, 2015-01-28 20:00:09]

Ok, thanks for testing.  I'm not too worried about the unnaturally-quick
scroll-up-then-down scenario.  LGTM.

https://codereview.chromium.org/872633005/diff/60001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/872633005/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:1929:
TEST_F(TouchEventQueueTest, AsyncTouchFlushedByTouchEnd) {
thanks for adding this test

https://codereview.chromium.org/872633005/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:1930:
SetTouchScrollingMode(TouchEventQueue::TOUCH_SCROLLING_MODE_ASYNC_TOUCHMOVE);
we don't need to explicitly set this (it's the default), right?  Lan is in the
process of ripping the mode APIs out - not sure how far along she is.

https://codereview.chromium.org/872633005/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:1960: //
Return the touch sequence to the original touchstart position.
this gets coalesced into the above event, right?

[jdduke (slow), 2015-01-28 20:10:07]

Thanks for review!

https://codereview.chromium.org/872633005/diff/60001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/872633005/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:1930:
SetTouchScrollingMode(TouchEventQueue::TOUCH_SCROLLING_MODE_ASYNC_TOUCHMOVE);
On 2015/01/28 20:00:08, Rick Byers wrote:
> we don't need to explicitly set this (it's the default), right?  Lan is in the
> process of ripping the mode APIs out - not sure how far along she is.

Probably not, only did it for consistency with other tests. Should be easy to
remove.

https://codereview.chromium.org/872633005/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:1960: //
Return the touch sequence to the original touchstart position.
On 2015/01/28 20:00:09, Rick Byers wrote:
> this gets coalesced into the above event, right?

yup, i'll make a note.

[jdduke (slow), 2015-01-28 20:10:11]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2015-01-28 20:11:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/872633005/80001

[commit-bot: I haz the power, 2015-01-28 21:32:57]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-28 21:34:38]

Patchset 5 (id:??) landed as
https://crrev.com/635c6ec3ee861ee290cecf66e81b98a000ad23cd
Cr-Commit-Position: refs/heads/master@{#313588}