Remove stopPropagation call from ChromeUtils.synthesizeDrop.

RESOLVED FIXED in Firefox 41

Status

Testing
Mochitest
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox40 affected, firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Derived from bug 1147700.

simulateItemDrag in browser/components/customizableui/test/head.js requires calling stopPropagation in ChromeUtils.synthesizeDrop to suppress dragstart handler, since it passes dragData to overwrite dataTransfer.

https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/head.js#188 
> function simulateItemDrag(toDrag, target) {
>   let docId = toDrag.ownerDocument.documentElement.id;
>   let dragData = [[{type: 'text/toolbarwrapper-id/' + docId,
>                     data: toDrag.id}]];
>   synthesizeDragStart(toDrag.parentNode, dragData);
>   synthesizeDrop(target, target, dragData);
> }

https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizeMode.jsm#1526
>   _onDragStart: function(aEvent) {
>     __dumpDragData(aEvent);
>     let item = aEvent.target;
> ...
>     dt.mozSetDataAt(kDragDataTypePrefix + documentId, draggedItem.id, 0);
>     dt.effectAllowed = "move";
> ...

https://hg.mozilla.org/integration/mozilla-inbound/file/fa7e6ceab5f3/testing/mochitest/tests/SimpleTest/ChromeUtils.js#l208
> function synthesizeDrop(srcElement, destElement, dragData, dropEffect, aWindow, aDestWindow, aDragEvent={})
> {
> ...
>   var trapDrag = function(event) {
> ...
>     event.preventDefault();
>     if (dragData) {
>       event.stopPropagation();
>     }
>   }

but simulateItemDrag function could simulate item dragging just by calling synthesizeDrop with dragData=null (added in bug 1147700, which doesn't overwrite dataTransfer), and in that case, we can remove stopPropagation call.
(Assignee)

Comment 1

3 years ago
Created attachment 8594442 [details] [diff] [review]
Remove stopPropagation call from ChromeUtils.synthesizeDrop.

Fixed 4 issues in addition to stopPropagation.

1. https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_978084_dragEnd_after_move.js

First test doesn't end customization, so second test adds toolbar item (draggedItem) while customizing, and wrapper element is not created for it (it worked before because simulateItemDrag passes dragData, but basically it shouldn't work, am I correct?).
So added endCustomizing to both tests, which can be called multiple times (it checks customizing, and returns if not).


2. https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_1003588_no_specials_in_panel.js

checkDragging function needs dragend event, to move separator/spring/spacer back to toolbar, so added it to synthesizeDrop.


3. https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_962069_drag_to_overflow_chevron.js

This doesn't test a behavior for drop event, but dragover event (panel is shown only in drag session), so it needs to wait for panelShownPromise before ending drag session (this worked before because synthesizeDrop didn't send dragend).
So I splitted two logics out from synthesizeDrop, to synthesizeDragOver and synthesizeDropOrEnd. Those function should be called inside drag session.
Also, reformatted documentation comments in ChromeUtils.js match to rest of file.


4. https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_995164_registerArea_during_customize_mode.js

Sync button in palette may outside of client area, if palette has many items and window size is small (happened on Windows xp test, there are a lot of test buttons before Sync button). so added scrollIntoView to make it visible and draggable.


For other cases, just changing simulateItemDrag to call synthesizeDrop with dragData=null works.


Green on try runs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e776b19ffb9d (windows)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c06ad0f97861 (linux + osx)
Assignee: nobody → arai.unmht
Attachment #8594442 - Flags: review?(enndeakin)

Comment 2

3 years ago
Comment on attachment 8594442 [details] [diff] [review]
Remove stopPropagation call from ChromeUtils.synthesizeDrop.

>+/**
>+ * Emulate a event sequence of dragstart, dragenter, and dragover.
>+ * This should be called inside drag session.

It doesn't need to be called inside a drag session, but you may want to add documentation saying that it may be.


>+ * @return              An array contains the returned value from
>+ *                      EventUtils.sendDragEvent for dragover event,
>+ *                      and dataTransfer for current drag session.

Clarify that a two element array is returned with the two values...


>+function synthesizeDropOrEnd(aResult, aDataTransfer, aDestElement, aDestWindow, aDragEvent={})
>+{
>+  if (!aDestWindow)
>+    aDestWindow = window;
>+
>+  var effect = aDataTransfer.dropEffect;
>+  var event;
>+
>+  if (aResult) {
>+    event = createDragEventObject("dragend", aDestElement, aDestWindow,
>+                                  aDataTransfer, aDragEvent);
>+    EventUtils.sendDragEvent(event, aDestElement, aDestWindow);

The dragend event fires on the source element not the destination element. It should also always fire even when the drop is successful. But I don't think you should send a dragend event when synthesizeDrop is used since you we aren't synthesizing a full drag and drop, just a drop.
Attachment #8594442 - Flags: review?(enndeakin) → review-
(Assignee)

Comment 3

3 years ago
Created attachment 8597616 [details] [diff] [review]
Remove stopPropagation call from ChromeUtils.synthesizeDrop.

Thank you for reviewing!

(In reply to Neil Deakin from comment #2)
> Comment on attachment 8594442 [details] [diff] [review]
> Remove stopPropagation call from ChromeUtils.synthesizeDrop.
> 
> >+/**
> >+ * Emulate a event sequence of dragstart, dragenter, and dragover.
> >+ * This should be called inside drag session.
> 
> It doesn't need to be called inside a drag session, but you may want to add
> documentation saying that it may be.

removed the line.

> >+ * @return              An array contains the returned value from
> >+ *                      EventUtils.sendDragEvent for dragover event,
> >+ *                      and dataTransfer for current drag session.
> 
> Clarify that a two element array is returned with the two values...

Fixed. Let me know if my wording is still unclear.

> >+function synthesizeDropOrEnd(aResult, aDataTransfer, aDestElement, aDestWindow, aDragEvent={})
> >+{
> >+  if (!aDestWindow)
> >+    aDestWindow = window;
> >+
> >+  var effect = aDataTransfer.dropEffect;
> >+  var event;
> >+
> >+  if (aResult) {
> >+    event = createDragEventObject("dragend", aDestElement, aDestWindow,
> >+                                  aDataTransfer, aDragEvent);
> >+    EventUtils.sendDragEvent(event, aDestElement, aDestWindow);
> 
> The dragend event fires on the source element not the destination element.
> It should also always fire even when the drop is successful. But I don't
> think you should send a dragend event when synthesizeDrop is used since you
> we aren't synthesizing a full drag and drop, just a drop.

Okay, currently only one test (browser_1003588_no_specials_in_panel.js) needs dragend, so moved it there.
Attachment #8594442 - Attachment is obsolete: true
Attachment #8597616 - Flags: review?(enndeakin)

Comment 4

3 years ago
Comment on attachment 8597616 [details] [diff] [review]
Remove stopPropagation call from ChromeUtils.synthesizeDrop.

>+  ds.startDragSession();
>+  try {
>+    var [result, dataTransfer] = ChromeUtils.synthesizeDragOver(aToDrag.parentNode, aTarget);
>+    // Send dragend to move dragging item back to initial place.
>+    EventUtils.sendDragEvent({ type: "dragend", dataTransfer: dataTransfer },
>+                             aToDrag.parentNode);
>+    ChromeUtils.synthesizeDropAfterDragOver(result, dataTransfer, aTarget);

Dragend fires after the drop event.



>+ * @return              A two elements array, its 1st element is the value
>+ *                      returned from EventUtils.sendDragEvent for dragover
>+ *                      event, and 2nd element is dataTransfer for current drag
>+ *                      session.

Change to read:

"A two element array, where the first"

and 

"and the second element is the dataTransfer for the current drag session."
Attachment #8597616 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/mozilla-central/rev/1c91f5d39dea
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.