Closed Bug 1328285 Opened 3 years ago Closed 3 years ago

Support touch-based drag and drop gestures in content (on Windows)

Categories

(Core :: Drag and Drop, defect)

53 Branch
Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- unaffected
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files)

See bug 1147335 comment 4 onwards. Prior to turning on APZ for touch on Windows, there was a (IMO pretty undiscoverable) gesture that allowed doing drag-and-drop by touch. The gesture was "touchdown, touchup, touchdown, touchmove, .... touchup". This used to work both in the chrome process (e.g. for dragging to rearrange tabs) and in content (e.g. on http://html5demos.com/drag). Turning on APZ for touch broke this behaviour. The patch in bug 1147335 fixes it for the chrome process. This bug is to fix it for the content process. I already have a patch (one-liner that builds on top of bug 1147335) that does this, but smaug wanted to let that bug bake a little before landing this, so I'm filing is as a follow-up.
Attached patch Part 1 - PatchSplinter Review
This is to enable the drag-and-drop gesture in the content process as well.
Needs more work (see XXX comment in the patch).
I'm having a lot of trouble with this test. I forgot that even after solving the synthetic eMouseTouchDrag event problem, ::DoDragDrop still requires mousemove events to be coming from the OS. If I synthesize touch events from the test, and InitializeTouchInjection fails (which it does on Windows 7 VM automation platform) then the touch events won't drive the ::DoDragDrop function and the test hangs. So the options are to (a) make the test run on Windows8 only in automation or (b) synthesize mouse events instead of touch events from the test.

I'd rather do (a) than (b) because (b) is and not really representative of what the code is doing. If I can assume that we have native touch injection though, then synthesizing the eMouseTouchDrag becomes kind of unnecessary, since the OS should be generating it properly. I have a try push at [1] with the test as attached. It's failing on Win 7 because of the failure to initialize touch injection. On Win 8 it gets further but still seems to hang right at the end for some reason. I can't reproduce that locally, but I'll try a few things.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=87eabe464046a1bf69c025e60e5a15d85efd7d04
Comment on attachment 8823281 [details] [diff] [review]
Part 1 - Patch

I'm really not making much progress with figuring out why the test is not working in automation. In order to debug this I'd basically need access to windows source code (specifically of ::DoDragDrop). I think at this point I'd rather land the patch without the test since it seems to work fine in practice. What do you think, smaug?
Attachment #8823281 - Flags: review?(bugs)
Comment on attachment 8823281 [details] [diff] [review]
Part 1 - Patch

I know testing dnd properly is really hard or often impossible given our current testing infrastructure (perhaps using Marionette would work in some cases?).
Attachment #8823281 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #5)
> (perhaps using Marionette would work in some
> cases?).

That's a good thought. I looked through the existing Marionette tests and the marionette harness code but unfortunately it also doesn't do OS-level event injection, so it wouldn't properly exercise this code. :(
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0759511f24e1
Add support for drag-and-drop via touch in Windows content processes as well. r=smaug
https://hg.mozilla.org/mozilla-central/rev/0759511f24e1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8823281 [details] [diff] [review]
Part 1 - Patch

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 web content in Windows
[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?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: yes, comment 0 has STR.
[List of other uplifts needed for the feature/fix]: requires bug 1147335
[Is the change risky?]: not particularly
[Why is the change risky/not risky?]: one-line change to allow an event to propagate into the content process. the event is newly added in bug 1147335 and should not affect pre-existing codepaths.
[String changes made/needed]: none
Attachment #8823281 - Flags: approval-mozilla-aurora?
Comment on attachment 8823281 [details] [diff] [review]
Part 1 - Patch

drag-and-drop via touch for content process, aurora52+
Attachment #8823281 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
^ That's going to break the build without bug 1147335. I'll rebase and uplift bug 1147335 now.
Tested with http://html5demos.com/drag demo using latest Nightly 53.0a1 and Aurora 52.0a2 (*forced e10s*) on a Microsoft Surface Pro tablet with Win 10 64-bit.

Results:
- dragging the items only works if the second tap is performed really quickly after the first one (if ~1 second passes, the item cannot be dragged anymore). This worked fine in Firefox 45. 
- 'Tap and drag' action doesn't work. This used to work in Firefox 45 and Aurora 52.0a2 non-e10s (e10s is disabled by default by accessibility tools).
(In reply to Petruta Rasa [QA] [:petruta] from comment #13)
> - 'Tap and drag' action doesn't work. This used to work in Firefox 45 and
> Aurora 52.0a2 non-e10s (e10s is disabled by default by accessibility tools).

I'm able to reproduce this. What I suspect is happening here is that the accessibility tools (which are automatically enabled on Windows touch devices) are providing the tap-and-drag functionality. That explains why this only works when e10s is disabled by accessibility tools, and not if e10s is force-enabled. I'll try to verify this by seeing if there's a way to force-disable the accessibility tools, and leave e10s off at the same time. If the tap-and-drag action stops working in that scenario, then it must be provided by the accessibility tools.

> - dragging the items only works if the second tap is performed really
> quickly after the first one (if ~1 second passes, the item cannot be dragged
> anymore). This worked fine in Firefox 45. 

Hm, I tried in FF 51 (current release) with e10s enabled and was not able to reproduce this. In my testing the allowed delay was around 500ms between the first tap and the second tap, which is what I would expect - same as in Nightly. However, in the default FF 51 configuration, accessibility tools is enabled and e10s is disabled. In that scenario, you can wait as long as you want before doing the second tap, because what's really happening is a "single-tap/drag" action (the same one discussed in the first half of this comment).
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> (In reply to Petruta Rasa [QA] [:petruta] from comment #13)
> > - 'Tap and drag' action doesn't work. This used to work in Firefox 45 and
> > Aurora 52.0a2 non-e10s (e10s is disabled by default by accessibility tools).
> 
> I'm able to reproduce this. What I suspect is happening here is that the
> accessibility tools (which are automatically enabled on Windows touch
> devices) are providing the tap-and-drag functionality. That explains why
> this only works when e10s is disabled by accessibility tools, and not if
> e10s is force-enabled. I'll try to verify this by seeing if there's a way to
> force-disable the accessibility tools, and leave e10s off at the same time.
> If the tap-and-drag action stops working in that scenario, then it must be
> provided by the accessibility tools.

So it seems like this feature is *not* coming from the accessibility tools, as it happens even if accessibility.force_disabled=1 is set, and about:support shows the accessibility tools are disabled. I'll file a new bug for this regression. Petruta - can you confirm that you see this as a general e10s regression? The 'tap and drag' seems to work whenever e10s is off, but doesn't work when e10s is on, even if APZ and touch events are turned off (layers.async-pan-zoom.enabled=false and/or dom.w3c_touch_events.enabled=0).
Flags: needinfo?(petruta.rasa)
Tap-and-drag works for me too with APZ + Touch events turned off and Accessibility tools disabled. 

Setting accessibility.force_disabled to 1 and restarting the browser doesn't enable e10s, but after force-enabling it, tap-and-drag no longer works.
Flags: needinfo?(petruta.rasa)
Thanks, I filed bug 1331649 for it.

Also I just realized that I was testing in 51 beta, not 51 release (since 51 hasn't been released yet). I don't think it matters but just wanted to make a note of that.
You need to log in before you can comment on or make changes to this bug.