Closed Bug 1354078 Opened 3 years ago Closed 2 years ago

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

Categories

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

53 Branch
enhancement

Tracking

()

VERIFIED FIXED
Firefox 55
Iteration:
55.6 - May 29
Tracking Status
firefox55 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [photon-structure])

Attachments

(2 files)

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.
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
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!)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 55.6 - May 29
Priority: P2 → P1
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
Attachment #8869481 - Flags: review?(bgrinstead)
(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...
Flags: needinfo?(abenson)
Depends on: 1367097
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
Attachment #8869481 - Flags: review?(bgrinstead) → review+
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?
Attachment #8870758 - Flags: review?(bgrinstead) → review+
(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
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1368370
Depends on: 1378427
Depends on: 1368300
Blocks: 1387512
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.