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

RESOLVED FIXED in Firefox 28

Status

()

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

People

(Reporter: zer0, Assigned: Gijs)

Tracking

(Blocks: 1 bug, {addon-compat})

Trunk
Firefox 28
x86
Mac OS X
addon-compat
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M8][Australis:P2])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 872617
Whiteboard: [Australis:M?]
(Assignee)

Comment 1

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

Updated

5 years ago
Keywords: addon-compat
Whiteboard: [Australis:M?] → [Australis:P2]
(Assignee)

Updated

5 years ago
Assignee: nobody → gijskruitbosch+bugs
(Assignee)

Comment 2

5 years ago
Created attachment 773316 [details] [diff] [review]
Patch

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)
(Assignee)

Comment 3

5 years ago
Created attachment 773318 [details] [diff] [review]
Test

For reference, this was my test (the opening window thing was a check to see if that helped; it didn't).
(Assignee)

Comment 4

5 years ago
Created attachment 773322 [details] [diff] [review]
Patch

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)
Attachment #773322 - Flags: review?(mconley) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 773425 [details] [diff] [review]
Tests v2

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

Comment 7

5 years ago
Folded and pushed: https://hg.mozilla.org/projects/ux/rev/ec67f6224dd9
Status: NEW → ASSIGNED
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-ux]
(Assignee)

Updated

5 years ago
Whiteboard: [Australis:P2][fixed-in-ux] → [Australis:M8][Australis:P2][fixed-in-ux]
(Assignee)

Comment 8

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

Comment 9

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

Comment 10

5 years ago
https://hg.mozilla.org/mozilla-central/rev/ec67f6224dd9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.