1354127 - Add 'More' button to static hamburger menu

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

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.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1354127 - Add a 'More' toolbar button to the Photon app menu.

This also improves the styling of checkbox buttons inside the menu and improves
support for adopting panelviews into the app menu from another area properly.

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

Comment 2

[:Gijs (out for now; he/him)]

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.

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8866881 [details]
Bug 1354127 - Add a 'More' toolbar button to the Photon app menu.

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

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8866881 [details]
Bug 1354127 - Add a 'More' toolbar button to the Photon app menu.

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e6c8eee951e
Add a 'More' toolbar button to the Photon app menu. r=Gijs

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e6c8eee951e

Comment 7

[Francesco Lodolo [:flod]]

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

Comment 8

[u554753]

Verified on Windows, Mac, and Ubuntu.