Open Bug 1352852 Opened 4 years ago Updated 2 years ago
"dragstart" requires setting data
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
Duplicate of this bug: 1352974
Michael, can you please take a look?
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
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?
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?
(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.
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.
(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)
Attachment #8860516 - Flags: review?(enndeakin) → review-
Attachment #8860516 - Attachment is obsolete: true
Pushed by email@example.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 firstname.lastname@example.org: 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?
That's fine. We don't really have a way to test drag and drop automatically.
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 email@example.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
(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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/314678959de2 Add dummy data to the DataTransfer when dragging a draggable object, r=enndeakin
Pushed by email@example.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
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
Why is it OK to disable whatever failing tests? Why actual drag and drop operations will never hang?
(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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/d20646799abb Add dummy data to the DataTransfer when dragging a draggable object, r=enndeakin
Backing this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=105641927&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/53cdf23f7d183ee68dee237efc074d15cc299f77
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)
You need to log in before you can comment on or make changes to this bug.