996635 - Non-widget nodes can be inserted into toolbars anyway

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

STR:

0) Open browser console
1) input to confirm initial parent is 'navigator-toolbox'
> document.getElementById('addon-bar').parentNode.id

2) try to move add-on bar as a widget to an area
> CustomizableUI.addWidgetToArea('addon-bar', 'TabsToolbar')
2.1) this will throw
> "[CustomizableUI]" "Widget 'addon-bar' not found, unable to move"

3) BUT it did add it to the toolbar's placements! input in console:
> CustomizableUI.getPlacementOfWidget('addon-bar')
will return
> Object { area: "TabsToolbar", position: 4 }

4) try to add it again but to another toolbar
> CustomizableUI.addWidgetToArea('addon-bar', 'nav-bar')

5) this won't throw and it will actually move the node! input in console:
> document.getElementById('addon-bar').parentNode.id
will return
> 'nav-bar-customization-target'

Hint: bug 996571 ;) in short, isWidgetRemovable returns true by default for everything, so "things" proceed as if they actually are. My bug 996571 comment 6 is one way to go to fix this.

Comment 1

[:Gijs (he/him)]

This is actually because onWidgetRemoved doesn't check if the node it document.getElementByIds is actually inside the area it's supposed to be (or in any area, really) and then moves it to the palette, where it is duly found by findWidgetInWindow.

Comment 2

[:Gijs (he/him)]

Attachment: fix removing widgets from outside their area,

w/ test, this seems to be sufficient, and non-crazy. On the other hand, this will mean that if the location of the DOM node doesn't match its area, widgets don't get removed. That... might be bad, if our state ever gets out of sync? Then again, that shouldn't normally happen...

Comment 3

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

Comment on attachment 8421242 [details] [diff] [review]
fix removing widgets from outside their area,

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

::: browser/components/customizableui/test/browser_996635_remove_non_widgets.js
@@ +15,5 @@
> +  CustomizableUI.addWidgetToArea(buttonID, CustomizableUI.AREA_TABSTRIP);
> +  let placement = CustomizableUI.getPlacementOfWidget(buttonID);
> +  ok(placement, "Button should be placed");
> +  is(placement && placement.area, CustomizableUI.AREA_TABSTRIP, "Should be placed on tabstrip.");
> +  is(btn.parentNode && btn.parentNode.id, "nav-bar", "Actual button should still be on navbar.");

Why are we OK with CustomizableUI reporting incorrect state? CUI says it is in the tabstrip, but in reality it's in the navbar.

Comment 4

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Comment on attachment 8421242 [details] [diff] [review]
> fix removing widgets from outside their area,
> 
> Review of attachment 8421242 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> browser/components/customizableui/test/browser_996635_remove_non_widgets.js
> @@ +15,5 @@
> > +  CustomizableUI.addWidgetToArea(buttonID, CustomizableUI.AREA_TABSTRIP);
> > +  let placement = CustomizableUI.getPlacementOfWidget(buttonID);
> > +  ok(placement, "Button should be placed");
> > +  is(placement && placement.area, CustomizableUI.AREA_TABSTRIP, "Should be placed on tabstrip.");
> > +  is(btn.parentNode && btn.parentNode.id, "nav-bar", "Actual button should still be on navbar.");
> 
> Why are we OK with CustomizableUI reporting incorrect state? CUI says it is
> in the tabstrip, but in reality it's in the navbar.

CustomizableUI.addWidgetToArea("ex-parrot", "nav-bar");
let p = CustomizableUI.getPlacementOfWidget("ex-parrot");
p.area; // "nav-bar"

So, to put it briefly, placements just indicate where we think widgets ought to go if they exist. In this case, no widget exists (even if there's some other node with the same ID, it's not a widget) and so its placement is all fiction. This was the same with the currentset attribute - there could be all manner of since-removed (ex-parrot...) junk in the placements in an area. This also allows add-ons to get their button back in the same place if it's not there on window load (e.g. enabling/disabling restartless add-ons).

The panelUI-button *isn't* actually in the customizetarget part of the navbar, so I disagree with your assertion "in reality it's in the navbar". I can add comments to the test to make this more clear, though. :-)

Comment 5

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

(In reply to :Gijs Kruitbosch from comment #4)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> > Comment on attachment 8421242 [details] [diff] [review]
> > fix removing widgets from outside their area,
> > 
> > Review of attachment 8421242 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > :::
> > browser/components/customizableui/test/browser_996635_remove_non_widgets.js
> > @@ +15,5 @@
> > > +  CustomizableUI.addWidgetToArea(buttonID, CustomizableUI.AREA_TABSTRIP);
> > > +  let placement = CustomizableUI.getPlacementOfWidget(buttonID);
> > > +  ok(placement, "Button should be placed");
> > > +  is(placement && placement.area, CustomizableUI.AREA_TABSTRIP, "Should be placed on tabstrip.");
> > > +  is(btn.parentNode && btn.parentNode.id, "nav-bar", "Actual button should still be on navbar.");
> > 
> > Why are we OK with CustomizableUI reporting incorrect state? CUI says it is
> > in the tabstrip, but in reality it's in the navbar.
> 
> The panelUI-button *isn't* actually in the customizetarget part of the
> navbar, so I disagree with your assertion "in reality it's in the navbar". I
> can add comments to the test to make this more clear, though. :-)

But I wasn't talking about the "PanelUI-button". And calling CustomizableUI.getPlacementOfWidget("PanelUI-button") returns null. I understand the reasoning behind this, but I'm confused by your use of "PanelUI-button" here.

This test should have clearer comments explaining the disconnect here. It's not uncommon for people to use tests as a guide of how to use APIs, and we should be explicit here that this test shows one way that the API reports results different than reality.

Comment 6

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> (In reply to :Gijs Kruitbosch from comment #4)
> > 
> > The panelUI-button *isn't* actually in the customizetarget part of the
> > navbar, so I disagree with your assertion "in reality it's in the navbar". I
> > can add comments to the test to make this more clear, though. :-)
> 
> But I wasn't talking about the "PanelUI-button". And calling
> CustomizableUI.getPlacementOfWidget("PanelUI-button") returns null. I
> understand the reasoning behind this, but I'm confused by your use of
> "PanelUI-button" here.

Egh, sorry. I forgot that I switched away from the panelui-button itself, and instead put a dummy thing in there and then removed it again. I'll add the comments before landing.

Comment 7

[:Gijs (he/him)]

Marco, can you put this in the backlog as well, please? :-)

Comment 8

[Marco Mucci [:MarcoM]]

Added to Iteration 32.2

Comment 9

[:Gijs (he/him)]

w/ comments

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

Comment 10

[:Gijs (he/him)]

Of course, it wasn't that easy... I thought I tested this against all our tests, but I must have not done it properly one way or another.

Backed out as remote:   https://hg.mozilla.org/integration/fx-team/rev/00a3ea4089ce
because overflow tests are failing.

I'll need to add an extra check for overflowable toolbar areas (sigh) because they have a second dom node (namely the overflow list) that the node could be a part of.

Hope to have a new patch tomorrow. Shouldn't be too difficult, but it's past midnight here so backing out seemed better than trying to sort this out in-tree.

Comment 11

[:Gijs (he/him)]

Attachment: fix removing widgets from outside their area,

We already looked up the overflowable container. Don't know how I missed that. Well, this should work, at least. Try push: https://tbpl.mozilla.org/?tree=Try&rev=b69261e55a8e

Comment 12

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

Comment on attachment 8423504 [details] [diff] [review]
fix removing widgets from outside their area,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +776,3 @@
>        let widgetNode = window.document.getElementById(aWidgetId);
> +      if (widgetNode && isOverflowable) {
> +        container = areaNode.overflowable.getContainerFor(widgetNode);

This is fine, but I think our API could be better if the caller didn't need to ask the OverflowableToolbar where the item is, and instead the areaNode would know to check itself and then the overflowable toolbar if it was one.

::: browser/components/customizableui/test/browser_996635_remove_non_widgets.js
@@ +29,5 @@
> +  ok(placement, "Button should be placed");
> +  is(placement && placement.area, CustomizableUI.AREA_TABSTRIP, "Should be placed on tabstrip.");
> +  // Check we didn't move the node.
> +  is(btn.parentNode && btn.parentNode.id, "nav-bar", "Actual button should still be on navbar.");
> +  

nit, whitespace

Comment 13

[:Gijs (he/him)]

try push because argh @ syntax (bug 1011700): remote:   https://tbpl.mozilla.org/?tree=Try&rev=4f574bf23c58

Comment 14

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #13)
> try push because argh @ syntax (bug 1011700): remote:  
> https://tbpl.mozilla.org/?tree=Try&rev=4f574bf23c58

Trees of green... so: remote:   https://hg.mozilla.org/integration/fx-team/rev/aee3e89fa990

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/aee3e89fa990