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

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 30
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox29 fixed, firefox30 fixed)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 8372324 [details] [diff] [review]
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.
Attachment #8372324 - Flags: review?(bmcbride)

Comment 1

5 years ago
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...
Blocks: 872617
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-
(Assignee)

Comment 4

5 years ago
Created attachment 8373662 [details] [diff] [review]
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 :-).
Assignee: nobody → florian
Attachment #8372324 - Attachment is obsolete: true
Attachment #8373662 - Flags: review?(bmcbride)

Comment 5

5 years ago
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+
(Assignee)

Comment 6

5 years ago
Created attachment 8373680 [details] [diff] [review]
Patch v3

Updated to address comment 5.
Attachment #8373662 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c530339e8155
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30

Comment 9

5 years ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/7f05473140d9
status-firefox29: --- → fixed
status-firefox30: --- → fixed

Updated

4 years ago
Whiteboard: [Australis:P3] → [Australis:P3][qa-]
You need to log in before you can comment on or make changes to this bug.