945191 - Combined buttons show wrong context menu options when they are in the toolbar

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Peter Retzer (:pretzer)]

STR:
1) Put a combined button (Zoom or Copy/Paste controls) in the toolbar, either via customization mode or via the context menu
2) Leave customization mode, if you were in it
3) Right-click the combined button on the toolbar

ER: Context menu shows 'Add to Menu' and 'Remove from Toolbar'
AR: Context menu shows 'Add to Toolbar' and 'Remove from Menu' again

The options are only wrong when you're not in customization mode. Right clicking while in customization mode shows the expected options.

Comment 1

[Ge3k0s]

I would add that the options are greyed out with the bookmark star and the search bar when it shouldn't be.

Comment 2

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

Attachment: Patch

Comment 3

[:Gijs (he/him)]

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

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

I don't understand the removal, and I found a bug, so I'm going to clear review on this one.

(did you want to address the disabled-ness of the star button's context menu item here or not?)

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ -420,5 @@
>              continue;
>            }
>          }
>  
> -        this.ensureButtonContextMenu(node, aAreaNode);

This is weird. Why are we removing this?

@@ +594,5 @@
>      let placements = gPlacements.get(CustomizableUI.AREA_PANEL);
>      this.buildArea(CustomizableUI.AREA_PANEL, placements, aPanel);
>      for (let btn of aPanel.querySelectorAll("toolbarbutton")) {
>        btn.setAttribute("tabindex", "0");
> +      if (btn.parentNode.id == CustomizableUI.AREA_PANEL) {

AIUI, this is just to ensure we don't add the contextmenu specially on nested buttons, right?

However, this means that with the following STR, I don't see the panel context menu any more:

1) new profile
2) Open the panel menu
3) right click the zoom/edit buttons

We should probably just make the loop loop through aPanel.children and disregard any non-toolbarbuttons for everything else, and non-toolbarbutton/toolbaritems for the context menu.

::: browser/components/customizableui/test/browser_880164_customization_context_menus.js
@@ +243,5 @@
>    },
> +  {
> +    desc: "Bug 945191 - Combined buttons show wrong context menu options when they are in the toolbar.",
> +    run: function () {
> +      yield startCustomizing();

Nit: please just:

setup: startCustomizing,

Comment 4

[:Gijs (he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #1)
> I would add that the options are greyed out with the bookmark star and the
> search bar when it shouldn't be.

Was the search bar in the panel or the toolbar?

Comment 5

[Ge3k0s]

(In reply to :Gijs Kruitbosch from comment #4)
> (In reply to Guillaume C. [:ge3k0s] from comment #1)
> > I would add that the options are greyed out with the bookmark star and the
> > search bar when it shouldn't be.
> 
> Was the search bar in the panel or the toolbar?

Toolbar

Comment 6

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

(In reply to Guillaume C. [:ge3k0s] from comment #1)
> I would add that the options are greyed out with the bookmark star and the
> search bar when it shouldn't be.

Please file a separate bug for this.

Comment 7

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

Attachment: Patch v1.1

I can only reproduce this when going through Customization Mode. This is happening because the we place a context attribute on the toolbaritems for the panel, and then when customization mode is entered we stash those away when the items get wrapped. Exiting customization mode restores them.

NB: I moved the startCustomizing to the setup function, but I originally didn't have it there since it didn't feel like setup to me, more like just part of the repro-steps. Not a big deal to me, so I moved it, but its previous placement was intentional.

Comment 8

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

Forgot to qref the moving of startCustomizing, but it's in my local patch.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8341960 [details] [diff] [review]
Patch v1.1

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

This makes sense. Hopefully this is the last time we have to adjust this...

Comment 10

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

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

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Hey, sorry for nitpicking but I just saw this has been pushed without "Australis" in the commit message. Thought this would be worth to mention so that you don't run into surprises when merging.

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[:Gijs (he/him)]

(In reply to Tim Taubert [:ttaubert] from comment #11)
> Hey, sorry for nitpicking but I just saw this has been pushed without
> "Australis" in the commit message. Thought this would be worth to mention so
> that you don't run into surprises when merging.

Thank you for letting us know. Fixed on m-c directly because that's what we're merging:

remote:   https://hg.mozilla.org/mozilla-central/rev/e55e2e773ae6
remote:   https://hg.mozilla.org/mozilla-central/rev/7476bb2b8e9c