Closed Bug 942626 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

28 Branch
x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: landis, Assigned: Gijs)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [Australis:P2])

Attachments

(2 files, 1 obsolete file)

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.
Can you reproduce this on a clean profile?
Flags: needinfo?(landis)
Component: Untriaged → Toolbars and Customization
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.
Blocks: 880164
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(landis)
Summary: toolbar: right-click context menu : add to menu → Add to Menu/Toolbar items don't filter nodes appropriately
Whiteboard: [Australis:P2]
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?
Flags: needinfo?(gijskruitbosch+bugs)
(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. :-)
Flags: needinfo?(gijskruitbosch+bugs)
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...
Attachment #8337746 - Flags: review?(jaws)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(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 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.
Attachment #8337746 - Flags: review?(jaws)
Now with tests.
Attachment #8337800 - Flags: review?(jaws)
Attachment #8337746 - Attachment is obsolete: true
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.
Attachment #8337800 - Flags: review?(jaws) → review+
(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
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/763b04634768
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][fixed-in-fx-team] → [Australis:P2]
Target Milestone: --- → Firefox 28
It's still an issue in latest Nightly.
(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!
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.
(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.
(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.
You need to log in before you can comment on or make changes to this bug.