1354108 - Add a restyled edit control to static hamburger menu

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

This widget remains almost the same, compared to the current zoom widget in the menu panel, with the following exceptions:

* There's a static label shown at the start of the menu item.
* The edit buttons float at the end of the menu item.
* The button captions are shown as tooltips when hovered, not as labels.

A separator is shown below this menu item.

Comment 1

[Mike de Boer [:mikedeboer]]

Note: the tooltips may contain the hotkeys associated with the respective button command.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1354108 - Add the edit controls (cut, copy, paste) to the static menu panel.

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

Comment 3

[:Gijs (he/him)]

Comment on attachment 8867359 [details]
Bug 1354108 - Add the edit controls (cut, copy, paste) to the static menu panel.

https://reviewboard.mozilla.org/r/138886/#review143008

Almost there, but I'm not convinced about the HTML flexbox use and the string stuff. Sorry again for the delay here, my review/needinfo queue has been a bit of a Sisyphus-style yo-yo.

::: browser/base/content/browser.js:5613
(Diff revision 1)
> +    if (nodeId.startsWith("appMenu-")) {
> +      gDynamicTooltipCache.set(nodeId, CustomizableUI.bundle
> +        .formatStringFromName(strId, args, args.length));

This plus having to expose the bundle the way we do is a bit ugly. Can we either:

- copy the strings so we don't need to branch here and can keep using gNavigatorBundle
- adjust getLocalizedProperty in CUI so it works for this usecase
- use a separate lazy string bundle getter here, given that on any day we're unlikely to actually ever trigger this code? :-)
- omit the tooltips with shortcuts for now, unless they were explicitly specced by Aaron/Bryan - I thought we weren't doing tooltips at all?

I don't really mind which of these we do, they all have pros and cons. Feel free to pick any of them and/or argue that I'm being daft and we should go with what you already have...

::: browser/components/customizableui/CustomizableUI.jsm:2923
(Diff revision 1)
>        for (let [window, ] of gBuildWindows)
>          yield window;
>      }
>    },
>  
> +  get bundle() {

Docstring comment if we keep this, please. :-)

::: browser/themes/shared/customizableui/panelUI.inc.css:1183
(Diff revision 1)
>  photonpanelmultiview .subviewbutton > .menu-iconic-left {
>    -moz-appearance: none;
>    margin-inline-end: 0;
>  }
>  
> +photonpanelmultiview .toolbaritem-combined-buttons:-moz-any(:not([cui-areatype="toolbar"]), [overflowedItem="true"]) {

So... why do we need the :-moz-any(:not(...)) stuff here? Isn't this always in the same place?

::: browser/themes/shared/customizableui/panelUI.inc.css:1185
(Diff revision 1)
> +  display: flex;
> +  flex: auto 1;
> +  justify-content: end;

Can we avoid mixing XUL and 'normal' flexbox and just use flex=1 on the label to make that stretch, thus causing the other buttons to be end-justified anyway? Given that the other items don't flex I'm not sure why we need to use HTML-style flexbox here.

For context, the mixture of these two things is giving me pain for my WIP patch for bug 1354126 (because we need to move where the footer is and how the panel stretches in the DOM, and that's some unholy XUL+HTML flexbox mess right now), so I would prefer to avoid repeating this if possible.

::: browser/themes/shared/customizableui/panelUI.inc.css:1192
(Diff revision 1)
> +  justify-content: end;
> +  margin-inline-end: 8px;
> +}
> +
> +photonpanelmultiview .toolbaritem-combined-buttons > label {
> +  align-self: center;

Why do we need this?

::: browser/themes/shared/customizableui/panelUI.inc.css:1199
(Diff revision 1)
> +  font: menu;
> +  margin: 0;
> +  padding-inline-start: 36px; /* 12px toolbarbutton padding + 16px icon + 8px label padding start */
> +}
> +
> +photonpanelmultiview .PanelUI-subView .toolbaritem-combined-buttons > .subviewbutton {

Do we need the extra class (PanelUI-subView) for specificity reasons here?

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8867359 [details]
Bug 1354108 - Add the edit controls (cut, copy, paste) to the static menu panel.

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

Comment 5

[Mike de Boer [:mikedeboer]]

The funky selectors are solely due to selector precedence. I also kinda like mirroring the ones I'm overriding so they're easy to backtrack once we make this the default in Fx 57.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8867359 [details]
Bug 1354108 - Add the edit controls (cut, copy, paste) to the static menu panel.

https://reviewboard.mozilla.org/r/138886/#review145270

Nice, thanks!

Don't forget the followup to make left/right arrows work for these types of items. :-)

::: browser/themes/shared/customizableui/panelUI.inc.css:1205
(Diff revision 2)
> +}
> +
> +photonpanelmultiview .PanelUI-subView .toolbaritem-combined-buttons > .subviewbutton {
> +  height: auto;
> +  margin-inline-start: 18px;
> +  min-width: auto;

There's a non-photon rule (namely: .toolbaritem-combined-buttons@inAnyPanel@ > toolbarbutton, https://dxr.mozilla.org/mozilla-central/rev/9851fcb0bf4d855c36729d7de19f0fa5c9f69776/browser/themes/shared/customizableui/panelUI.inc.css#1423-1435 ) that gives these -moz-box-flex: 1 so on my machine these buttons are now 29px wide + padding, or something.

You either need a max-width: 24px to override the other non-photon max-width rule, or set -moz-box-flex: 0 in this rule. The latter seems more in keeping with your decision to use height/min-width: auto.

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8867359 [details]
Bug 1354108 - Add the edit controls (cut, copy, paste) to the static menu panel.

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

Comment 8

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs from comment #6)
> You either need a max-width: 24px to override the other non-photon max-width
> rule, or set -moz-box-flex: 0 in this rule. The latter seems more in keeping
> with your decision to use height/min-width: auto.

Awesome you provided the solution for me here, thanks! :)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51114054d776
Add the edit controls (cut, copy, paste) to the static menu panel. r=Gijs

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/51114054d776

Comment 11

[u554753]

Verified on Windows, Mac, and Ubuntu.