bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
a year ago
10 months ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks: 3 bugs)

53 Branch
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

a year ago
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 hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
mozreview-review
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)

Updated

a year ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 55.6 - May 29
Priority: P2 → P1

Comment 3

a year ago
mozreview-review
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)
(Assignee)

Comment 4

a year ago
(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)
(Assignee)

Updated

a year ago
Depends on: 1367097
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)

Comment 8

a year ago
More details added to the spec (at the bottom): https://mozilla.invisionapp.com/share/ENBBK0F9U#/229252091_Customization
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

a year ago
(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

a year ago
mozreview-review
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 13

a year ago
mozreview-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+
(Assignee)

Comment 14

a year ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

a year ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

a year ago
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
(Assignee)

Comment 23

a year ago
(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?
(Assignee)

Comment 25

a year ago
(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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fd1d772083aa
https://hg.mozilla.org/mozilla-central/rev/1ff36fd81097
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1368370
(Assignee)

Updated

a year ago
Depends on: 1378427
Depends on: 1368300

Comment 27

10 months ago
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

Updated

10 months ago
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.