1299286 - In touch enabled laptop drag and drop by finger is not working on toolbar and customization window

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, enhancement, P1)

Description

[Abe - QA (:Abe_LV)]

Mozilla/5.0 (Windows NT 10.0; rv 51.0) Gecko/20100101 Firefox/51.0

[Pre-Requisites] 
javascript.options.asyncstack; false
browser.tabs.remote.force-enable; true
layers.async-pan-zoom.enabled; true 

about:support shows
Multiprocess Windows 1/1 (Enabled by user)
Asynchronous Pan/Zoom wheel input enabled; touch input enabled


[Steps to Reproduce]
1. Open nightly 
2. Click the menu bars 
3. Click Customize
4. Drag icon/tool by your finger from customization and drop on the toolbar(or apply the reverse). 

[Expected Results]
Drag and drop by finger should work just like using a mouse.

[Actual Results]
Drag and drop by finger does not work in touch-enabled windows laptop.

[Note]
This is not e10s specific.

Comment 1

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

Thanks, this is something we should probably get working as it would be good UX.

Comment 2

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

So drag-and-drop in the toolbars and customization menu should work on windows touch-enabled machines now. The gesture you need to use is "touchdown, touchup, touchdown, touchmove" (i.e. do a double-tap but turn the second tap into a drag instead of lifting your finger). This was implemented in bug 1147335 and applies to all of the browser chrome / stuff in the parent process.

That being said, the main action in the australis customization panel is to be dragging stuff around so I think it might make sense to use a more discoverable gesture there for better UX. This would need to be implemented directly in the JS for the australis customization panel. This part is more of an enhancement and not a regression from the touch events implementation, so I'm going to remove the blocking dependency on 1244402.

Comment 3

[Julien Cristau [:jcristau]]

Mass wontfix for bugs affecting firefox 52.

Comment 4

[Johann Hofmann [:johannh]]

Might be cool to have this for Photon.

Comment 5

[Justin Dolske [:Dolske]]

Calling high-priority to improve touch usefulness on Windows.

Comment 6

[Johann Hofmann [:johannh]]

Just a quick update before going on PTO:

I'm not sure it's worth re-implementing our current usage of the drag API in customize mode using touch events. It's a lot of code. I talked to kats about maybe exposing the event that currently natively initiates a touchdrag to privileged JS and will tinker with that idea (I'll have to see if it needs a separate bug). This would allows us to continue to use the drag API and simply trigger the touchdrag event on touch input for compatibility. I think that's worth pursuing since we could re-use this technique everywhere else.

Comment 8

[:Gijs (he/him)]

(In reply to Johann Hofmann [:johannh] from comment #6)
> Just a quick update before going on PTO:
> 
> I'm not sure it's worth re-implementing our current usage of the drag API in
> customize mode using touch events. It's a lot of code.

So I mean, I haven't looked at this in detail, but can we not just add the 3 or 4 event handlers for this, and once we get a touchstart (touchdown?) on a button, followed by touchmove that is more than 3 or 4 pixels out, run the _onDragStart and _onDragMove or whatever methods just as if it were a mouse drag, followed by onDragDrop for touchup/touchend ? That is, what makes you say "it's a lot of code" ?

Comment 9

[Johann Hofmann [:johannh]]

(In reply to :Gijs from comment #8)
> (In reply to Johann Hofmann [:johannh] from comment #6)
> > Just a quick update before going on PTO:
> > 
> > I'm not sure it's worth re-implementing our current usage of the drag API in
> > customize mode using touch events. It's a lot of code.
> 
> So I mean, I haven't looked at this in detail, but can we not just add the 3
> or 4 event handlers for this, and once we get a touchstart (touchdown?) on a
> button, followed by touchmove that is more than 3 or 4 pixels out, run the
> _onDragStart and _onDragMove or whatever methods just as if it were a mouse
> drag, followed by onDragDrop for touchup/touchend ? That is, what makes you
> say "it's a lot of code" ?

That was my first thought as well, but a DragEvent is a MouseEvent, not a TouchEvent. It has things like DataTransfer and I'm not sure what other MouseEvent specific features are used by the CustomizeMode code. We might be able to shim/avoid these things in the frontend, but since we have the ability to perfectly emulate drag events for touch events in native code already, I don't see why we should.

I haven't really had the time to really look into this so far and I'm not very experienced with the code that this likely involves. I'm happy to unassign myself from this if anyone else wants to give it a shot.

Comment 10

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

(In reply to :Gijs from comment #8)
> So I mean, I haven't looked at this in detail, but can we not just add the 3
> or 4 event handlers for this, and once we get a touchstart (touchdown?) on a
> button, followed by touchmove that is more than 3 or 4 pixels out, run the
> _onDragStart and _onDragMove or whatever methods just as if it were a mouse
> drag, followed by onDragDrop for touchup/touchend ? That is, what makes you
> say "it's a lot of code" ?

I think the piece you're missing here, is that if you just wire up the touch listeners to the drag event handlers, you're not going to get the OS drag feedback (i.e. the partial-opacity version of the thing you're dragging, stuck to your cursor as you drag around). That feedback only happens if the OS is driving the drag, which means we need to somehow start a drag session in the OS and let it manage the drag. That drag session in the OS is what triggers the dragstart/dragmove/etc. events which are then handled in the front-end code.

If you want the drag feedback (and there might be more stuff the OS does beyond just the visual feedback, I'm not sure...) then you'd have to reimplement it in JS, or something. That's where the "it's a lot of code" comes into play. At least that's my understanding of the problem so far.

Comment 11

[Johann Hofmann [:johannh]]

I've been working on an approach that hooks into https://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/widget/windows/nsWindow.cpp#4346 to switch the eventMessage to eMouseTouchDrag when the event that was hit has an attribute such as "touchdownstartsdrag". That works well, but we still need to figure out some details around ensuring that this doesn't have a big performance impact for touchdown events.

Comment 12

[Johann Hofmann [:johannh]]

(In reply to Johann Hofmann [:johannh] from comment #11)
> switch the eventMessage to eMouseTouchDrag when the event that was hit has
> an attribute such as "touchdownstartsdrag".

*the _element_ that was hit

Comment 13

[Johann Hofmann [:johannh]]

Attachment: Bug 1299286 - Enable touchdown to start dragging on elements with touchdownstartsdrag attributes.

Review commit: https://reviewboard.mozilla.org/r/196208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196208/

Comment 14

[Johann Hofmann [:johannh]]

Attachment: Bug 1299286 - Add the touchdownstartsdrag attribute to customize mode elements.

Review commit: https://reviewboard.mozilla.org/r/196210/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196210/

Comment 15

[Johann Hofmann [:johannh]]

kats, you mentioned we'll need to check that this hittest is not propagated to content processes but I'm not sure how to best do that. Currently I just ensure that we're in the default process, which might not be what we want, but it seems to stop propagating this to web content.

Performance-wise I'd say only considering touchdown events on chrome UI sounds fine to me, but I'm not sure how to validate that either.

Let me know what you think.

Thanks!

Comment 16

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

Comment on attachment 8924982 [details]
Bug 1299286 - Enable touchdown to start dragging on elements with touchdownstartsdrag attributes.

https://reviewboard.mozilla.org/r/196208/#review201480

This looks pretty good, thanks! Give me another day or so to apply the patch locally and do some more checks to make sure nothing unexpected is going on. I'll grant the r+ after I do my verification.

::: widget/windows/nsWindow.cpp:4297
(Diff revision 1)
> +  // In the UI process, allow touchdownstartsdrag attributes
> +  // to cause any touchdown event to trigger a drag.
> +  if (XRE_GetProcessType() == GeckoProcessType_Default &&
> +      aEventMessage == eMouseDown) {

Yeah I don't think we need the process check here, as this code should only ever be running in the UI process anyway. I'll apply this patch locally and test to make sure the event isn't getting dispatched into the content process.

::: widget/windows/nsWindow.cpp:4313
(Diff revision 1)
> +        node->GetAttribute(NS_LITERAL_STRING("touchdownstartsdrag"), startDrag);
> +        if (!startDrag.IsEmpty()) {
> +          return true;

This works, but only if your finger lands on the actual element with the attribute. Elements nested inside the one with the attribute will not "inherit" the attribute - I'm not sure if that's what you want or not. If you do want the inheritance behaviour, then this will need to change slightly, to walk up the element parent chain and check for the attribute on each element.

::: widget/windows/nsWindow.cpp:4376
(Diff revision 1)
>        // send touch-generated mouse events to content. The only exception is
>        // the touch-generated mouse double-click, which is used to start off the
>        // touch-based drag-and-drop gesture.

The last part of this comment needs updating, please mention that touchdown on the specially marked elements will also trigger touch-based drag-and-drop.

Comment 17

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

Comment on attachment 8924982 [details]
Bug 1299286 - Enable touchdown to start dragging on elements with touchdownstartsdrag attributes.

https://reviewboard.mozilla.org/r/196208/#review201610

Verified that it doesn't send the hit test event into the content process. The perf impact should be small as well.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8924983 [details]
Bug 1299286 - Add the touchdownstartsdrag attribute to customize mode elements.

https://reviewboard.mozilla.org/r/196210/#review201948

Comment 19

[Johann Hofmann [:johannh]]

Comment on attachment 8924982 [details]
Bug 1299286 - Enable touchdown to start dragging on elements with touchdownstartsdrag attributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196208/diff/1-2/

Comment 20

[Johann Hofmann [:johannh]]

Comment on attachment 8924983 [details]
Bug 1299286 - Add the touchdownstartsdrag attribute to customize mode elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196210/diff/1-2/

Comment 21

[Johann Hofmann [:johannh]]

Comment on attachment 8924982 [details]
Bug 1299286 - Enable touchdown to start dragging on elements with touchdownstartsdrag attributes.

kats, I feel like I've changed enough to warrant another quick review. This now walks up the tree to find the attribute on parent elements. I agree that that's probably what we want.

Thanks :)

Comment 22

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

Comment on attachment 8924982 [details]
Bug 1299286 - Enable touchdown to start dragging on elements with touchdownstartsdrag attributes.

Looks good to me, thanks!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16b3860f4f84
Enable touchdown to start dragging on elements with touchdownstartsdrag attributes. r=kats
https://hg.mozilla.org/integration/autoland/rev/ab416cb2c8db
Add the touchdownstartsdrag attribute to customize mode elements. r=jaws

Comment 24

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/16b3860f4f84
https://hg.mozilla.org/mozilla-central/rev/ab416cb2c8db

Comment 25

[Valentina Claudia Ona]

I verified this issue using Nightly 58.0a1 with Build ID 20171109220104 on Windows 10 x64 Surface Pro 4.
I will mark this as verified fixed.