Suppress drag and drop on links on touch screen.

Suppress drag and drop on links on touch screen.

Both drag and drop and context menu use the same gesture: long press on Android.
In order to maintain backwards compatibility, we need to suppress drag and drop
for links on Android and continue to use long press gesture for context menus.

This change also applies to other platforms with a touch screen, so that touch
screen behavior is consistent on all platforms.

BUG=636005
Committed: https://crrev.com/444ef13122a6e440c1462a187d8f4f7031935855
Cr-Commit-Position: refs/heads/master@{#415769}

[hush (inactive), 2016-08-15 23:11:12]

hush@chromium.org changed reviewers:
+ dtapuska@chromium.org

[hush (inactive), 2016-08-15 23:11:13]

Hello Dave, 
could you take a look first, before I send it for owner review?
Thank you!

[hush (inactive), 2016-08-16 23:23:13]

PS2:
there are cases when the hitTestResult contains no link element, but still
contains an image URL, and webview/chrome is supposed to open a context menu.

[dtapuska, 2016-08-17 13:39:49]

https://codereview.chromium.org/2247963002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2247963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/GestureManager.cpp:257: IntPoint
hitTestPoint = m_frame->view()->rootFrameToContents(gestureEvent.position());
Seems reasonable. I'd like to see a test exercising this behavior though.

Moving the hit test before the handleDragDropIfPossible seemed dangerous at
first because what about DOM mutations. But after a further examination the
handleDragDropIfPossible returns true if it dispatches an event into JS so I
wouldn't expect any mutations to the DOM.

[hush (inactive), 2016-08-17 18:09:46]

https://codereview.chromium.org/2247963002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2247963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/GestureManager.cpp:257: IntPoint
hitTestPoint = m_frame->view()->rootFrameToContents(gestureEvent.position());
On 2016/08/17 13:39:49, dtapuska wrote:
> Seems reasonable. I'd like to see a test exercising this behavior though.
> 
> Moving the hit test before the handleDragDropIfPossible seemed dangerous at
> first because what about DOM mutations. But after a further examination the
> handleDragDropIfPossible returns true if it dispatches an event into JS so I
> wouldn't expect any mutations to the DOM.

Is it OK for the test to be Android only? The added logic is Android only.

[dtapuska, 2016-08-17 18:12:46]

https://codereview.chromium.org/2247963002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2247963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/GestureManager.cpp:257: IntPoint
hitTestPoint = m_frame->view()->rootFrameToContents(gestureEvent.position());
On 2016/08/17 18:09:46, hush wrote:
> On 2016/08/17 13:39:49, dtapuska wrote:
> > Seems reasonable. I'd like to see a test exercising this behavior though.
> > 
> > Moving the hit test before the handleDragDropIfPossible seemed dangerous at
> > first because what about DOM mutations. But after a further examination the
> > handleDragDropIfPossible returns true if it dispatches an event into JS so I
> > wouldn't expect any mutations to the DOM.
> 
> Is it OK for the test to be Android only? The added logic is Android only.
I believe there are virtual LayoutTests for android under
third_party/WebKit/LayoutTests/virtual/android/*. But best to check with someone
who has written a test in that directory.

[hush (inactive), 2016-08-19 19:14:15]

hush@chromium.org changed reviewers:
+ dcheng@chromium.org

[hush (inactive), 2016-08-19 19:14:44]

Hi dcheng@, 
PTAL third_party/WebKit

[hush (inactive), 2016-08-19 19:15:35]

Patchset #4 (id:60001) has been deleted

[dcheng, 2016-08-19 19:22:05]

Just curious, but why do we need to add a toggle for this behavior at all?
Wouldn't we probably want this behavior on non-Android platforms with
touchscreens too? How does CrOS behave?

[hush (inactive), 2016-08-19 21:30:12]

On 2016/08/19 19:22:05, dcheng wrote:
> Just curious, but why do we need to add a toggle for this behavior at all?
> Wouldn't we probably want this behavior on non-Android platforms with
> touchscreens too? How does CrOS behave?

I have been informed in multiple occasions that platform specific #ifdefs are
bad. So I'm trying to push the platform specific logic in upper layers
(render_view_impl). And that seems to be what the other code is doing too.
The toggle helps to make the test easier to write. 

I think any platform that has touchscreens as the exclusive source of input will
need this change, because long press means two things at the same time. But I'm
not sure if that applies to chrome os.
For chrome os, or chrome books at least, I think users can still use mouse
(right click) or the touch pad (two finger tap) to trigger a context menu? I
haven't tried on chrome os though because I don't have one

[hush (inactive), 2016-08-19 21:54:49]

I just got my hands on a chromebook and tested its behavior. You can always two
finger tap to bring up the context menu, so we don't need to change anything for
chromeos.

[hush (inactive), 2016-08-23 16:05:32]

ping dcheng@?

[hush (inactive), 2016-08-29 17:57:29]

Description was changed from

==========
Suppress drag and drop on links on Android.

Both drag and drop and context menu use the same gesture: long press on Android.
In order to maintain backwards compatibility, we need to suppress drag and drop
for links on Android and continue to use long press gesture for context menus.

BUG=636005
==========

to

==========
Suppress drag and drop on links on touch screen.

Both drag and drop and context menu use the same gesture: long press on Android.
In order to maintain backwards compatibility, we need to suppress drag and drop
for links on Android and continue to use long press gesture for context menus.

This change also applies to other platforms with a touch screen, so that touch
screen behavior is consistent on all platforms.

BUG=636005
==========

[hush (inactive), 2016-08-29 17:58:55]

Hello dcheng@,
PTAL

[dcheng, 2016-08-29 19:45:08]

https://codereview.chromium.org/2247963002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2247963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/GestureManager.cpp:261: bool
hitTestContainsLinks = hitTestResult.URLElement() ||
!hitTestResult.absoluteLinkURL().isNull() ||
!hitTestResult.absoluteImageURL().isNull() ||
!hitTestResult.absoluteMediaURL().isNull();
This isn't just links, right? It's images and videos as well?

Alo, why do we needto check URLElement() and absoluteLinkURL().isNull()?

[hush (inactive), 2016-08-29 20:49:49]

https://codereview.chromium.org/2247963002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2247963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/GestureManager.cpp:261: bool
hitTestContainsLinks = hitTestResult.URLElement() ||
!hitTestResult.absoluteLinkURL().isNull() ||
!hitTestResult.absoluteImageURL().isNull() ||
!hitTestResult.absoluteMediaURL().isNull();
On 2016/08/29 19:45:08, dcheng wrote:
> This isn't just links, right? It's images and videos as well?
> 
> Alo, why do we needto check URLElement() and absoluteLinkURL().isNull()?

Yes. Long pressing on images and videos should open context menu as well,
instead of staring DnD. Otherwise, the user can't open context menu on these
things any more.

We need to check URLElement() and absoluteLinkURL().isNull() because they are
only for links, both URLElement() and absoluteLinkURL() can be null when the
user is long pressing on an image.

[dcheng, 2016-08-29 21:10:04]

LGTM

https://codereview.chromium.org/2247963002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2247963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/GestureManager.cpp:261: bool
hitTestContainsLinks = hitTestResult.URLElement() ||
!hitTestResult.absoluteLinkURL().isNull() ||
!hitTestResult.absoluteImageURL().isNull() ||
!hitTestResult.absoluteMediaURL().isNull();
On 2016/08/29 20:49:49, hush wrote:
> On 2016/08/29 19:45:08, dcheng wrote:
> > This isn't just links, right? It's images and videos as well?
> > 
> > Alo, why do we needto check URLElement() and absoluteLinkURL().isNull()?
> 
> Yes. Long pressing on images and videos should open context menu as well,
> instead of staring DnD. Otherwise, the user can't open context menu on these
> things any more.
> 
> We need to check URLElement() and absoluteLinkURL().isNull() because they are
> only for links, both URLElement() and absoluteLinkURL() can be null when the
> user is long pressing on an image.

As discussed offline, I think we can just check HitTestResult::URLElement().

[hush (inactive), 2016-08-29 21:14:53]

https://codereview.chromium.org/2247963002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2247963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/GestureManager.cpp:261: bool
hitTestContainsLinks = hitTestResult.URLElement() ||
!hitTestResult.absoluteLinkURL().isNull() ||
!hitTestResult.absoluteImageURL().isNull() ||
!hitTestResult.absoluteMediaURL().isNull();
On 2016/08/29 21:10:03, dcheng wrote:
> On 2016/08/29 20:49:49, hush wrote:
> > On 2016/08/29 19:45:08, dcheng wrote:
> > > This isn't just links, right? It's images and videos as well?
> > > 
> > > Alo, why do we needto check URLElement() and absoluteLinkURL().isNull()?
> > 
> > Yes. Long pressing on images and videos should open context menu as well,
> > instead of staring DnD. Otherwise, the user can't open context menu on these
> > things any more.
> > 
> > We need to check URLElement() and absoluteLinkURL().isNull() because they
are
> > only for links, both URLElement() and absoluteLinkURL() can be null when the
> > user is long pressing on an image.
> 
> As discussed offline, I think we can just check HitTestResult::URLElement().

Done.

[hush (inactive), 2016-08-29 21:20:48]

The CQ bit was checked by hush@chromium.org

[hush (inactive), 2016-08-29 21:20:49]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2247963002/#ps120001
(title: "change the check to be hitTestResult.URLElement()")

[commit-bot: I haz the power, 2016-08-29 21:45:18]

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

[hush (inactive), 2016-08-30 00:39:18]

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

[hush (inactive), 2016-08-30 00:42:59]

dcheng@ PTAL PS 8:
It fixes the layout test touchadjustment/touch-links-longpress.html.
This test was originally written for 
https://bugs.webkit.org/show_bug.cgi?id=92914
which was mainly about touch adjustments.

It relies on the old fact that long pressing on links will initiate drag and
drop to assert that correct nodes are tapped.

Since I've changed the behavior, the test needs to change accordingly. But it
still tests touch adjustments effectively.

[hush (inactive), 2016-08-30 01:22:10]

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

[commit-bot: I haz the power, 2016-08-30 01:51:16]

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-08-30 04:51:20]

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

[commit-bot: I haz the power, 2016-08-30 04:51:21]

Dry run: Exceeded global retry quota

[hush (inactive), 2016-08-30 18:37:22]

OK... I have no idea why the test touchadjustment/touch-links-longpress.html
passes on Linux consistently but fails on mac and windows.

Do you guys have any ideas?

[hush (inactive), 2016-08-30 18:41:44]

On 2016/08/30 18:37:22, hush wrote:
> OK... I have no idea why the test touchadjustment/touch-links-longpress.html
> passes on Linux consistently but fails on mac and windows.
> 
> Do you guys have any ideas?

Actually, it fails just on Mac which has a different expected.txt for whatever
reason.

[hush (inactive), 2016-08-30 18:53:09]

On 2016/08/30 18:41:44, hush wrote:
> On 2016/08/30 18:37:22, hush wrote:
> > OK... I have no idea why the test touchadjustment/touch-links-longpress.html
> > passes on Linux consistently but fails on mac and windows.
> > 
> > Do you guys have any ideas?
> 
> Actually, it fails just on Mac which has a different expected.txt for whatever
> reason.

I dug around for a bit and this was introduced in
https://bugs.webkit.org/show_bug.cgi?id=103363
Basically the original devs thought this test was useless on Mac.
I'm just gonna rebaseline this test for mac.

[hush (inactive), 2016-08-30 18:58:51]

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

[commit-bot: I haz the power, 2016-08-30 18:59:32]

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-08-30 21:29:33]

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

[commit-bot: I haz the power, 2016-08-30 21:29:34]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hush (inactive), 2016-08-30 22:56:51]

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

[commit-bot: I haz the power, 2016-08-30 22:57:31]

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-08-31 00:33:09]

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

[commit-bot: I haz the power, 2016-08-31 00:33:10]

Dry run: This issue passed the CQ dry run.

[hush (inactive), 2016-08-31 21:51:30]

The CQ bit was checked by hush@chromium.org

[hush (inactive), 2016-08-31 21:51:30]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2247963002/#ps200001
(title: "rebase mac result so it uses the expected txt shared by all platforms")

[commit-bot: I haz the power, 2016-08-31 21:52:29]

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

[dcheng, 2016-08-31 21:56:46]

still LGTM

[commit-bot: I haz the power, 2016-08-31 21:57:36]

Description was changed from

==========
Suppress drag and drop on links on touch screen.

Both drag and drop and context menu use the same gesture: long press on Android.
In order to maintain backwards compatibility, we need to suppress drag and drop
for links on Android and continue to use long press gesture for context menus.

This change also applies to other platforms with a touch screen, so that touch
screen behavior is consistent on all platforms.

BUG=636005
==========

to

==========
Suppress drag and drop on links on touch screen.

Both drag and drop and context menu use the same gesture: long press on Android.
In order to maintain backwards compatibility, we need to suppress drag and drop
for links on Android and continue to use long press gesture for context menus.

This change also applies to other platforms with a touch screen, so that touch
screen behavior is consistent on all platforms.

BUG=636005
==========

[commit-bot: I haz the power, 2016-08-31 21:57:37]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-08-31 21:59:30]

Description was changed from

==========
Suppress drag and drop on links on touch screen.

Both drag and drop and context menu use the same gesture: long press on Android.
In order to maintain backwards compatibility, we need to suppress drag and drop
for links on Android and continue to use long press gesture for context menus.

This change also applies to other platforms with a touch screen, so that touch
screen behavior is consistent on all platforms.

BUG=636005
==========

to

==========
Suppress drag and drop on links on touch screen.

Both drag and drop and context menu use the same gesture: long press on Android.
In order to maintain backwards compatibility, we need to suppress drag and drop
for links on Android and continue to use long press gesture for context menus.

This change also applies to other platforms with a touch screen, so that touch
screen behavior is consistent on all platforms.

BUG=636005

Committed: https://crrev.com/444ef13122a6e440c1462a187d8f4f7031935855
Cr-Commit-Position: refs/heads/master@{#415769}
==========

[commit-bot: I haz the power, 2016-08-31 21:59:31]

Patchset 10 (id:??) landed as
https://crrev.com/444ef13122a6e440c1462a187d8f4f7031935855
Cr-Commit-Position: refs/heads/master@{#415769}