873282 - Fix off-by-one error in placements code

Status: RESOLVED INVALID

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

There is some code in my patch on bug 872209 that I'd like to land separately ASAP as I believe it'll help with the SDK tests and is generally going to make DnD a little more robust, from what I've been able to test.

Comment 1

[:Gijs (he/him)]

Attachment: Patch

I believe this is a general robustness improvement which we should just take regardless of what happens with bug 872209, and I'd like to see if it fixes anything regarding the SDK tests (which were partially broken because of the ids being inside wrappers which wouldn't be direct children of the toolbar etc.)

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 750761 [details] [diff] [review]
Patch

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

On the surface, this seems to do the job nicely, but I'm concerned about the case where the nodes in the containers don't have a 1:1 mapping to the placements for the area.

Example - I've installeed an add-on that puts an item in the nav-bar customization target. The ID is now in the placement set for the nav-bar. Now I uninstall the add-on. The ID remains in the placement set for the nav-bar, and now the childnodes of the container are not mapped directly to the state for that container.

I think that's that case that relying on positions was meant to address.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Just wondering what your thoughts are on the above.

Comment 4

[:Gijs (he/him)]

Comment on attachment 750761 [details] [diff] [review]
Patch

Based on discussion with mconley on IRC, this is definitely not the right way forward here, because placements can grow out of sync with XUL nodes because of add-ons going away etc.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

So should we close this bug then?

Comment 6

[:Gijs (he/him)]

.

Comment 7

[:Gijs (he/him)]

Attachment: Patch

I still think these should be fixed as done here, because we're inserting before the node, so to get the right position, we should get the node currently at that position.

Comment 8

[:Gijs (he/him)]

(we might actually want to unify those event handlers a bit more, they seem awfully similar...)

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 751807 [details] [diff] [review]
Patch

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

This breaks the current drag and drop behaviour for me; items are just being appended to the end of the toolbar instead of going where I've indicated. See my comment below for a possible explanation.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ -398,5 @@
>                       " when adding a widget.");
>        return;
>      }
>  
> -    let nextNodeId = placements[aPosition + 1];

I don't believe this is an off-by-one error. I *think* we're doing the right thing already, since by the time onWidgetAdded is called, the placements for the area have already been updated - the ID of the widget that's being inserted has already been spliced in at aPosition.

So aPosition + 1 should be the ID of the next node, as desired.

Or am I missing something here?

Comment 10

[:Gijs (he/him)]

Per comment #9 and discussion with mconley on IRC, marking INVALID.