1147335 - can't drag tabs over on Windows 8.1 touch

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[Karen Rudnitski [:kar]]

I would expect to be able to long-tap and drag a tab across other tabs, but it doesn't work. I have to <sigh> use my mouse to do that. It's something that I come across several times a week, really... thought I'd finally get around to logging the bug because it does bug me.

Comment 1

[Abe - QA (:Abe_LV)]

This is still reproducible in windows-10 touch enabled laptop.

--
Version 	51.0a1
Build ID 	20160830030201
Update Channel 	nightly
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0

Comment 2

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

Philipp, do you have any thoughts on this feature? Right now long-pressing the tab pops up a context menu on it, so I don't think we can re-use that gesture for moving tabs. And I intended to make regular "dragging" (without the long-press) the tab scroll the tabs, in bug 1302737. If you have comments on that also please let me know.

Comment 3

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

It looks like the default behavior in Windows conveniently solves this:
- Long press and drag => move the tab
- Long press and release => context menu
- Short press and drag => scroll in tab strip as you suggested.

That sounds pretty reasonable to me and it's consistent with the platform :)

Comment 4

[Petruta Horea [:phorea], Desktop QA]

According to https://www.microsoft.com/surface/en-us/support/touch-mouse-and-search/using-touch-gestures-tap-swipe-and-beyond?os=windows-8.1-rt-update-3&=undefined , tapping twice - holding on the second tap, then sliding enables the dragging.

This worked fine until bug 1180706 landed. Setting pref dom.w3c_touch_events.enabled to 2 and restarting the browser, no longer shows the issue in latest Aurora 52.0a2 and latest Nightly 53.0a1 2016-12-07.

Tested on a Microsoft Surface Pro tablet with Windows 10 Pro Insider Preview.

Comment 5

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

I can repro this, seeing if there's a good way to fix it.

Comment 6

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

So I think what we need to do is have the APZ GestureEventListener fire some sort of event for when the second tap goes down, and hook that event up to EventStateManager::PreHandleEvent so that it calls BeginTrackingDragGesture. Touchmoves after that should get piped to GenerateDragGesture, and the final touchend calls StopTrackingDragGesture. I think those *DragGesture functions could probably take WidgetUIEvent instead of WidgetMouseEvent, so we can pass touch events into them directly.

However we probably also need to consider interleaving of touch drag with other kinds of events (e.g. mouse drag), I don't know what what the expected behaviour there is.

Comment 7

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

After starting to implement this I discovered that during a mouse-drag, mousemove events are not actually fired. Instead, dragover (or other drag events like dragenter/dragexit) are fired instead. If we want to do the same for touch (suppress touchmove events from firing, but fire drag events instead) then I think we need to put the drag gesture detection and handling code on the main thread (in EventStateManager, where it currently happens for mouse events).

If we try to do it the way I described above, the drag messages from GestureEventListener -> EventStateManager are going to be "out of band" relative to the actual WidgetTouchEvent flow, and that makes it hard to suppress the touch events based on the drag messages unless we implement some complicated mapping and queueing system. It seems simpler to just do touch-drag detection in EventStateManager (or some helper class, most likely).

Comment 8

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

Attachment: Part 1 (WIP) - Add TouchDragDetector

Comment 9

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

Attachment: Part 2 (WIP) - Hook up the TouchDragDetector

smaug, just looking for feedback as to whether this approach is ok or not. Basically I want to add a touch gesture that allows doing drag-and-drop. The patches are WIP but seem to do what I expect. Still be done: extract magic numbers into prefs, enable this only on Windows touch devices, and try to write some tests for it.

As far as I can tell Chrome doesn't seem to support doing drag-and-drop via touch. There's a flag for it in chrome:///flags but it doesn't seem to change anything.

Comment 10

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> After starting to implement this I discovered that during a mouse-drag,
> mousemove events are not actually fired.

Turns out this is handled by the OS. Once we initiate the drag the OS stops sending us touch/mouse events until the drag is complete. So most of what I said in comment 7 is wrong, and it might still be possible to implement this in the APZ gesture listener.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Do we need to support this touch-drag in content? Or would it be enough to have it in parent process/chrome ?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

+  enum class State {
+    eInitial,
+    eFirstDown,
+    eFirstUp,
+    eSecondDown,
+    eDrag
+  };
is a bit unclear to me. Is "first" and "second" about basically same touch clicks (second ~ doubleclick) or about first one touch and while that is on, the next touch?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(Just commenting the code there, not thinking about what I expect user to do)

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

And the state handling in the patch one is a bit weird. After first up we may enter to second down.
Shouldn't the state be something like eWaitingForSecondDown or so?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8820364 [details] [diff] [review]
Part 2 (WIP) - Hook up the TouchDragDetector

I think something like this could work.
The question is do we want this to be initialized initially only from chrome code to get some more experience on this.

Touch handling web sites probably rely on touch for dnd. 
Though, there are cases like drag-and-drop images from web page to desktop etc.

Comment 16

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

(In reply to Olli Pettay [:smaug] from comment #11)
> Do we need to support this touch-drag in content? Or would it be enough to
> have it in parent process/chrome ?

For this bug as filed just the chrome process would be fine. However there is also a regression with touch-dragging in content, so I would like to enable it in both chrome and content. For example going to http://html5demos.com/drag and trying to the dnd action there worked before we turned on touch-apz support on a windows touch device. Now it doesn't work, so I'd like to not ship that regression.

(In reply to Olli Pettay [:smaug] from comment #12)
> is a bit unclear to me. Is "first" and "second" about basically same touch
> clicks (second ~ doubleclick) or about first one touch and while that is on,
> the next touch?

Sorry, I'll comment the code. Basically the gesture that the user has to do is "down, up, down, move", all with one finger.

(In reply to Olli Pettay [:smaug] from comment #14)
> And the state handling in the patch one is a bit weird. After first up we
> may enter to second down.
> Shouldn't the state be something like eWaitingForSecondDown or so?

eFirstUp basically is the same as eWaitingForSecondDown. That is, we enter state eFirstUp after the first touch is lifted, and so the next step in the gesture is for the finger go down again. I tend to name my states based on what has already happened rather than what we're expecting to happen (hence eFirstUp rather than eWaitingForSecondDown).

Comment 17

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

In general I feel like this gesture is very hard to discover, so a lot of people probably are not using it. Chrome also doesn't support touch-based dnd in content (I confirmed with dtapuska on input-dev that it's a neglected feature on their side) so if we don't ship it in content I don't think it would be a huge loss. However it would still be a regression and it's not clear to me that we really gain much shipping it only in chrome and waiting. How long would we wait in the absence of any feedback before enabling it in content?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Hard to say, but perhaps at least land initially for chrome only and ask people to try out the behavior in Nightly. If everything feels ok, repeat the same for content? Maybe could happen even in one release cycle?
I'm just hoping that we're a bit careful before shipping.

Comment 19

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

I cleaned up the implementation a bit but during testing I ran into a problem where intermittently we go into ::DoDragDrop while my finger is off the screen. Since the OS has control at that point, touch interaction appears broken to the user, but then moving the mouse corrects the state. If you don't have a mouse attached it may not be easy to get out of this state, so I'm trying to figure out why it's happening and how to avoid it. It seems kind of like a timing issue where the touchup has happened before we enter DoDragDrop, except since everything is running serially on the main thread I don't see how that's possible.

Comment 20

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

The issue I described in comment 19 was encountered by Chromium devs as well [1] and explored in some depth in [2]. Basically, DoDragDrop cannot be driven by touch events, so we might have to do some crazy mouse event injection to get it working properly. I'll need to play around with this more.

[1] https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/LK9Jjm3y61k
[2] https://codereview.chromium.org/14122008/

Comment 21

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

Comment 20 applies for a long-press-and-drag gesture, but not really for a double-tap-drag gesture. This is because long-press-and-drag is converted to a right-mouse-button-drag by the OS, so the simulated mouse events are WM_MOUSEMOVE with MK_RBUTTON flag. However the DoDragDrop function is only waiting for WM_MOUSEMOVE with MK_LBUTTON flag events, and so it can effectively hang waiting for the right type of event.

The problem I was running into in comment 19 seems to be that because we go straight from receiving the WM_TOUCH into DoDragDrop, the OS has not yet generated the correct mouse events to simulate the left-mouse-button-drag, and so DoDragDrop just waits. In a way it's a deadlock, because the DoDragDrop is waiting for the left-mouse-button events, but those won't get generated until after DoDragDrop returns and we unwind from the WM_TOUCH handler.

I think I might need to delay calling DoDragDrop until we know for sure that the OS is generating left-mouse-button mouse events. It's going to be a bit tricky because DoDragDrop is called from the ESM, but those mouse events are discarded in the widget code at [1], so we'll need to transfer information from the widget to the ESM somehow.

[1] http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/widget/windows/nsWindow.cpp#4173

Comment 22

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

So after some more attempts I came up with a fairly simple solution. Since Windows already detects the "double-tap" gesture and sends us a synthetic mouse double-click event, we can use that to trigger the drag-and-drop action. I'll put up the patches soon. Try try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b7b95964f7e83bdae2ff8075ffb2b1d2d4d8109 is green, but I'm trying to see if I can write a new test to cover this functionality. It's tricky since it requires native touch synthesization which doesn't always work properly on Windows.

Comment 23

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

Actually the test won't work anyway until we enable it in the content process so I might as well get the parent process version in.

Comment 24

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

Attachment: Add support for touch drag-and-drop

Comment 25

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

Attachment: (Not for landing yet) Enable drag-and-drop in content process

This is what it would take to enable it in the content process as well. I can move this to a different bug later, just putting it here as FYI.

Comment 26

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

Attachment: (Not for landing yet) Test to exercise touch-drag-and-drop

Here's a test that I got passing locally (requires both patches above). It probably won't work in automation though, since the touch event synthesization doesn't work on all platforms in automation. It also has a setTimeout. Both of these issues can be fixed by adding a way to synthesize a eMouseTouchDrag event from the test, so I'll wait until the main patch is landed before doing that. Stashing the test here for now.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8822692 [details] [diff] [review]
Add support for touch drag-and-drop

So this exposes the event to web pages in non-e10s. We don't want that, certainly not before this is in some spec.
We probably want mOnlyChromeDispatch flag set with this event. 

>+NON_IDL_EVENT(mousetouchdrag,
>+      eMouseTouchDrag,
>+      EventNameType_HTMLXUL,
>+      eMouseEventClass)
could you align the latter params under the first one

>+++ b/widget/windows/nsWindow.cpp
>@@ -4175,17 +4175,21 @@ nsWindow::DispatchMouseEvent(EventMessage aEventMessage, WPARAM wParam,
>       Telemetry::Accumulate(Telemetry::FX_TOUCH_USED, 1);
>     }
> 
>     if (mTouchWindow) {
>       // If mTouchWindow is true, then we must have APZ enabled and be
>       // feeding it raw touch events. In that case we don't need to
>       // send touch-generated mouse events to content.
>       MOZ_ASSERT(mAPZC);
>-      return result;
>+      if (aEventMessage == eMouseDoubleClick) {
>+        aEventMessage = eMouseTouchDrag;
>+      } else {
>+        return result;
>+      }
>     }
>   }

So somewhere in this method, maybe in 'switch (aEventMessage) {',  
case eMouseTouchDrag:
  event.mFlags.mOnlyChromeDispatch = true;
  break;


I guess another option is to add the event message to IsAllowedToDispatchDOMEvent() and then not add anything to nsGkAtomList.h nor EventNameList.h.
That might be better, if we don't need the event in chrome JS.

Comment 28

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

(In reply to Olli Pettay [:smaug] from comment #27)
> So this exposes the event to web pages in non-e10s. We don't want that,
> certainly not before this is in some spec.

Whoops, I keep forgetting NON_IDL_EVENT still exposes it to web pages.

> could you align the latter params under the first one

Done

> So somewhere in this method, maybe in 'switch (aEventMessage) {',  
> case eMouseTouchDrag:
>   event.mFlags.mOnlyChromeDispatch = true;
>   break;
> 
> 
> I guess another option is to add the event message to
> IsAllowedToDispatchDOMEvent() and then not add anything to nsGkAtomList.h
> nor EventNameList.h.
> That might be better, if we don't need the event in chrome JS.

Yeah, we don't need it in chrome JS so I'll try doing this instead.

Comment 29

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

Attachment: Add support for touch drag-and-drop (v2)

Updating patch as requested. I checked that it still works fine. Carrying r+.

I'm going to move the followup patches (for the content process) over to bug 1328285 and we can land them once this has baked a bit.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ead32283c57
Add support for drag-and-drop via touch (Windows-only, main-process-only). r=smaug

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

CCing stone, since he might be interested in this stuff.

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9ead32283c57

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Were you thinking about uplifting this to Aurora or letting it ride the trains?

Comment 34

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

I'd like to uplift, but it needs verification first. Petruta, could you check to see if this works as expected on the latest nightly? Thanks!

Comment 35

[Petruta Horea [:phorea], Desktop QA]

With 53.0a1 Nightly 2017-01-09 on a Microsoft Pro Surface 2 tablet, I can use the gesture described in comment 4 to drag and drop tabs with finger and pen. This gesture also drags toolbar items (bug 1299286).

The items from http://html5demos.com/drag demo cannot be dragged in latest Nightly, while this used to work in Firefox 40RC.

Comment 36

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

(In reply to Petruta Rasa [QA] [:petruta] from comment #35)
> With 53.0a1 Nightly 2017-01-09 on a Microsoft Pro Surface 2 tablet, I can
> use the gesture described in comment 4 to drag and drop tabs with finger and
> pen. This gesture also drags toolbar items (bug 1299286).

Thanks!

> The items from http://html5demos.com/drag demo cannot be dragged in latest
> Nightly, while this used to work in Firefox 40RC.

I'll fix this in bug 1328285.

Comment 37

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

Comment on attachment 8823276 [details] [diff] [review]
Add support for touch drag-and-drop (v2)

Approval Request Comment
[Feature/Bug causing the regression]: APZ touch support
[User impact if declined]: Drag-and-drop using the touch gesture will stop working on Windows chrome (e.g. dragging to rearrange tabs, or dragging to customize toolbar items)
[Is this code covered by automated tests?]: No, drag-and-drop is really hard to write automated tests for
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: see comment 4 for STR
[List of other uplifts needed for the feature/fix]: bug 1328285
[Is the change risky?]: not particularly
[Why is the change risky/not risky?]: the codepath being added is relatively isolated from existing codepaths. Changes to existing codepaths are minimal.
[String changes made/needed]: none

Comment 38

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8823276 [details] [diff] [review]
Add support for touch drag-and-drop (v2)

drag-and-drop via touch for main process, aurora52+

Comment 39

[Julien Cristau [:jcristau] (back April 22)]

Petruta would you mind verifying this and bug 1328285 when they land in aurora?  Thanks!

Comment 40

[Carsten Book [:Tomcat]]

need rebasing for aurora

Comment 41

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/7bd54083f7a1446f8df22310c6833a59aa2d96cd

Comment 42

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed with latest Aurora 52.0a2 2017-01-13 with Microsoft Surface Pro Win 10 insider Preview build, 64-bit.

Marking version 53 as verified too (comment 35).