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

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Blocks 1 bug)

Trunk
Firefox 28
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:P3])

Attachments

(1 attachment, 2 obsolete attachments)

(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
Posted patch Patch (obsolete) — Splinter Review
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8336572 - Flags: review?(gijskruitbosch+bugs)

Comment 2

6 years ago
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)? :-)
Attachment #8336572 - Flags: review?(gijskruitbosch+bugs) → feedback+
(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.
Whiteboard: [Australis:P3]
(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 :)
Posted patch Patch v2 (obsolete) — Splinter Review
Attachment #8336989 - Flags: review?(gijskruitbosch+bugs)
Attachment #8336572 - Attachment is obsolete: true
Attachment #8336989 - Attachment is obsolete: true
Attachment #8336989 - Flags: review?(gijskruitbosch+bugs)
Attachment #8336994 - Flags: review?(gijskruitbosch+bugs)

Comment 7

6 years ago
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. :-)
Attachment #8336994 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/0817a274a7aa
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Depends on: 943518
No longer depends on: 943518
You need to log in before you can comment on or make changes to this bug.