982027 - Moving an icon around removes custom context menu

Status: VERIFIED FIXED

Description

[Wladimir Palant]

I noticed that toolbar customization will remove the "context" attribute from the Adblock Plus button under some circumstances. Steps to reproduce:

1. Install Adblock Plus.
2. Right-click the Adblock Plus icon - note the custom menu showing up.
3. Start toolbar customization.
4. Move the Adblock Plus icon from the toolbar into the overflow panel, then back.
5. Finish toolbar customization.
6. Right-click the Adblock Plus icon.

Expected results:

Custom Adblock Plus menu opens.

Actual results:

Default menu opens, the context attribute originally present on the button has been removed and not restored (tested in the 2014-03-06 Firefox nightly on OS X). Adding regression keyword because I am pretty certain that this used to work correctly immediately after the Australis merge.

Comment 1

[:Gijs (he/him)]

(In reply to Wladimir Palant from comment #0)
> 4. Move the Adblock Plus icon from the toolbar into the overflow panel, then
> back.

You mean the main menu panel, right? The overflow panel is the thing that shows up when you make the window narrow and then click the chevron that shows all the items that didn't fit...

Comment 2

[Wladimir Palant]

Yes, I guess the main menu panel is what I meant.

Comment 3

[:Gijs (he/him)]

I've written an automated test for this already, just need to figure out why it's breaking. :-\

Comment 4

[:Gijs (he/him)]

Attachment: moving icon around removes custom context menu,

This took a bit in order not to break stuff... but I believe this is sane, and it passes all the tests (including the new one I added for this particular problem). The root cause was the sizing checks already doing wrapping/unwrapping, which puts the context menu back in its 'normal' attribute, which then means it's not in the wrappedcontext attribute, which means stuff breaks.

Comment 5

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

Comment on attachment 8389115 [details] [diff] [review]
moving icon around removes custom context menu,

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

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +1752,5 @@
>  
>      if (targetArea != currentArea) {
>        this.unwrapToolbarItem(aDraggedItem.parentNode);
>        // Put the item back into its previous position.
> +      currentParent.insertBefore(aDraggedItem, currentSibling);

thank you for tidying this up

Comment 6

[:Gijs (he/him)]

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

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[:Gijs (he/him)]

Comment on attachment 8389115 [details] [diff] [review]
moving icon around removes custom context menu,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: icons' custom menus go missing when moving them in customize mode
Testing completed (on m-c, etc.): on m-c, has automated test
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 9

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0959eabf077e

Comment 10

[Ioana (away)]

Reproduced on the 03/11 Nightly with Adblock Plus and Firebug. Verified as fixed on Firefox 29.0b3 and the latest 30.0a2. All the testing was performed on Mac OS X 10.9.