878452 - Dragging an item from the palette to the panel (not to an item in the panel) doesn't work when the panel is empty.

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

STR:
1. Clear the panel by moving everything out of it into the palette.
2. Try to drag something back in.

Expected: works.
Actual: doesn't! :-)

I'll take this and force myself to do an actual dnd thing for testing...

Comment 1

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

The patches I'm working on for bug 877370 should fix this.

Comment 2

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

Of course, smaller patches are better. I'd be happy to rebase on top of your work.

Comment 3

[:Gijs (he/him)]

Attachment: Patch with tests

This is a minimal patch which fixes this issue and adds tests. :-)

(based on the latest test framework stuff after the patches from bug 877178 land)

Comment 4

[:Gijs (he/him)]

Also, I filed bug 878762 for an issue with the add/move/remove APIs when in customization mode. They are designed to be used when not in customization mode, and when the customization mode calls them it takes care to unwrap the nodes and then wrap them again. This is why the last test in the file (which is the one which doesn't work without the null check in _setDragActive) is doing its emptying of the panel before entering customization mode.

Separately, I am concerned about not seeing any errors in the console from the _setDragActive code... should we wrap the contents of handleEvent in a try catch and report errors ourselves? Or was this only because it was a mochitest and would I have seen things in the browser console?

Comment 5

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

(In reply to :Gijs Kruitbosch from comment #4)
> Separately, I am concerned about not seeing any errors in the console from
> the _setDragActive code... should we wrap the contents of handleEvent in a
> try catch and report errors ourselves? Or was this only because it was a
> mochitest and would I have seen things in the browser console?

The LOG calls in the ondrag* functions that are called by handleEvent are working. I do think there is an issue with the logging of exceptions being thrown from within handleEvent.

It may be a good idea to do like so:

handleEvent: function(aEvent) {
  try {
    ...
  } catch (ex) {
    LOG(ex);
    throw ex;
  }
},

We should also file a bug to fix the root cause of exceptions not being logged from within event handlers.

Comment 6

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

Comment on attachment 757363 [details] [diff] [review]
Patch with tests

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

::: browser/components/customizableui/test/browser_878452_drag_to_panel.js
@@ +10,5 @@
> +      let btn = document.getElementById("developer-button");
> +      let panel = document.getElementById(CustomizableUI.AREA_PANEL);
> +      let placements = getAreaWidgetIds(CustomizableUI.AREA_PANEL);
> +
> +      let lastButtonIndex = placements.length -1;

nit, spaces on both sides of '-'.

Comment 7

[:Gijs (he/him)]

Pushed: https://hg.mozilla.org/projects/ux/rev/e878799f7d5a

Comment 8

[:Gijs (he/him)]

Backed out (https://hg.mozilla.org/projects/ux/rev/65fde4f8d09b) because of privatebrowsing test failures, see bug 879098.

Comment 9

[:Gijs (he/him)]

Pushed again with test fixes: https://hg.mozilla.org/projects/ux/rev/1a274e647119

Comment 10

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/1a274e647119

Comment 11

[Alexandre Magno]

Is it related to bug 1005677?

Comment 12

[:Gijs (he/him)]

(In reply to Alexandre Magno from comment #11)
> Is it related to bug 1005677?

No.