Remove UserGesture on touch scrolls

Remove UserGesture on touch scrolls

The only type of touch which now takes a user gesture is touchend/pointerup
when it doesn't correspond to a touch scroll.  Conceptually scrolling
consume's the user's intent - no sensitive action should be triggered for
the same gesture that does a scroll.  For details see:
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nvOg/edit

Intent to remove: https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/gesture%7Csort:relevance/blink-dev/TO_x7FRkdmw/uCrOwufTCAAJ

BUG=611981
Committed: https://crrev.com/60b7754704a8fd81b0a010ce253b3754e72c50ec
Cr-Commit-Position: refs/heads/master@{#430330}

[Rick Byers, 2016-10-14 21:40:40]

The CQ bit was checked by rbyers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 21:41:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 23:06:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 23:06:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Rick Byers, 2016-11-04 13:41:46]

Description was changed from

==========
Remove UserGesture on touch scrolls

The only type of touch which now takes a user gesture is touchend/pointerup
when it doesn't correspond to a touch scroll.  For details see:
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

BUG=611981
==========

to

==========
Remove UserGesture on touch scrolls

The only type of touch which now takes a user gesture is touchend/pointerup
when it doesn't correspond to a touch scroll.  For details see:
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

Intent to remove:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/gesture%7...

BUG=611981
==========

[Rick Byers, 2016-11-04 13:54:49]

The CQ bit was checked by rbyers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 13:55:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Rick Byers, 2016-11-04 13:56:24]

Description was changed from

==========
Remove UserGesture on touch scrolls

The only type of touch which now takes a user gesture is touchend/pointerup
when it doesn't correspond to a touch scroll.  For details see:
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

Intent to remove:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/gesture%7...

BUG=611981
==========

to

==========
Remove UserGesture on touch scrolls

The only type of touch which now takes a user gesture is touchend/pointerup
when it doesn't correspond to a touch scroll.  Conceptually scrolling consume's
the user's intent - no sensitive action should be triggered for the same gesture
that does a scroll.  For details see:
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

Intent to remove:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/gesture%7...

BUG=611981
==========

[Rick Byers, 2016-11-04 13:56:50]

Description was changed from

==========
Remove UserGesture on touch scrolls

The only type of touch which now takes a user gesture is touchend/pointerup
when it doesn't correspond to a touch scroll.  Conceptually scrolling consume's
the user's intent - no sensitive action should be triggered for the same gesture
that does a scroll.  For details see:
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

Intent to remove:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/gesture%7...

BUG=611981
==========

to

==========
Remove UserGesture on touch scrolls

The only type of touch which now takes a user gesture is touchend/pointerup
when it doesn't correspond to a touch scroll.  Conceptually scrolling
consume's the user's intent - no sensitive action should be triggered for
the same gesture that does a scroll.  For details see:
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

Intent to remove:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/gesture%7...

BUG=611981
==========

[commit-bot: I haz the power, 2016-11-04 14:13:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 14:13:07]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Rick Byers, 2016-11-04 14:31:38]

The CQ bit was checked by rbyers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 14:31:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 15:53:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 15:53:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Rick Byers, 2016-11-04 17:49:28]

The CQ bit was checked by rbyers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 17:49:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Rick Byers, 2016-11-04 17:57:06]

rbyers@chromium.org changed reviewers:
+ nzolghadr@chromium.org, tedchoc@chromium.org

[Rick Byers, 2016-11-04 17:57:07]

Navid,
PTAL.  This moves to the simpler UserGesture handling for touch events (and
poitnertype=touch PointerEvents).  WDYT?

+Ted for chrome/test/data/navigation_interception.  In general pages should not
be trying to navigate in response to a touchstart because the user is likely
just trying to scroll, not tap (some bad ads do this, so we explicitly want to
prevent it).  See the intent thread for a discussion of the compat risk.  We'll
keep our eyes open for reports of site breakage and revert if necessary, but I
believe it should be pretty safe.

[Navid Zolghadr, 2016-11-04 18:08:19]

https://codereview.chromium.org/2414273003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/2414273003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:319:
DocumentUserGestureToken::create(touchInfos[0].targetFrame->document());
I'm a little confused about the intended behavior when different fingers are on
different iframes.
PointerEvents are targeted to the elements in their own document as oppose to
the touch events. So basically if the second finger touches another iframe for
the pointerup of that finger we will not have a token in that document.

[commit-bot: I haz the power, 2016-11-04 19:33:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 19:33:09]

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-11-04 23:06:31]

On 2016/11/04 17:57:07, Rick Byers wrote:
> Navid,
> PTAL.  This moves to the simpler UserGesture handling for touch events (and
> poitnertype=touch PointerEvents).  WDYT?
> 
> +Ted for chrome/test/data/navigation_interception.  In general pages should
not
> be trying to navigate in response to a touchstart because the user is likely
> just trying to scroll, not tap (some bad ads do this, so we explicitly want to
> prevent it).  See the intent thread for a discussion of the compat risk. 
We'll
> keep our eyes open for reports of site breakage and revert if necessary, but I
> believe it should be pretty safe.

lgtm -- assuming the tests past no issue on my end (and the tests passed so
yay!)

[Rick Byers, 2016-11-07 15:16:06]

On 2016/11/04 18:08:19, Navid Zolghadr wrote:
>
https://codereview.chromium.org/2414273003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):
> 
>
https://codereview.chromium.org/2414273003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/PointerEventManager.cpp:319:
> DocumentUserGestureToken::create(touchInfos[0].targetFrame->document());
> I'm a little confused about the intended behavior when different fingers are
on
> different iframes.
> PointerEvents are targeted to the elements in their own document as oppose to
> the touch events. So basically if the second finger touches another iframe for
> the pointerup of that finger we will not have a token in that document.

Yeah I've been sloppy about the multi-finger case, I don't think the exact
semantics matter too much.  I think it's very unlikely that anyone would do
something "sensitive" on a multi-finger gesture.  But my intention was that at
least lifting the last finger would be a user gesture, and whichever document
received the first finger would have been considered to have had a gesture.  I
could beef this up a bit to be precise (only a gesture when the final finger has
been lifted) and add a test if you think it's worth it?

[Navid Zolghadr, 2016-11-07 15:28:55]

On 2016/11/07 15:16:06, Rick Byers wrote:
> On 2016/11/04 18:08:19, Navid Zolghadr wrote:
> >
>
https://codereview.chromium.org/2414273003/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2414273003/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/input/PointerEventManager.cpp:319:
> > DocumentUserGestureToken::create(touchInfos[0].targetFrame->document());
> > I'm a little confused about the intended behavior when different fingers are
> on
> > different iframes.
> > PointerEvents are targeted to the elements in their own document as oppose
to
> > the touch events. So basically if the second finger touches another iframe
for
> > the pointerup of that finger we will not have a token in that document.
> 
> Yeah I've been sloppy about the multi-finger case, I don't think the exact
> semantics matter too much.  I think it's very unlikely that anyone would do
> something "sensitive" on a multi-finger gesture.  But my intention was that at
> least lifting the last finger would be a user gesture, and whichever document
> received the first finger would have been considered to have had a gesture.  I
> could beef this up a bit to be precise (only a gesture when the final finger
has
> been lifted) and add a test if you think it's worth it?

I don't think a test is needed. I just wanted to make sure you knew the side
effect here. Also one more point regarding the touchend. You mentioned "that at
least lifting the last finger would be a user gesture". Just wanted to point out
that this CL will create two gestures for two fingers in the same document. So
even for the first finger that releases the screen we get a touchend (on the
element that the finger started on) and we create a gesture which is different
from the previous logic of only allowing on tap (as two finger would not be
considered tap I guess). If that is also okay, the rest of the change should be
fine. lgtm.

[Rick Byers, 2016-11-07 16:52:32]

On 2016/11/07 15:28:55, Navid Zolghadr wrote:
> On 2016/11/07 15:16:06, Rick Byers wrote:
> > On 2016/11/04 18:08:19, Navid Zolghadr wrote:
> > >
> >
>
https://codereview.chromium.org/2414273003/diff/60001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2414273003/diff/60001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/input/PointerEventManager.cpp:319:
> > > DocumentUserGestureToken::create(touchInfos[0].targetFrame->document());
> > > I'm a little confused about the intended behavior when different fingers
are
> > on
> > > different iframes.
> > > PointerEvents are targeted to the elements in their own document as oppose
> to
> > > the touch events. So basically if the second finger touches another iframe
> for
> > > the pointerup of that finger we will not have a token in that document.
> > 
> > Yeah I've been sloppy about the multi-finger case, I don't think the exact
> > semantics matter too much.  I think it's very unlikely that anyone would do
> > something "sensitive" on a multi-finger gesture.  But my intention was that
at
> > least lifting the last finger would be a user gesture, and whichever
document
> > received the first finger would have been considered to have had a gesture. 
I
> > could beef this up a bit to be precise (only a gesture when the final finger
> has
> > been lifted) and add a test if you think it's worth it?
> 
> I don't think a test is needed. I just wanted to make sure you knew the side
> effect here. Also one more point regarding the touchend. You mentioned "that
at
> least lifting the last finger would be a user gesture". Just wanted to point
out
> that this CL will create two gestures for two fingers in the same document. So
> even for the first finger that releases the screen we get a touchend (on the
> element that the finger started on) and we create a gesture which is different
> from the previous logic of only allowing on tap (as two finger would not be
> considered tap I guess). If that is also okay, the rest of the change should
be
> fine. lgtm.

Yeah although it probably will never be hit, I did want it to be any finger. 
Eg.
maybe some site would use a "pinch to open" gesture, it shouldn't matter whether
they trigger on the first touchend or second touchend.

I've expanded the comment and also the details in the linked design doc.
If this works, then we can try to get this into the HTML spec.

[Rick Byers, 2016-11-07 16:53:11]

The CQ bit was checked by rbyers@chromium.org

[Rick Byers, 2016-11-07 16:53:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, nzolghadr@chromium.org
Link to the patchset: https://codereview.chromium.org/2414273003/#ps80001
(title: "Improve comments")

[commit-bot: I haz the power, 2016-11-07 16:53:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 18:55:39]

Description was changed from

==========
Remove UserGesture on touch scrolls

The only type of touch which now takes a user gesture is touchend/pointerup
when it doesn't correspond to a touch scroll.  Conceptually scrolling
consume's the user's intent - no sensitive action should be triggered for
the same gesture that does a scroll.  For details see:
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

Intent to remove:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/gesture%7...

BUG=611981
==========

to

==========
Remove UserGesture on touch scrolls

The only type of touch which now takes a user gesture is touchend/pointerup
when it doesn't correspond to a touch scroll.  Conceptually scrolling
consume's the user's intent - no sensitive action should be triggered for
the same gesture that does a scroll.  For details see:
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

Intent to remove:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/gesture%7...

BUG=611981
==========

[commit-bot: I haz the power, 2016-11-07 18:55:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-07 18:59:19]

Description was changed from

==========
Remove UserGesture on touch scrolls

The only type of touch which now takes a user gesture is touchend/pointerup
when it doesn't correspond to a touch scroll.  Conceptually scrolling
consume's the user's intent - no sensitive action should be triggered for
the same gesture that does a scroll.  For details see:
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

Intent to remove:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/gesture%7...

BUG=611981
==========

to

==========
Remove UserGesture on touch scrolls

The only type of touch which now takes a user gesture is touchend/pointerup
when it doesn't correspond to a touch scroll.  Conceptually scrolling
consume's the user's intent - no sensitive action should be triggered for
the same gesture that does a scroll.  For details see:
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

Intent to remove:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/gesture%7...

BUG=611981

Committed: https://crrev.com/60b7754704a8fd81b0a010ce253b3754e72c50ec
Cr-Commit-Position: refs/heads/master@{#430330}
==========

[commit-bot: I haz the power, 2016-11-07 18:59:20]

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