Open Bug 1352852 Opened 7 years ago Updated 2 years ago

"dragstart" requires setting dataTransfer

Categories

(Core :: DOM: Events, defect, P2)

Product:
Core
Component:
DOM: Events
Version:
55 Branch
Type:
defect

Tracking

()

Status:
UNCONFIRMED

People

(Reporter: ivan.kuckir, Unassigned)

Details

Attachments

(1 file, 4 obsolete files)

Add dummy data to the DataTransfer when dragging a draggable object
11.05 KB, patch
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36

Steps to reproduce:

Go to https://jsfiddle.net/owLt55hp/
Try to drag the paragraph into the rectangle. It works.
The line 13 is redundant, so remove it. It does not work anymore in Firefox (but works fine in Chrome).



Expected results:

I think that dragging should work without modifying dataTransfer
Component: Untriaged → DOM: Events
Product: Firefox → Core
Michael, can you please take a look?
Flags: needinfo?(michael)
Priority: -- → P2
This is a first pass at making this work. It looks like chrome's behaviour is more spec compliant than ours so this would bring us in line by removing the "data in DataTransfer" check.

I haven't tested too much to see if this has repurcussions on other behaviours like selections yet. I'll do that before requesting review, but I don't have enough time today.

MozReview-Commit-ID: Kfzw69Epvua
Assignee: nobody → michael
Flags: needinfo?(michael)
Yup, This doesn't work. We start a Drag in the DragService, and don't start a new one correctly, however something else causes the code to break, and we don't a) render the phantom image of the DOM node or b) fire any dragover or drop events.

This specific patch also causes problems with dragging selections, but I have a local patch which fixes that by reading MozItemCount, and then only early returning if do_QueryInterface<nsIDOMHtmlElement>(aTargetContent)->GetDraggable() is true. All that's left broken as far as I can tell after that patch is the specific example from this bug (and the test - if you open the test and drag the element it doesn't act correctly). - I'll attach the updated patch.

I'm not sure what's causing the drag session to not work properly in this situation, because I don't really understand how the underlying event logic works (I've mostly worked on the DataTransfer object which is mostly distinct). Neil, do you know why this might be happening?
Flags: needinfo?(enndeakin)
This is the updated (still broken) version of the patch.
Attachment #8856731 - Attachment is obsolete: true
The drag service implementations will generally bail out if there isn't any data to drag. We could check if the specific platform allows empty data, and change this. 

Do other browsers drag no data or put some blank piece of data for dragging?
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #6)
> The drag service implementations will generally bail out if there isn't any
> data to drag. We could check if the specific platform allows empty data, and
> change this. 
> 
> Do other browsers drag no data or put some blank piece of data for dragging?

I did some testing with this jsfiddle (https://jsfiddle.net/g5w82mos/1/) - it will print "got results" if it successfully received a drop event.

Chrome -> chrome created a ghost image of the node when the node was picked up, and successfully dropped it onto the drop zone, firing a drop event.
Safari -> safari created the ghost image, but dropping it onto the drop zone did not fire an event.
Internet Explorer -> IE created the ghost image, but dropping it onto the drop zone did not fire an event.
Firefox -> We don't create the ghost image, and dropping it onto the drop zone will not fire an event.

All in all our behavior is actually consistent with the majority of the other browser vendors, other than chrome. We perhaps should update the spec to mention that if the dataTransfer is empty after the dragstart event, the drag will not occur. If we decide that we want to align with chrome (and thus what I can interpret from the current spec) we will probably need to add a dummy data type to the drag event (like application/x-moz-emptydrag) and hide it from the DataTransfer which is shown to content.
Flags: needinfo?(enndeakin)
Attachment #8857664 - Attachment is obsolete: true
It seems like we could support dragging without modifying the dataTransfer. The easiest may be just use kCustomTypesMime with nothing in it.
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #8)
> It seems like we could support dragging without modifying the dataTransfer.
> The easiest may be just use kCustomTypesMime with nothing in it.

I suppose we could add that type to the DataTransfer unconditionally?
When dragging a `draggable=true` HTML DOM node, if no data is added to the
DataTransfer during the DragStart event, we currently cancel the drag. This is
inconsistent with Chrome's behaviour.

This patch adds a chrome-only (hidden from content) item to the DataTransfer:
application/x-moz-dummy-data. This data is added only when no other data has
been added to the DataTransfer, and the target of the dragstart event was a
draggable=true HTML DOM node.

This hidden node allows for the drag event to complete successfully, while
appearing the same as Chrome's behavior to content scripts.

MozReview-Commit-ID: HVqEr7aR6DD
Attachment #8860516 - Flags: review?(enndeakin)
Comment on attachment 8860516 [details] [diff] [review]
Add dummy data to the DataTransfer when dragging a draggable object

>+      // If we didn't get any data in the dataTransfer, yet the targetContent is
>+      // draggable, we need to fire the drag event anyway. We'll add a custom
>+      // dummy data type to the DataTransfer to make this possible. This data
>+      // type will be marked as hidden so that content can't see it.

Clarify in the comment that this only applies to HTML draggable elements.


>+      uint32_t count = 0;
>+      dataTransfer->GetMozItemCount(&count);
>+      if (count == 0) {
>+        nsCOMPtr<nsIDOMHTMLElement> htmlDragTarget = do_QueryInterface(targetContent);
>+        bool draggable = false;
>+        if (htmlDragTarget &&
>+            NS_SUCCEEDED(htmlDragTarget->GetDraggable(&draggable)) ||

There is a compile warning about this line. Shouldn't this be && instead of || ? Or paranthesis around the or expression?
Note also that there is only one implementation of GetDraggable and it never fails.



>+  <title>Tests for the dragstart event</title>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css">
>+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+    ok(!ds.getCurrentSession(), "There should be no drag session in progress");
>+
>+    yield new Promise(resolve => setTimeout(resolve, 0));

Add a comment as to why this timeout is needed.


>diff --git a/widget/nsITransferable.idl b/widget/nsITransferable.idl
> #define kCustomTypesMime "application/x-moz-custom-clipdata"
> 
>+#define kDummyTypeMime "application/x-moz-dummy-data"

Maybe add a comment here explaining this (and there should be one for kCustomTypesMime too)
Attachment #8860516 - Flags: review?(enndeakin) → review-
Attachment #8860516 - Attachment is obsolete: true
Attachment #8862504 - Flags: review?(enndeakin) → review+
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/492fab8fac4e
Add dummy data to the DataTransfer when dragging a draggable object, r=enndeakin
Backout by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/525f7c248f91
Backout due to windows non-e10s test_drag_empty.html timeouts, a=backout
I'm getting windows non-e10s failures. As far as I can tell these are caused because windows hangs when we call DoDragDrop() in StartInvokingDragSession when synthesizing the mouse move events rather than responding to actual mouse move events in our non-e10s codepath. As soon as the browser receives any real input events the code seems to resume and continue running.

The code is hung at a call to PeekMessage with the arguments:

wRemoveMessage = PM_REMOVE
wMsgFilterMax = 0x100
wMsgFilterMin = 0x100
hWnd = NULL
lpMsg = &[some global memory]

I'm pretty sure that this is being caused by windows being confused by our synthesized mouse events (as they are not real mouse events like windows normally expects when doing D&D). I'm not sure how to fix the code to not hang in this place, so I'm inclined to just disable this test on non-e10s win32. How would you feel about that?
Flags: needinfo?(enndeakin)
That's fine. We don't really have a way to test drag and drop automatically.
Flags: needinfo?(enndeakin)
I've filed https://github.com/whatwg/html/issues/2642 to clarify this behavior in the spec. I think that Chrome's behavior will be specified, so I'm going to go ahead with landing it and we can back it out if spec discussions decide on some other behavior.
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0a52989a521
Add dummy data to the DataTransfer when dragging a draggable object, r=enndeakin
Backed out for timing out in dom/events/test/test_dragstart.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9769adade1ce9c93a963e0e5e340487a3d62780

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e0a52989a521e31b87299c623ed751a066456f6d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=98404793&repo=mozilla-inbound

15:16:13     INFO -  3254 INFO TEST-PASS | dom/events/test/test_dragstart.html | image text/uri-list
15:16:13     INFO -  3255 INFO TEST-PASS | dom/events/test/test_dragstart.html | image text/plain
15:16:13     INFO -  3256 INFO TEST-PASS | dom/events/test/test_dragstart.html | draggable true node
15:16:13     INFO -  Buffered messages finished
15:16:13  WARNING -  TEST-UNEXPECTED-TIMEOUT | dom/events/test/test_dragstart.html | application timed out after 330 seconds with no output
Flags: needinfo?(michael)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #19)
> Backed out for timing out in dom/events/test/test_dragstart.html:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> a9769adade1ce9c93a963e0e5e340487a3d62780
> 
> Push with failure:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=e0a52989a521e31b87299c623ed751a066456f6d&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=retry&filter-
> resultStatus=usercancel&filter-resultStatus=runnable
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=98404793&repo=mozilla-
> inbound
> 
> 15:16:13     INFO -  3254 INFO TEST-PASS |
> dom/events/test/test_dragstart.html | image text/uri-list
> 15:16:13     INFO -  3255 INFO TEST-PASS |
> dom/events/test/test_dragstart.html | image text/plain
> 15:16:13     INFO -  3256 INFO TEST-PASS |
> dom/events/test/test_dragstart.html | draggable true node
> 15:16:13     INFO -  Buffered messages finished
> 15:16:13  WARNING -  TEST-UNEXPECTED-TIMEOUT |
> dom/events/test/test_dragstart.html | application timed out after 330
> seconds with no output

This looks like the same problem I was running into with the test_drag_empty test. I am not sure why I didn't notice this failure when I was working on that test.
This test drags a draggable=true object and listens for its dragstart event. It then doesn't add any itens to the dataTransfer (in onDragStartDraggable).

This means that my code for this bug kicks in, and for some reason it isn't happy.

I'm inclined to disable it for the same reason as the test_drag_empty.html test and just make sure to manually test dragging draggable elements.
Flags: needinfo?(michael)
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/314678959de2
Add dummy data to the DataTransfer when dragging a draggable object, r=enndeakin
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd1be1a1e31b
Skip test_drag_empty.html on Win64 also a=bustage
Backed out for failing dom/events/test/test_dragstart.html on Windows 8 x64 with non-e10s:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f29ad3bbe66033b195f576506e3e24a66830899
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ae6d30e088eee31c1fc0a4cf1d39707568cc971

Failure log of push after follow-up push: https://treeherder.mozilla.org/logviewer.html#?job_id=98875437&repo=mozilla-inbound

02:41:06     INFO - TEST-PASS | dom/events/test/test_dragstart.html | initial image item count 
02:41:06     INFO - TEST-PASS | dom/events/test/test_dragstart.html | image text/uri-list 
02:41:06     INFO - TEST-PASS | dom/events/test/test_dragstart.html | image text/plain 
02:41:06     INFO - TEST-PASS | dom/events/test/test_dragstart.html | draggable true node 
02:41:06     INFO - Buffered messages finished
02:41:06     INFO - TEST-UNEXPECTED-TIMEOUT | dom/events/test/test_dragstart.html | application timed out after 330 seconds with no output
Flags: needinfo?(michael)
I think this version finally disables all of the affected tests which are now hanging on windows due to windows being weird.
Attachment #8862504 - Attachment is obsolete: true
Flags: needinfo?(michael)
Why is it OK to disable whatever failing tests? Why actual drag and drop operations will never hang?
Flags: needinfo?(michael)
(In reply to Masatoshi Kimura [:emk] from comment #25)
> Why is it OK to disable whatever failing tests? Why actual drag and drop
> operations will never hang?

Drag and drop is very hard to test due to the large amount of OS integration. We do a lot of manual testing for it. The problem here appears to be caused by the OS not receiving real mouse events in tandem with us telling it that we have mouse events, and from manual testing we know that the code works correctly.
Flags: needinfo?(michael)
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d20646799abb
Add dummy data to the DataTransfer when dragging a draggable object, r=enndeakin
Hey Anny, I was looking through some of my old needinfos & came across this bug. If you'd be interested in finishing this bug out, feel free to take it :-)
Flags: needinfo?(nika) → needinfo?(agakhokidze)
Assignee: nika → nobody
Flags: needinfo?(agakhokidze)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: