Dispatch an event when moving toolbaritems from/to customization dialog

NEW
Unassigned

Status

()

Firefox
General
5 years ago
4 years ago

People

(Reporter: ochameau, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 729628 [details] [diff] [review]
Bug 854962 - Dispatch an event when moving toolbaritems from/to customization dialog.
(Reporter)

Comment 2

5 years ago
I've no idea who to ask the review?
But do you think that's a reasonable request/patch?
(Reporter)

Comment 3

5 years ago
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!
Attachment #729628 - Flags: review?(gavin.sharp)
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.
Attachment #729628 - Flags: review?(gavin.sharp) → review?(jAwS)
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.
Attachment #729628 - Flags: review?(jAwS) → review-
(Reporter)

Comment 6

5 years ago
Created attachment 730753 [details] [diff] [review]
Bug 854962 - Dispatch an event when moving toolbaritems from/to customization dialog.
(Reporter)

Updated

5 years ago
Attachment #729628 - Attachment is obsolete: true
(Reporter)

Comment 7

5 years ago
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.
Attachment #730753 - Flags: review?(jAwS)
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'.
Attachment #730753 - Flags: review?(jmaher)
Attachment #730753 - Flags: review?(jAwS)
Attachment #730753 - Flags: review-
(Reporter)

Comment 10

5 years ago
(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.
(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 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.
Attachment #730753 - Flags: review?(jmaher)
(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.
(Reporter)

Comment 14

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
(Reporter)

Comment 15

5 years ago
(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.
(Reporter)

Updated

5 years ago
Assignee: nobody → poirot.alex
Depends on: 857073
(Reporter)

Comment 16

4 years ago
Looks like I won't have time to look at SDK bugs anymore.
Assignee: poirot.alex → nobody
You need to log in before you can comment on or make changes to this bug.