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

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Matteo Ferretti [:zer0] [:matteo]]

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.

Comment 1

[:Gijs (he/him)]

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. :-)

Comment 2

[:Gijs (he/him)]

Attachment: 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.

Comment 3

[:Gijs (he/him)]

Attachment: Test

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

Comment 4

[:Gijs (he/him)]

Attachment: Patch

Adding the right patch helps; sorry for the spam!

Comment 5

[:Gijs (he/him)]

Attachment: 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! :-)

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 773425 [details] [diff] [review]
Tests v2

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

LGTM! Woo, tests!

Comment 7

[:Gijs (he/him)]

Folded and pushed: https://hg.mozilla.org/projects/ux/rev/ec67f6224dd9

Comment 8

[:Gijs (he/him)]

This was backed out yesterday: https://hg.mozilla.org/projects/ux/rev/a8a320338f33

Comment 9

[:Gijs (he/him)]

(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

Comment 10

[:Gijs (he/him)]

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