969427 - recreating a destroyed widget after a CustomizableUI.reset() call causes the new widget to go in the customize palette

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Florian Quèze [:florian]]

Attachment: reset gSeenWidgets

This bug is the reason for the test failures I had in bug 965265 comment 6.

A reduced testcase for this (paste in the scratchpad) is:

var id = "foo";
Components.utils.import("resource:///modules/CustomizableUI.jsm");

function create() {
  CustomizableUI.createWidget({
    id: id,
    type: 'custom',
    removable: true,
    defaultArea: CustomizableUI.AREA_NAVBAR,
    onBuild: function(aDocument) {
      let node = aDocument.createElement('toolbarbutton');
      node.id = this.id;
      node.setAttribute("label", "foo");
      node.setAttribute("tooltiptext", "foo");
      node.style.listStyleImage = "url('about:logo')";

      node.setAttribute("oncommand", "alert('hello');");

      return node;
    }
  });
}

create();
CustomizableUI.destroyWidget(id);
CustomizableUI.reset();
create();



Executed this way, the widget ends up in the customize palette. If the "CustomizableUI.reset();" line is removed, the widget ends up in the browser toolbar.


I think the fix is simply to reset gSeenWidgets from CustomizableUI.reset.

Comment 1

[:Gijs (he/him)]

Blair, do you think resetting gSeenWidgets from CustomizableUI.reset() makes sense? It seems so to me, but I'm wondering if I'm missing something...

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yea, I think this fits in with the fact that when we reset, add-on's widgets get placed in their default placements again.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8372324 [details] [diff] [review]
reset gSeenWidgets

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

Really needs a test. Plenty of examples in ../test/

Comment 4

[Florian Quèze [:florian]]

Attachment: Patch v2

(In reply to Blair McBride [:Unfocused] from comment #3)
> Comment on attachment 8372324 [details] [diff] [review]

> Really needs a test.

Definitely; I guess the flag I really meant to set was the feedback one :-).

Comment 5

[:Gijs (he/him)]

Comment on attachment 8373662 [details] [diff] [review]
Patch v2

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

Nice, ship it!

::: browser/components/customizableui/test/browser_969427_recreate_destroyed_widget_after_reset.js
@@ +5,5 @@
> +"use strict";
> +
> +function isInNavBar(id) {
> +  let placement = CustomizableUI.getPlacementOfWidget(id);
> +  return placement && placement.area == CustomizableUI.AREA_NAVBAR;

Nit: rename to getPlacementArea, return the area

@@ +16,5 @@
> +  let spec = {id: kWidgetId, label: "Test re-create after reset.",
> +              removable: true, defaultArea: CustomizableUI.AREA_NAVBAR};
> +
> +  CustomizableUI.createWidget(spec);
> +  ok(isInNavBar(kWidgetId), "widget is in the navigation bar");

is(getArea(kWidgetId), CustomizableUI.AREA_NAVBAR, "widget is in the navigation bar");

and so forth through the rest of the test (use isnot for the negative case)

The purpose of this is to be able to see what else the value was if the test ever fails - otherwise it'll just say "no", instead of the area where it *did* end up (or null if it's in the palette or unknown)

Comment 6

[Florian Quèze [:florian]]

Attachment: Patch v3

Updated to address comment 5.

Comment 7

[Florian Quèze [:florian]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[:Gijs (he/him)]

Comment on attachment 8373680 [details] [diff] [review]
Patch v3

(Carrying over r+)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: add-on buttons disappearing
Testing completed (on m-c, etc.): m-c, has automated test
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

We should put this on Aurora to ensure the right API is available both there and on Nightly.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7f05473140d9