Closed Bug 1354108 Opened 3 years ago Closed 3 years ago

Add a restyled edit control to static hamburger menu

Categories

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

enhancement

Tracking

()

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

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [photon-structure])

Attachments

(1 file)

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.
Note: the tooltips may contain the hotkeys associated with the respective button command.
Blocks: 1354113
No longer depends on: 1354105
No longer blocks: 1354113
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Depends on: 1359625
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Priority: P2 → P1
Iteration: --- → 55.5 - May 15
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?
Attachment #8867359 - Flags: review?(gijskruitbosch+bugs)
Iteration: 55.5 - May 15 → 55.6 - May 29
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 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.
Attachment #8867359 - Flags: review?(gijskruitbosch+bugs) → review+
(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! :)
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/51114054d776
Add the edit controls (cut, copy, paste) to the static menu panel. r=Gijs
Depends on: 1367012
https://hg.mozilla.org/mozilla-central/rev/51114054d776
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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.