941990 - The customization context menus don't appear on buttons in the menu panel in customization mode if the panel is not opened prior to entering customization mode

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

(OMG longest bug summary evar)

STR:
Launch Firefox
Use the View -> Tools -> Customize menuitem to enter Customization mode
Right-click on a toolbarbutton within the menu panel

ER:
See context menu (introduced by bug 880164) to move the items to the toolbar/palette.

AR:
No context menu

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 2

[:Gijs (he/him)]

Comment on attachment 8336572 [details] [diff] [review]
Patch

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

This would have been an r+ but for the first question. I don't follow why this.panel.hidden needs moving. Maybe I'm missing something, but AFAICT that'd mean stuff might not have XBL bindings when the buildArea code runs, which could be bad depending on what nodes try to do in onBuild or similar...

::: browser/components/customizableui/content/panelUI.js
@@ +222,5 @@
>          this.beginBatchUpdate();
>          CustomizableUI.registerMenuPanel(this.contents);
>          this.endBatchUpdate();
>        }
> +      this.panel.hidden = false;

Why did you move this?

::: browser/components/customizableui/test/browser_880164_customization_context_menus.js
@@ +194,5 @@
> +      let contextMenu = otherWin.document.getElementById("customizationPanelItemContextMenu");
> +      let shownPromise = contextMenuShown(contextMenu);
> +      let newWindowButton = otherWin.document.getElementById("wrapper-new-window-button");
> +      EventUtils.synthesizeMouse(newWindowButton, 2, 2, {type: "contextmenu", button: 2}, otherWin);
> +      yield shownPromise;

This will time out without the fix (or when we regress it). At which point we'll never leave customize mode in this test because it doesn't define a cleanup function, and Bad Things Will Happen. Can you make a reasonable effort to make sure that that doesn't happen? We should probably (a) make the head.js framework try catch each run() method so the teardown still happens, (b) move endCustomizing and otherWin.close into the teardown here, (c) add a cleanup function to this test that closes customize mode, (d) make the show promises for both the context menu and the panel menu (in head.js) have a timeout which makes them reject the promise (say, after 20 seconds - it's OK if it takes a while, and I'd rather not introduce random orange rather than fix it, but we should try to keep control of the test rather than having it time out and have horrible things happen).

::: browser/components/customizableui/test/head.js
@@ +103,5 @@
>    synthesizeDragStart(toDrag.parentNode, dragData);
>    synthesizeDrop(target, target, dragData);
>  }
>  
> +function endCustomizing(aWindow=window) {

Does this syntax require a lack of spaces? If not, can you add them (also below)? :-)

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #2)
> Comment on attachment 8336572 [details] [diff] [review]
> Patch
> 
> Review of attachment 8336572 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This would have been an r+ but for the first question. I don't follow why
> this.panel.hidden needs moving. Maybe I'm missing something, but AFAICT
> that'd mean stuff might not have XBL bindings when the buildArea code runs,
> which could be bad depending on what nodes try to do in onBuild or similar...

When the menubar code path is run, the panel is still hidden. Because these context menus are children of the panel, they never show because they are in effect hidden also.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #2)
> > +function endCustomizing(aWindow=window) {
> 
> Does this syntax require a lack of spaces? If not, can you add them (also
> below)? :-)

This syntax doesn't require lack of spaces, but this change matches the coding style found throughout the rest of this file. I'd rather not change all of the places where default arguments are used in this file :)

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2 (qref'd)

Comment 7

[:Gijs (he/him)]

Comment on attachment 8336994 [details] [diff] [review]
Patch v2 (qref'd)

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

Nice! This will help make our tests better. :-)

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/0817a274a7aa

Comment 9

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/0817a274a7aa