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

VERIFIED FIXED in Firefox 58

Status

()

enhancement
P1
normal
VERIFIED FIXED
3 years ago
2 months ago

People

(Reporter: Abe_LV, Assigned: johannh)

Tracking

(Blocks 1 bug)

51 Branch
Firefox 58
x86_64
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox52 wontfix, firefox58 verified)

Details

(Whiteboard: [reserve-photon-visual][p4][high-priority], )

Attachments

(2 attachments)

Reporter

Description

3 years ago
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.
Reporter

Updated

3 years ago
Flags: needinfo?(bugmail)
Thanks, this is something we should probably get working as it would be good UX.
Blocks: 1244402
Flags: needinfo?(bugmail)
Severity: normal → enhancement
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.
No longer blocks: 1244402
Depends on: 1147335
Mass wontfix for bugs affecting firefox 52.
Assignee

Comment 4

2 years ago
Might be cool to have this for Photon.
Blocks: photon-touch
Whiteboard: [photon-visual][triage]
Flags: qe-verify+
Priority: -- → P3
QA Contact: brindusa.tot
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
Whiteboard: [reserve-photon-visual] → [reserve-photon-visual][p4]
Calling high-priority to improve touch usefulness on Windows.
Whiteboard: [reserve-photon-visual][p4] → [reserve-photon-visual][p4][high-priority]
Assignee

Updated

2 years ago
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Iteration: --- → 57.1 - Aug 15
Priority: P3 → P1
Iteration: 57.1 - Aug 15 → 57.2 - Aug 29
Assignee

Comment 6

2 years ago
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.
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19

Updated

2 years ago
Duplicate of this bug: 1400018

Comment 8

2 years ago
(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" ?
Flags: needinfo?(jhofmann)
Assignee

Comment 9

2 years ago
(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.
Flags: needinfo?(jhofmann)
(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.
Iteration: 57.3 - Sep 19 → ---
Assignee

Updated

2 years ago
Blocks: 1362065
See Also: 1362065
Assignee

Comment 11

2 years ago
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.
Assignee

Comment 12

2 years ago
(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
Assignee

Comment 15

2 years ago
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

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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.
Attachment #8924982 - Flags: review?(bugmail) → review+
Comment on attachment 8924983 [details]
Bug 1299286 - Add the touchdownstartsdrag attribute to customize mode elements.

https://reviewboard.mozilla.org/r/196210/#review201948
Attachment #8924983 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 21

2 years ago
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 :)
Attachment #8924982 - Flags: review+ → review?(bugmail)
Comment on attachment 8924982 [details]
Bug 1299286 - Enable touchdown to start dragging on elements with touchdownstartsdrag attributes.

Looks good to me, thanks!
Attachment #8924982 - Flags: review?(bugmail) → review+

Comment 23

2 years ago
Pushed by jhofmann@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/16b3860f4f84
https://hg.mozilla.org/mozilla-central/rev/ab416cb2c8db
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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.
Status: RESOLVED → VERIFIED

Updated

2 months ago
Depends on: 1539117
You need to log in before you can comment on or make changes to this bug.