Closed Bug 1354095 Opened 3 years ago Closed 3 years ago

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

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
Firefox 55
Iteration:
55.4 - May 1
Tracking Status
firefox55 --- verified

People

(Reporter: mikedeboer, Assigned: Gijs)

References

(Blocks 2 open bugs, )

Details

(Whiteboard: [photon-structure])

Attachments

(2 files)

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.
No longer depends on: 1354087
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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.)
Flags: needinfo?(mdeboer)
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. :-\
(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 ;-)
Flags: needinfo?(mdeboer)
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.
Flags: qe-verify+
Priority: -- → P1
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Attachment #8857213 - Flags: review?(mdeboer)
(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?
Flags: needinfo?(dao+bmo)
... or just mark wontfix, I guess.
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.
Blocks: 1289594
Flags: needinfo?(dao+bmo)
Iteration: --- → 55.4 - May 1
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.
Attachment #8857213 - Flags: review?(mdeboer) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1eb86cbfac79
add new {,private} window buttons to photon panel r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/1eb86cbfac79
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
When verifying this on Windows 10 and Windows 7, I noticed a difference between the two operating systems. Is this intended?
Flags: needinfo?(mdeboer)
(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.
Flags: needinfo?(mdeboer)
Thanks! I will give this another go.
Verified on Windows 10, Windows 7, 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.