Closed Bug 1354127 Opened 3 years ago Closed 3 years ago

Add 'More' button to static hamburger menu

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
Firefox 55
Iteration:
55.5 - May 15
Tracking Status
firefox55 --- verified

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [photon-structure])

Attachments

(1 file)

This button opens a subview when clicked, which contains - for now! - the following two buttons:

* 'Text Encoding' - another subview, which is an exact copy of the Text Encoding widget in the current menu panel. An icon does not seem required, but since we have one already, I'd suggest using it.
* 'Work Offline' - a new widget, its command is the same as the equivalent menu item in the 'File' menu. No icon.

The title of the subview reads 'More', the same as the button label.
The button should be placed right below the 'Find in Page...' button.

NB: the UX goal of this menu is to hold all items that are important enough to show in the main menu, but now in the primary list of buttons there.
No longer depends on: 1354119
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 55.5 - May 15
Priority: P2 → P1
Comment on attachment 8866881 [details]
Bug 1354127 - Add a 'More' toolbar button to the Photon app menu.

https://reviewboard.mozilla.org/r/138476/#review141762

::: browser/components/customizableui/CustomizableWidgets.jsm:1079
(Diff revision 1)
>          }
>        };
>        CustomizableUI.addListener(listener);
> +      this.onInit();
> +    },
> +    onInit(aNode) {

Unused argument

::: browser/components/customizableui/PanelMultiView.jsm:395
(Diff revision 1)
> +
> +      // Make sure that new panels always have a title set.
> +      if (this.panelViews && aAdopted && aAnchor) {
> +        if (aAnchor && !viewNode.hasAttribute("title"))
> +          viewNode.setAttribute("title", aAnchor.getAttribute("label"));
> +        let custWidget = CustomizableWidgets.find(widget => widget.viewId == viewNode.id);

Nit: move this after the classList addition, to have it together with the if (custWidget) check.

::: browser/components/customizableui/PanelMultiView.jsm:400
(Diff revision 1)
> +        let custWidget = CustomizableWidgets.find(widget => widget.viewId == viewNode.id);
> +        viewNode.classList.add("PanelUI-subView");
> +        if (custWidget) {
> +          if (custWidget.onInit)
> +            custWidget.onInit(aAnchor);
> +          custWidget.onViewShowing({ target: aAnchor });

Can you fold in a change I made in a later patch, to create the 'details' object earlier and pass in the same details object to this onViewShowing call?

::: browser/themes/osx/customizableui/panelUI.css:96
(Diff revision 1)
>  #PanelUI-remotetabs-tabslist {
>    padding-bottom: 4px;
>  }
> +
> +
> +/* START photon adjustments */

Please revert this move before landing.

::: browser/themes/shared/menu-icons/check.svg:1
(Diff revision 1)
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">

Nit: please add license header and rm the "viewBox" attribute.
Attachment #8866881 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1e6c8eee951e
Add a 'More' toolbar button to the Photon app menu. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/1e6c8eee951e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Reminder: localizers see a string landing that says "More", the ID doesn't help that much (moreMenu.label), adding a localization comment in these cases is going to be extremely useful (e.g. explaining where it's going to be used, and how).
Depends on: 1364445
Depends on: 1364672
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.