956602 - CustomizeMode doesn't handle well dragging special widgets into the palette

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Luís Miguel [:quicksaver] (not reading bugmail)]

STR:

1) Open Nightly in a new profile
2) Open the browser console
3) Type in "CustomizableUI.addWidgetToArea('separator', 'nav-bar')".
3.1) Separator is added to the nav-bar as expected
4) Enter customization mode
5) Drag the separator out of the toolbar into the palette

The following error message will appear in the console:
> "[CustomizeMode]" TypeError: Argument 1 of Node.insertBefore is not an object.
> Stack trace:
> CustomizeMode.prototype._applyDrop@resource://app/modules/CustomizeMode.jsm:1128
> CustomizeMode.prototype._onDragDrop@resource://app/modules/CustomizeMode.jsm:1091
> CustomizeMode.prototype.handleEvent@resource://app/modules/CustomizeMode.jsm:880

This doesn't seem to affect the placements of the nav-bar, as the separator is removed from it as soon as it's dragged out. And although this is the expected behavior (no special widgets in the palette), I suspect this may be happening by accident because it is throwing when it tries to append.

Maybe a special check for "If we're dragging a special widget to the palette, don't try to append" would be simple enough.

Comment 1

[Luís Miguel [:quicksaver] (not reading bugmail)]

Quick link to the method in question (if I'm not mistaken): http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/src/CustomizeMode.jsm#1078

Comment 2

[:Gijs (he/him)]

By default there ought not to be any separators anyway, and this error isn't fatal, so I'm downgrading this to a P3. Test + patch hopefully soon.

Comment 3

[:Gijs (he/him)]

Attachment: special widgets are removed when going to the palette; cope in Australis drop code,

This works. I wonder if we should ensure these items can't go intot go into the menupanel...

Comment 4

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to :Gijs Kruitbosch from comment #3)
> I wonder if we should ensure these items can't go intot go into
> the menupanel...

I don't know if the plan is to never have these items in the menu panel or not, but for what's worth, I got them to work (and display nicely I think) in there and in the palette: https://addons.mozilla.org/en-us/firefox/addon/the-puzzle-piece/versions/?page=1#version-1.2b1

Comment 5

[Luís Miguel [:quicksaver] (not reading bugmail)]

What I meant with that was mostly, at least not in a way that can't be overridden, please. :)

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8357108 [details] [diff] [review]
special widgets are removed when going to the palette; cope in Australis drop code,

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

r+ given the following fix

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +1106,5 @@
>          }
>  
>          CustomizableUI.removeWidgetFromArea(aDraggedItemId);
> +        // Special widgets are removed outright, we can return here:
> +        if (!draggedItem.parentNode) {

This is making an assumption - it's more correct to use CustomizableUI.isSpecialWidget() here. It *should* be the same, but it could also mask another error - if that does ever happen, we're better of knowing about it.

Comment 7

[:Gijs (he/him)]

(In reply to Blair McBride [:Unfocused] from comment #6)
> Comment on attachment 8357108 [details] [diff] [review]
> special widgets are removed when going to the palette; cope in Australis
> drop code,
> 
> Review of attachment 8357108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ given the following fix
> 
> ::: browser/components/customizableui/src/CustomizeMode.jsm
> @@ +1106,5 @@
> >          }
> >  
> >          CustomizableUI.removeWidgetFromArea(aDraggedItemId);
> > +        // Special widgets are removed outright, we can return here:
> > +        if (!draggedItem.parentNode) {
> 
> This is making an assumption - it's more correct to use
> CustomizableUI.isSpecialWidget() here. It *should* be the same, but it could
> also mask another error - if that does ever happen, we're better of knowing
> about it.

Excellent point, landed with that change:

remote:   https://hg.mozilla.org/integration/fx-team/rev/8e0ec2d5fa85

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8e0ec2d5fa85

Comment 9

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0

The mentioned error message is no longer displayed in the Error Console using Firefox 29 beta 6, build ID: 20140407135746.
Marking issue verified.