947586 - Menu panel context menu sometimes only has a single "Add More Items"

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[u428464]

STR : 
1. Put the bookmarks widget in the menu panel
2. Leave customization mode
3. Right click on the widget in the menu

Current result : Contextual menu only show "add more items"

ER : Contextual menu shows "add to toolbar", "remove from menu" and "add more items..."

Also note that if you right click the bookmark widget and then another widget they then only show "add more items..." too.

Comment 1

[u428464]

This widget is a nightmare. I've been able to completely break the customization mode just by randomly using the contextual menu and moving it.

Comment 2

[Justin Dolske [:Dolske]]

The actual impact here is probably more of a P3 or P4, but I'm going to leave it at P2 until we know what's causing this.

Comment 3

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

Attachment: Test

Test that reproduces the issue. When this happens, the context attribute disappears from the buttons in the panel, not sure why though.

Comment 4

[Justin Dolske [:Dolske]]

This isn't specific to the bookmarks button.

STR:

0) New profile
1) [optional] right-click any icon in the menu panel, confirm that it has the 3 expected "Move to Toolbar" + "Remove from Menu" + "Customize…" items.
2) Enter customize mode
3) right-click any icon in the menu panel shown for customization. It'll have the same items as #1, but "Customize…" is disabled.
4) Exit customize mode
5) right-click any icon in the menu panel. Now there is only a single "Add More Items…" entry.

Note that the single-item "Add More Items…" is the normal and expected context menu when you click empty areas of the menu panel (or the customize/help/quit buttons at the bottom).

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

So... createWrapper() tries to optimize (?) not saving the existing context menu attribute if it's already set to the expected value. I think it's assuming it can safely just nuke this attribute as if "expected" means "default", but that's not true. The panel has a default context menu -- the single-entry "Add More Items…" menu -- and so if we don't restore the |context| attribute for the items in the panel, they will no longer override the panel's default after exiting customization.

Comment 6

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

Comment on attachment 8349163 [details] [diff] [review]
Patch v.1

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

This patch fails the existing browser_880164_customization_context_menus.js test with "Uncaught exception - Context menu (toolbar-context-menu) did not show within 20 seconds." and there are also some failures with the test attached to this bug but those could be related or fallout from this failure.

Comment 8

[Justin Dolske [:Dolske]]

Ugh, I should have looked at blame to see that 945191 added thia code for a reason (even though it caused this bug).

I tried to instead just a call to ensureButtonContextMenu() when unwrapping items without a cached contextmenu attribute, but that's still broken. I don't understand what's happening now.

Comment 9

[:Gijs (he/him)]

Attachment: fix Australis' menu panel context menu being gone after customize mode,

Dolske, at least your comments made it easier for us to figure out how to fix it, so thank you for working on this\! :-) Here's a fix + test which passes on my machine (together with the other menu tests).

Comment 10

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/218d134a0e35

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/218d134a0e35