Closed Bug 890262 Opened 8 years ago Closed 7 years ago

CustomizableUI.destroyWidget raises an exception after `CustomizableUI.addWidgetToArea`

Categories

(Firefox :: Toolbars and Customization, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: zer0, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, Whiteboard: [Australis:M8][Australis:P2])

Attachments

(2 files, 2 obsolete files)

So, I'm not sure if it's a bug or I'm doing something wrong here.

In the test suite for the new Button in jetpack, that are using CustomizableUI, I'm testing that the appropriate icon is displayed depends if we are in panel or in the navbar. Because by default the button is added in the navbar, from my code I call:

  CustomizableUI.addWidgetToArea(widgetId, CustomizableUI.AREA_PANEL);

Then I test all the stuff I need, and I unload the jetpack loader – that calls the `CustomizableUI.destroyWidget`, and here the exception is reaised:

TypeError: buildArea is undefined
resource://app/modules/CustomizableUI.jsm 1492
[cut all the jetpack's module trace]
  File "resource://app/modules/CustomizableUI.jsm", line 1808, in 
    CustomizableUIInternal.destroyWidget(aWidgetId);
  File "resource://app/modules/CustomizableUI.jsm", line 1492, in 
    for (let buildNode of buildArea) {

As workaround, currently I'm move back the the widget to its original area (NAVBAR) before destroying it:

  CustomizableUI.addWidgetToArea(widgetId, CustomizableUI.AREA_NAVBAR);

Notice that this exception is not raised if I manually move the button from the navbar to the panel.
Whiteboard: [Australis:M?]
I'm fairly sure this is because we initialize the panel lazily. Can you easily test what happens if you simulate a click event on the menu panel button (to open it, and close it again after, I guess) before calling destroy ? I suspect that would also fix it.

If I'm correct, that still means it's a bug on our side, as we should cope with not having any build areas. :-)
Keywords: addon-compat
Whiteboard: [Australis:M?] → [Australis:P2]
Assignee: nobody → gijskruitbosch+bugs
Attached patch Patch (obsolete) — Splinter Review
I wrote a test for this but it only tests the failure properly if nothing opens the panel (adding it to gBuildAreas). Given that our other tests do that, this isn't really easily testable, and given the size of the patch, I think it's unlikely we'll really regress this. I did test, using that test in isolation, that this is actually fixed.
Attachment #773316 - Flags: review?(mconley)
Attached patch Test (obsolete) — Splinter Review
For reference, this was my test (the opening window thing was a check to see if that helped; it didn't).
Attached patch PatchSplinter Review
Adding the right patch helps; sorry for the spam!
Attachment #773316 - Attachment is obsolete: true
Attachment #773316 - Flags: review?(mconley)
Attachment #773322 - Flags: review?(mconley)
Attached patch Tests v2Splinter Review
This test does work correctly even if another test has run previously, by not depending on the panel. Thanks for the excellent suggestion! :-)
Attachment #773318 - Attachment is obsolete: true
Attachment #773425 - Flags: review?(mconley)
Comment on attachment 773425 [details] [diff] [review]
Tests v2

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

LGTM! Woo, tests!
Attachment #773425 - Flags: review?(mconley) → review+
Folded and pushed: https://hg.mozilla.org/projects/ux/rev/ec67f6224dd9
Status: NEW → ASSIGNED
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-ux]
Whiteboard: [Australis:P2][fixed-in-ux] → [Australis:M8][Australis:P2][fixed-in-ux]
This was backed out yesterday: https://hg.mozilla.org/projects/ux/rev/a8a320338f33
Whiteboard: [Australis:M8][Australis:P2][fixed-in-ux] → [Australis:M8][Australis:P2]
(In reply to :Gijs Kruitbosch from comment #8)
> This was backed out yesterday:
> https://hg.mozilla.org/projects/ux/rev/a8a320338f33

Sorry, this was inaccurate; the work was left in, but the test backed out. Relanded as: https://hg.mozilla.org/projects/ux/rev/56af4ca8e542
Whiteboard: [Australis:M8][Australis:P2] → [Australis:M8][Australis:P2][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/ec67f6224dd9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][Australis:P2][fixed-in-ux] → [Australis:M8][Australis:P2]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.