854962 - Dispatch an event when moving toolbaritems from/to customization dialog

Status: NEW

(Firefox :: General, defect)

Description

[Alexandre Poirot [:ochameau]]

Playing with toolbox customization dialog is hard.
We have to juggle a lot in order to implement the widget API in the addon SDK.

Here I'm suggesting to dispatch a custom DOM event, called ToolbaritemMove, when the user drag and drop a toolbaritem element from/to toolbar/dialog.
It will help us setup/destroy iframe hosted in SDK's widgets that are injected into the toolbaritems.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 854962 - Dispatch an event when moving toolbaritems from/to customization dialog.

Comment 2

[Alexandre Poirot [:ochameau]]

I've no idea who to ask the review?
But do you think that's a reasonable request/patch?

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 729628 [details] [diff] [review]
Bug 854962 - Dispatch an event when moving toolbaritems from/to customization dialog.

Looks like you were the last reviewer of this file.
Feel free to forward the review to whoever makes sense!

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 729628 [details] [diff] [review]
Bug 854962 - Dispatch an event when moving toolbaritems from/to customization dialog.

Seems reasonable.

We're in the process of re-writing customization in bug 770135, we should make sure the add-on SDK's use cases are covered by that work.

Comment 5

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

Comment on attachment 729628 [details] [diff] [review]
Bug 854962 - Dispatch an event when moving toolbaritems from/to customization dialog.

Review of attachment 729628 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like this only dispatches the event when there is a toolbar drop. You'll need to add the same code to onPaletteDrop.

::: toolkit/content/customizeToolbar.js
@@ +772,5 @@
> +  let event = document.createEvent("CustomEvent");
> +  event.initCustomEvent("ToolbaritemMove", true, true, {
> +    destination: draggedPaletteWrapper ? "palette" : "toolbar"
> +  });
> +  toolbarItem.dispatchEvent(event);

Can you add an automated test for this? As Gavin said, we're in the midst of rewriting our customization and I don't want us to break this for you since it might be easy to go unnoticed.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 854962 - Dispatch an event when moving toolbaritems from/to customization dialog.

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 730753 [details] [diff] [review]
Bug 854962 - Dispatch an event when moving toolbaritems from/to customization dialog.

Same patch, with tests and event being dispatched on palette drop.

I also patched chromeUtils in order to stop having to pass EventUtils in each of its method...
And added popupClone in the event in order to pass the clone being created of the toolbaritem in the customization dialog.

Comment 8

[Alexandre Poirot [:ochameau]]

https://tbpl.mozilla.org/?tree=Try&rev=11bbf1be2cd8

Comment 9

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

Comment on attachment 730753 [details] [diff] [review]
Bug 854962 - Dispatch an event when moving toolbaritems from/to customization dialog.

Review of attachment 730753 [details] [diff] [review]:
-----------------------------------------------------------------

This is really close, but r- since we should have tests cover both directions and I'm not sure about the removal of the preventDefault/stopPropagation.

Joel, can you review the /testing/ changes?

::: browser/base/content/test/browser_customize_bug854962.js
@@ +33,5 @@
> +        chromeUtils.synthesizeDrop(popupNode, navigationBar, [], null, popupWindow, null, aWindow);
> +      }, 0);
> +    }
> +    else if (count == 2) {
> +      is(event.detail.destination, "toolbar", "event destination is toolbar");

Can you also include a drag from the toolbar to the palette?

::: testing/mochitest/tests/SimpleTest/ChromeUtils.js
@@ +4,5 @@
>   * mochitest-chrome and browser-chrome tests.  Originally these functions were in 
>   * EventUtils.js, but when porting to specialPowers, we didn't want
>   * to move unnecessary functions.
>   *
>   * ChromeUtils.js depends on EventUtils.js being loaded.

These comments are now out-of-date.

@@ -240,5 @@
>        }
>      }
>      dataTransfer.dropEffect = dropEffect || "move";
> -    event.preventDefault();
> -    event.stopPropagation();

Why are these removed?

::: toolkit/content/customizeToolbar.js
@@ +776,5 @@
> +// from/to palette/toolbar.
> +function dispatchMoveEvent(toolbaritem, destination, popupClone) {
> +  var event = toolbaritem.ownerDocument.createEvent("CustomEvent");
> +  event.initCustomEvent("ToolbaritemMove", true, true, {
> +    // destination is either: toolbar or popup

We should use the name 'palette' instead of 'popup'.

Comment 10

[Alexandre Poirot [:ochameau]]

(In reply to Jared Wein [:jaws] (Vacation 3/30 to 4/7) from comment #9)
> ::: browser/base/content/test/browser_customize_bug854962.js
> @@ +33,5 @@
> > +        chromeUtils.synthesizeDrop(popupNode, navigationBar, [], null, popupWindow, null, aWindow);
> > +      }, 0);
> > +    }
> > +    else if (count == 2) {
> > +      is(event.detail.destination, "toolbar", "event destination is toolbar");
> 
> Can you also include a drag from the toolbar to the palette?

That's already included. I listen for two events, the first one is toolbar to palette,
the second one is palette to toolbar.

> @@ -240,5 @@
> >        }
> >      }
> >      dataTransfer.dropEffect = dropEffect || "move";
> > -    event.preventDefault();
> > -    event.stopPropagation();
> 
> Why are these removed?

It looks really wrong to do that, I'm really why we were doing that.
It prevent from receiving the dragstart event and prevent to receive it in customizeToolbar code.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to Alexandre Poirot (:ochameau) from comment #7)
> I also patched chromeUtils in order to stop having to pass EventUtils in
> each of its method...

You shouldn't need to touch chromeUtils at all, or use it in this test - browser chrome tests have "EventUtils" global in scope that you can use directly.

(The test should also have a license header.)

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 730753 [details] [diff] [review]
Bug 854962 - Dispatch an event when moving toolbaritems from/to customization dialog.

Review of attachment 730753 [details] [diff] [review]:
-----------------------------------------------------------------

I am unclear why iwas added as a reviewer for this.  I am familiar with the ChromeUtils.js code and I find the changes just fine with the exception of the previous comment by jAwS asking why we removed event preventDefault() and stopPropegation().

The code regarding the testcase or customizeToolbar.js are all very unfamiliar to me.

Comment 13

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

(In reply to Alexandre Poirot (:ochameau) from comment #10)
> (In reply to Jared Wein [:jaws] (Vacation 3/30 to 4/7) from comment #9)
> > Can you also include a drag from the toolbar to the palette?
> 
> That's already included. I listen for two events, the first one is toolbar
> to palette, the second one is palette to toolbar.

Sorry, I misread it. I see it now.

(In reply to Joel Maher (:jmaher) from comment #12)
> Comment on attachment 730753 [details] [diff] [review]
> Bug 854962 - Dispatch an event when moving toolbaritems from/to
> customization dialog.
> 
> Review of attachment 730753 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am unclear why iwas added as a reviewer for this.  I am familiar with the
> ChromeUtils.js code and I find the changes just fine with the exception of
> the previous comment by jAwS asking why we removed event preventDefault()
> and stopPropegation().
> 
> The code regarding the testcase or customizeToolbar.js are all very
> unfamiliar to me.

I added you as a reviewer specifically because it touched ChromeUtils.js. If the eventual patch needs to change something in ChromeUtils.js (which comment #11 says is unnecessary), then it should get reviewed by a /testing/ peer.

Comment 14

[Alexandre Poirot [:ochameau]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #11)
> (In reply to Alexandre Poirot (:ochameau) from comment #7)
> > I also patched chromeUtils in order to stop having to pass EventUtils in
> > each of its method...
> 
> You shouldn't need to touch chromeUtils at all, or use it in this test -
> browser chrome tests have "EventUtils" global in scope that you can use
> directly.
> 
> (The test should also have a license header.)

Actually, I do want to use ChromeUtils and its synthesizeDrop method.

Comment 15

[Alexandre Poirot [:ochameau]]

(In reply to Joel Maher (:jmaher) from comment #12)
> Comment on attachment 730753 [details] [diff] [review]
> Bug 854962 - Dispatch an event when moving toolbaritems from/to
> customization dialog.
> 
> Review of attachment 730753 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am unclear why iwas added as a reviewer for this.  I am familiar with the
> ChromeUtils.js code and I find the changes just fine with the exception of
> the previous comment by jAwS asking why we removed event preventDefault()
> and stopPropegation().

What is the rational behind preventDefault/stopPropagation in dragstart ?
We *do want* this event, but here we trap and cancel it, so that code that actually listen to drag events won't receive the drag start and break.

Comment 16

[Alexandre Poirot [:ochameau]]

Looks like I won't have time to look at SDK bugs anymore.