Closed Bug 945191 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 28

People

(Reporter: pretzer, Assigned: jaws)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [Australis:P2])

Attachments

(1 file, 1 obsolete file)

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.
I would add that the options are greyed out with the bookmark star and the search bar when it shouldn't be.
Whiteboard: [Australis:P2]
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8341510 - Flags: review?(gijskruitbosch+bugs)
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,
Attachment #8341510 - Flags: review?(gijskruitbosch+bugs)
(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?
Flags: needinfo?(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
Flags: needinfo?(ge3k0s)
(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.
Attached patch Patch v1.1Splinter Review
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.
Attachment #8341960 - Flags: review?(gijskruitbosch+bugs)
Attachment #8341510 - Attachment is obsolete: true
Forgot to qref the moving of startCustomizing, but it's in my local patch.
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...
Attachment #8341960 - Flags: review?(gijskruitbosch+bugs) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/d5a62078b4f2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
(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
Status: RESOLVED → VERIFIED
Depends on: 963726
Depends on: 947586
You need to log in before you can comment on or make changes to this bug.