1354078 - Update toolbar context menu strings and functionality (based on a pref) to move items to/from overflow panel instead of hamburger panel

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, enhancement, P1)

Description

[:Gijs (he/him)]

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 1

[:Gijs (he/him)]

Attachment: Bug 1354078 - update panel/toolbar context menus to move items to the right place, with tests,

Review commit: https://reviewboard.mozilla.org/r/141108/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141108/

Comment 2

[:Gijs (he/him)]

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 3

[Brian Grinstead [:bgrins]]

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

Comment 4

[:Gijs (he/him)]

(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...

Comment 5

[Aaron Benson]

Lets go with this pattern: Send to {Overflow Menu,Toolbar}", and then "Remove from {Overflow Menu,Toolbar}

Comment 6

[Aaron Benson]

On second thought, "Send to" is a pattern we use for sharing items. Maybe best to keep it as "Move to {Overflow Menu, Toolbar}", and then "Remove from {Overflow Menu, Toolbar}"

Comment 7

[Aaron Benson]

On third thought (and properly vetted with others to ensure sanity), the pinning metaphor is best used here. 

"Pin to {Overflow Menu, Toolbar}", "Unpin from {Overflow Menu, Toolbar}"

Comment 8

[Aaron Benson]

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

Comment 9

[:Gijs (he/him)]

Attachment: Bug 1354078 - update labels for customization context menus for Photon,

Review commit: https://reviewboard.mozilla.org/r/142250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142250/

Comment 10

[:Gijs (he/him)]

Comment on attachment 8869481 [details]
Bug 1354078 - update panel/toolbar context menus to move items to the right place, with tests,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141108/diff/1-2/

Comment 11

[:Gijs (he/him)]

(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 12

[Brian Grinstead [:bgrins]]

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 13

[Brian Grinstead [:bgrins]]

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?

Comment 14

[:Gijs (he/him)]

(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.

Comment 15

[:Gijs (he/him)]

Comment on attachment 8869481 [details]
Bug 1354078 - update panel/toolbar context menus to move items to the right place, with tests,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141108/diff/2-3/

Comment 16

[:Gijs (he/him)]

Comment on attachment 8870758 [details]
Bug 1354078 - update labels for customization context menus for Photon,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142250/diff/1-2/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Francesco Lodolo [:flod]]

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

Comment 20

[:Gijs (he/him)]

Comment on attachment 8869481 [details]
Bug 1354078 - update panel/toolbar context menus to move items to the right place, with tests,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141108/diff/3-4/

Comment 21

[:Gijs (he/him)]

Comment on attachment 8870758 [details]
Bug 1354078 - update labels for customization context menus for Photon,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142250/diff/2-3/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[:Gijs (he/him)]

(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.

Comment 24

[Francesco Lodolo [:flod]]

(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?

Comment 25

[:Gijs (he/him)]

(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...

Comment 26

[Phil Ringnalda (:philor)]

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

Comment 27

[Abe - QA (:Abe_LV)]

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.