Toolbar buttons have context menus that allow removing them, or moving them to either the toolbar or the hamburger menu, depending on where they are. We should update the menus so that, when the photon hamburger/overflow pref is flipped, they allow moving items to/from the overflow panel instead of the hamburger one. This involves both updating the strings, and updating the actual functionality of the items.

Comment on attachment 8869481 [details] Bug 1354078 - update panel/toolbar context menus to move items to the right place, with tests, https://reviewboard.mozilla.org/r/141108/#review144616 ::: browser/components/customizableui/content/panelUI.inc.xul (Diff revision 1) > </vbox> > </panelview> > > </panelmultiview> > - <!-- These menupopups are located here to prevent flickering, > - see bug 492960 comment 20. --> This bug got marked wfm, and in any case seemed to be about older versions of Windows, so I think we can stop hacking around it (yay!)

Comment on attachment 8869481 [details] Bug 1354078 - update panel/toolbar context menus to move items to the right place, with tests, https://reviewboard.mozilla.org/r/141108/#review144620 A few things things I noticed: 1) The height of the overflow menu doesn't get shrunk properly if you have two items there and remove one of them 2) Are "Add To Menu" / "Remove From Menu" still the right strings here for the overflow area? 3) If I remove the last item from the overflow menu there's a weird thing where the overflow menu flickers briefly below the browser window before it disappears. I can make a screencast if you can't reproduce locally ::: browser/components/customizableui/test/browser_photon_customization_context_menus.js:1 (Diff revision 1) > /* This Source Code Form is subject to the terms of the Mozilla Public Do we have a bug tracking tests that can be deleted once photon is shipped? Maybe they should be annotated somehow in browser.ini or maybe just searching for ["browser.photon.structure.enabled", false] (along with any other relevant prefs) is enough

(In reply to Brian Grinstead [:bgrins] from comment #3) > Comment on attachment 8869481 [details] > Bug 1354078 - update panel/toolbar context menus to move items to the right > place, with tests, > > https://reviewboard.mozilla.org/r/141108/#review144620 > > A few things things I noticed: > 1) The height of the overflow menu doesn't get shrunk properly if you have > two items there and remove one of them Per discussion in slack, let's move this to a followup, as this happens with the current menu, too. > 2) Are "Add To Menu" / "Remove From Menu" still the right strings here for > the overflow area? No, good catch. The spec says "Show in Overflow Menu". I think this is confusing because we do already display these context menus on items in the 'dynamic' overflow list. So they are already in the overflow menu at that point... Similarly, the existing entries (that stay the same) say "Move to Toolbar" but they might still be in the overflow menu then, if it's overflowing. Aaron, can you think of terminology that would make sense of this for the user? > 3) If I remove the last item from the overflow menu there's a weird thing > where the overflow menu flickers briefly below the browser window before it > disappears. I can make a screencast if you can't reproduce locally I haven't tried looking at this yet. I'm surprised the overflow panel disappears - oh, I guess the anchor might go away because there'll be no more items. This should be fixable by just calling hidePopup() before hiding the button. > ::: > browser/components/customizableui/test/ > browser_photon_customization_context_menus.js:1 > (Diff revision 1) > > /* This Source Code Form is subject to the terms of the Mozilla Public > > Do we have a bug tracking tests that can be deleted once photon is shipped? > Maybe they should be annotated somehow in browser.ini or maybe just > searching for ["browser.photon.structure.enabled", false] (along with any > other relevant prefs) is enough Yeah, I think it'll be fine to do this when we rip out the dead code post-photon. The pref will help find these tests, and otherwise the tests will just fail on automation and then we can remove them. :-) At this point I'm not sure if we'll remove the pref and everything during 57. Depends how much time we have, I guess...

More details added to the spec (at the bottom): https://mozilla.invisionapp.com/share/ENBBK0F9U#/229252091_Customization

(In reply to :Gijs from comment #4) > (In reply to Brian Grinstead [:bgrins] from comment #3) > > Comment on attachment 8869481 [details] > > Bug 1354078 - update panel/toolbar context menus to move items to the right > > place, with tests, > > > > https://reviewboard.mozilla.org/r/141108/#review144620 > > > > A few things things I noticed: > > 1) The height of the overflow menu doesn't get shrunk properly if you have > > two items there and remove one of them > > Per discussion in slack, let's move this to a followup, as this happens with > the current menu, too. This is now bug 1367097 > > 2) Are "Add To Menu" / "Remove From Menu" still the right strings here for > > the overflow area? I stuck these changes in its own patch for clarity. > > 3) If I remove the last item from the overflow menu there's a weird thing > > where the overflow menu flickers briefly below the browser window before it > > disappears. I can make a screencast if you can't reproduce locally > > I haven't tried looking at this yet. I'm surprised the overflow panel > disappears - oh, I guess the anchor might go away because there'll be no > more items. This should be fixable by just calling hidePopup() before hiding > the button. Done as part of the (updated) first patch. :-)

Comment on attachment 8869481 [details] Bug 1354078 - update panel/toolbar context menus to move items to the right place, with tests, https://reviewboard.mozilla.org/r/141108/#review146630

Comment on attachment 8870758 [details] Bug 1354078 - update labels for customization context menus for Photon, https://reviewboard.mozilla.org/r/142250/#review146636 In customize mode it's "Add to overflow menu" on the left, but then "Unpin from overflow menu" on the right - should that be "Remove from overflow menu" on the right?

(In reply to Brian Grinstead [:bgrins] from comment #13) > Comment on attachment 8870758 [details] > Bug 1354078 - update labels for customization context menus for Photon, > > https://reviewboard.mozilla.org/r/142250/#review146636 > > In customize mode it's "Add to overflow menu" on the left, but then "Unpin > from overflow menu" on the right - should that be "Remove from overflow > menu" on the right? It's the same as in the menu outside of customize mode, where this was specced (bottom of https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/229252091_Customization ) - I think we can leave it like this and we can update the strings some more later if we need to. I would prefer not to have different strings inside customize mode compared to outside for the overflow panel, given that it's the same menu.

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a2edd07d05dd update panel/toolbar context menus to move items to the right place, with tests, r=bgrins https://hg.mozilla.org/integration/autoland/rev/7be34fd75378 update labels for customization context menus for Photon, r=bgrins

Backed out for browser_photon_customization_context_menus.js failures. https://hg.mozilla.org/integration/autoland/rev/4cdd6c74b66f46524662790f50dc6d11cc4050ea https://treeherder.mozilla.org/logviewer.html#?job_id=102392598&repo=autoland

Since this has been backed out, can you please explain in a localization comment that "overflow menu" is? I confess I have no clue

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/fd1d772083aa update panel/toolbar context menus to move items to the right place, with tests, r=bgrins https://hg.mozilla.org/integration/autoland/rev/1ff36fd81097 update labels for customization context menus for Photon, r=bgrins

(In reply to Francesco Lodolo [:flod] from comment #19) > Since this has been backed out, can you please explain in a localization > comment that "overflow menu" is? I confess I have no clue I added some, though I will say that in general, I don't think we should need to be describing existing features (positively old ones by now - we've had an overflow menu since australis!) in localization comments. If there is confusion among localizers about what particular features are, then we should have a central other resource for this - in fact, I thought that there was a localization glossary somewhere, also to ensure things like "tab" or "page" or "site" or whatever get translated consistently throughout? (In reply to Ryan VanderMeulen [:RyanVM] from comment #18) > Backed out for browser_photon_customization_context_menus.js failures. > https://hg.mozilla.org/integration/autoland/rev/ > 4cdd6c74b66f46524662790f50dc6d11cc4050ea > > https://treeherder.mozilla.org/logviewer.html#?job_id=102392598&repo=autoland This was caused by a bug in an earlier-running test. I made trivial adjustments to the earlier test and verified that fixed the issue for me locally. Fixing the underlying issue in CUI.jsm exposed by that test is now bug 1368255.

(In reply to :Gijs from comment #23) > I added some, though I will say that in general, I don't think we should > need to be describing existing features (positively old ones by now - we've > had an overflow menu since australis!) in localization comments. If there is > confusion among localizers about what particular features are, then we > should have a central other resource for this - in fact, I thought that > there was a localization glossary somewhere, also to ensure things like > "tab" or "page" or "site" or whatever get translated consistently throughout? Thanks for adding the note. I've been around for a long time, and I've never heard it called "Overflow menu". There are no mentions on SUMO, or Google (mostly unrelated references to Fennec), even Bugzilla has very few results (some indeed old). I honestly would question that it should be called out as "Overflow menu" in the UI, but I don't know the discussion/spec that lead to this bug. Should I split out the discussion in a different bug, and who should be involved?

(In reply to Francesco Lodolo [:flod] from comment #24) > (In reply to :Gijs from comment #23) > > I added some, though I will say that in general, I don't think we should > > need to be describing existing features (positively old ones by now - we've > > had an overflow menu since australis!) in localization comments. If there is > > confusion among localizers about what particular features are, then we > > should have a central other resource for this - in fact, I thought that > > there was a localization glossary somewhere, also to ensure things like > > "tab" or "page" or "site" or whatever get translated consistently throughout? > > Thanks for adding the note. I've been around for a long time, and I've never > heard it called "Overflow menu". > > There are no mentions on SUMO, or Google (mostly unrelated references to > Fennec), even Bugzilla has very few results (some indeed old). I honestly > would question that it should be called out as "Overflow menu" in the UI, > but I don't know the discussion/spec that lead to this bug. Should I split > out the discussion in a different bug, and who should be involved? I mean, it's always been called that, I think? Maybe 'panel' rather than 'menu' in some places - that's how it's called out in https://developer.mozilla.org/en-US/Firefox/Australis_add-on_compat , for instance. "Toolbar overflow" is the original thing that we added this for - stuff that doesn't fit in the toolbar anymore, depending on the window size and/or how many things the user puts in the toolbar - so it overflows into this panel. If you feel strongly that the name is 'wrong' and have suggestions about what it should be instead, I would suggest taking it up with the relevant UX folks (Bryan, Aaron and Stephen), probably on IRC / slack, or the photon-dev mailing list. I personally am not invested in whatever name, though obviously changing the name consistently throughout the codebase is going to be a bit of a slog. It won't ultimately affect what it *does*, and it has existed for a while now without people being bothered by it or its name...

https://hg.mozilla.org/mozilla-central/rev/fd1d772083aa https://hg.mozilla.org/mozilla-central/rev/1ff36fd81097

I have tested this on latest nightly and I am able to move items to/from the overflow panel. Updating this bug as verified-fixed.