1354095 - Add 'New Window' and 'New Private Window' buttons to static hamburger menu

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

These buttons will look and behave the same as the equivalent buttons in the current menu panel.

The icon will be smaller; the size to be the same as all other menu item icons.

The captions remain the same as well as the associated command will stay the same.

Please take a possible future hotkey assignment into account. (no pun intended.)

A separator is shown below the 'New Private Window' menu item.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1354095 - add new {,private} window buttons to photon panel

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

Comment 2

[:Gijs (he/him)]

So, as I worked on this I discovered an unfortunate aspect here - it's a bit tricky to get this to look good with the current styling. We basically assume, in a number of places, that we use the vertical layout for all subviews and standalone subviews, but never for the main panel. The layout is also supposed to work for items with checkmarks (as well as icons) in front of them, on OS X, which the design in bug 1353362 doesn't really cater for.

My plan is to just ensure that items look (more or less) like other menu items do today, and to tweak the remaining bits when bug    1353360 has also landed. Mike, does that sound reasonable? (Feel free to check out the current patch to see what I mean.)

Comment 3

[:Gijs (he/him)]

Comment on attachment 8857213 [details]
Bug 1354095 - add new {,private} window buttons to photon panel

https://reviewboard.mozilla.org/r/129146/#review131644

::: browser/base/content/browser.xul:1049
(Diff revision 1)
>                       label="&printButton.label;"/>
>  
>  
>        <toolbarbutton id="new-window-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
>                       label="&newNavigatorCmd.label;"
> -                     command="key_newNavigator"
> +                     command="cmd_newNavigator"

Noticed this when copy-pasting this. This has been here for donkeys' years, and apparently works, but I figured I should fix it while here. :-\

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs from comment #2)
> My plan is to just ensure that items look (more or less) like other menu
> items do today, and to tweak the remaining bits when bug    1353360 has also
> landed. Mike, does that sound reasonable? (Feel free to check out the
> current patch to see what I mean.)

That sounds very reasonable. I'll take care of the hairy bits ;-)

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8857213 [details]
Bug 1354095 - add new {,private} window buttons to photon panel

https://reviewboard.mozilla.org/r/129146/#review132028

::: browser/components/customizableui/content/panelUI.inc.xul:497
(Diff revision 1)
>         type="arrow"
>         hidden="true"
>         flip="slide"
>         position="bottomcenter topright"
>         noautofocus="true">
> -  This space intentionally left blank.
> +  <panelmultiview id="PanelUI-photon-multiView" mainViewId="PanelUI-photon-mainView">

Rather than introducing a new cumbersome naming scheme (PanelUI-photon-*) that we might end up dragging along for years, you might want to use this opportunity to start using "appMenu", see bug 1289594.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8857213 [details]
Bug 1354095 - add new {,private} window buttons to photon panel

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

Comment 7

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #5)
> Rather than introducing a new cumbersome naming scheme (PanelUI-photon-*)
> that we might end up dragging along for years, you might want to use this
> opportunity to start using "appMenu", see bug 1289594.

Good point. Done. Should we dupe that bug to either the metabug or this bug or something?

Comment 8

[:Gijs (he/him)]

... or just mark wontfix, I guess.

Comment 9

[Dão Gottwald [:dao]]

Or just marked as FIXED by this bug. That we're adding a new panel rather than renaming the existing one is an unimportant detail of how we're developing photon.

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8857213 [details]
Bug 1354095 - add new {,private} window buttons to photon panel

https://reviewboard.mozilla.org/r/129146/#review134250

Apology for the delay, Gijs! This looks very good, thanks.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1eb86cbfac79
add new {,private} window buttons to photon panel r=mikedeboer

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[u554753]

Attachment: Win 10 / Win 7 different button layouts

When verifying this on Windows 10 and Windows 7, I noticed a difference between the two operating systems. Is this intended?

Comment 14

[:Gijs (he/him)]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #13)
> Created attachment 8861123 [details]
> Win 10 / Win 7 different button layouts
> 
> When verifying this on Windows 10 and Windows 7, I noticed a difference
> between the two operating systems. Is this intended?

This doesn't look like you toggled browser.photon.structure.enabled, so you're not testing the new UI, you're testing the old UI. To verify, you'll need to toggle that Photon structure pref.

The difference you're seeing is due to the different e10s settings on the two browsers - one has an "open a non-e10s window" button, the other doesn't.

Comment 15

[u554753]

Thanks! I will give this another go.

Comment 16

[u554753]

Verified on Windows 10, Windows 7, Mac, and Ubuntu.