942626 - Add to Menu/Toolbar items don't filter nodes appropriately

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Landis Reed]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20131124104839

Steps to reproduce:

right-click on left arrow, tabs-bar or menu-bar 
context menu, top item: Add to Menu


Actual results:

What is it?
clicking on 'Add to Menu' seems to do Nothing...

Landis.

Comment 1

[:Gijs (he/him)]

Can you reproduce this on a clean profile?

Comment 2

[:Gijs (he/him)]

So I just looked into this some more and could reproduce myself. Better STR (on mac):

1. Right click just outside the rounded corner of the search bar (so inside the search bar's element);
2. Click "Add to Menu"

AR:
Nothing happens

ER:
Item moves to menu panel

The fact that the item appears and isn't disabled is correct because the code that evaluates the disabling of the items correctly loops through the DOM tree and looks for the right item.

Unfortunately the addToMenu/addToToolbar code doesn't do the same looping and only checks for toolbarpaletteitems, not for possible child/descendant elements . It should be doing the same thing, or the node we detected should somehow be passed (I don't /think/ that's possible, but I might be wrong).

Maybe expose this kind of loop as a utility method on CustomizableUI? We do this kind of stuff a lot, also e.g. for panel close stuff.

Comment 3

[Peter Retzer (:pretzer)]

Attachment: Screenshot of the problem

I've found another problem which might be just a different manifestation of this bug.

STR:
1. Right click on one of the combined buttons in the menu (copy/paste controls, zoom controls)
2. Click "Add to Toolbar"

AR: Nothing happens

ER: Item moves to toolbar


STR (continued):
3. Again right click on one of the combined buttons in the menu (copy/paste controls, zoom controls)
4. Click "Remove from Menu"

AR: Just the part of the combined button moves to the palette that was right-clicked (e.g. just the "Cut" part, or just the "100%" part) and shows up corrupted there.

ER: Whole combined button moves to palette


Do you want me to file a new bug or would that be handled here?

Comment 4

[:Gijs (he/him)]

(In reply to Peter Retzer (:pretzer) from comment #3)
> Created attachment 8337727 [details]
> Screenshot of the problem
> 
> I've found another problem which might be just a different manifestation of
> this bug.
> 
> STR:
> 1. Right click on one of the combined buttons in the menu (copy/paste
> controls, zoom controls)
> 2. Click "Add to Toolbar"
> 
> AR: Nothing happens
> 
> ER: Item moves to toolbar
> 
> 
> STR (continued):
> 3. Again right click on one of the combined buttons in the menu (copy/paste
> controls, zoom controls)
> 4. Click "Remove from Menu"
> 
> AR: Just the part of the combined button moves to the palette that was
> right-clicked (e.g. just the "Cut" part, or just the "100%" part) and shows
> up corrupted there.
> 
> ER: Whole combined button moves to palette
> 
> 
> Do you want me to file a new bug or would that be handled here?

Thanks for pointing this out. We should fix this here, there shouldn't be a need for a new bug. Also, that's a pretty hilarious bug to be divvying up combined items like that. :-)

Comment 5

[:Gijs (he/him)]

Attachment: fix filtering for customize context menus in Australis,

This fixes things for me. However, it seems the context menu isn't always updated correctly. I'm not sure why, because looking at the code in the debugger, it seems the right bits run...

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #5)
> Created attachment 8337746 [details] [diff] [review]
> fix filtering for customize context menus in Australis,
> 
> This fixes things for me. However, it seems the context menu isn't always
> updated correctly. I'm not sure why, because looking at the code in the
> debugger, it seems the right bits run...

Annnd now I can't reproduce anymore. Huh.

Comment 7

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

Comment on attachment 8337746 [details] [diff] [review]
fix filtering for customize context menus in Australis,

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

We should have a test for this.

Comment 8

[:Gijs (he/him)]

Attachment: fix filtering for customize context menus in Australis,

Now with tests.

Comment 9

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

Comment on attachment 8337800 [details] [diff] [review]
fix filtering for customize context menus in Australis,

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

::: browser/components/customizableui/test/browser_880164_customization_context_menus.js
@@ +84,5 @@
> +      let searchbar = document.getElementById("searchbar");
> +      gCustomizeMode.addToPanel(searchbar);
> +      let placement = CustomizableUI.getPlacementOfWidget("search-container");
> +      is(placement.area, CustomizableUI.AREA_PANEL, "Should be in panel");
> +      gCustomizeMode.addToToolbar(searchbar);

Please move the show/hide of the menu panel to immediately before this line as it is a better representation of the workflow that a user would follow.

Comment 10

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #9)
> Comment on attachment 8337800 [details] [diff] [review]
> fix filtering for customize context menus in Australis,
> 
> Review of attachment 8337800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> browser/components/customizableui/test/
> browser_880164_customization_context_menus.js
> @@ +84,5 @@
> > +      let searchbar = document.getElementById("searchbar");
> > +      gCustomizeMode.addToPanel(searchbar);
> > +      let placement = CustomizableUI.getPlacementOfWidget("search-container");
> > +      is(placement.area, CustomizableUI.AREA_PANEL, "Should be in panel");
> > +      gCustomizeMode.addToToolbar(searchbar);
> 
> Please move the show/hide of the menu panel to immediately before this line
> as it is a better representation of the workflow that a user would follow.

with this addressed,

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/763b04634768

Comment 12

[u428464]

It's still an issue in latest Nightly.

Comment 13

[:Gijs (he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #12)
> It's still an issue in latest Nightly.

Can you provide more specific STR and file a new bug with those? Thanks!

Comment 14

[u428464]

I'm not sure if it's the right bug anymore. But when I click on the tabbar the "add to menu" and "remove from toolbar" are still enabled, but clicking here doesn't do anything.

Comment 15

[:Gijs (he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #14)
> I'm not sure if it's the right bug anymore. But when I click on the tabbar
> the "add to menu" and "remove from toolbar" are still enabled, but clicking
> here doesn't do anything.

Filed bug 944403 with this.

Comment 16

[Peter Retzer (:pretzer)]

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > Created attachment 8337746 [details] [diff] [review]
> > fix filtering for customize context menus in Australis,
> > 
> > This fixes things for me. However, it seems the context menu isn't always
> > updated correctly. I'm not sure why, because looking at the code in the
> > debugger, it seems the right bits run...
> 
> Annnd now I can't reproduce anymore. Huh.

I still can reproduce an issue where the context menu isn't updated, maybe the one you saw. Filed bug 945191.