Closed Bug 969427 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: florian, Assigned: florian)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3][qa-])

Attachments

(1 file, 2 obsolete files)

Attached patch reset gSeenWidgets (obsolete) — Splinter Review
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.
Attachment #8372324 - Flags: review?(bmcbride)
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...
Flags: needinfo?(bmcbride)
Whiteboard: [Australis:P3]
Yea, I think this fits in with the fact that when we reset, add-on's widgets get placed in their default placements again.
Flags: needinfo?(bmcbride)
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/
Attachment #8372324 - Flags: review?(bmcbride) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
(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 :-).
Assignee: nobody → florian
Attachment #8372324 - Attachment is obsolete: true
Attachment #8373662 - Flags: review?(bmcbride)
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)
Attachment #8373662 - Flags: review?(bmcbride) → review+
Attached patch Patch v3Splinter Review
Updated to address comment 5.
Attachment #8373662 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c530339e8155
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
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.
Attachment #8373680 - Flags: review+
Attachment #8373680 - Flags: approval-mozilla-aurora?
Attachment #8373680 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [Australis:P3] → [Australis:P3][qa-]
You need to log in before you can comment on or make changes to this bug.