Optimize titlebar space on Windows #3854

bsclifton · 2016-09-09T21:33:15Z

Changes done, only a few minor bugs open (see checklist below under known issues)

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).

Fixes #3036

Lots of changes; this PR:

Menubar and caption button changes only affect Windows
- removes the standard menubar from the Window
- custom renders the caption buttons (the minimize / maximize / close buttons)
- custom renders a menubar (if visible). Can be toggled with ALT (left and right ALT both work).
- Windows 10, 8.1, 8, or other will use Windows 10 styles
- Windows 7 will use Windows 7 styles
Functionality for Linux was removed- this is captured separately with Titlebar on some Linux distros has a lot of wasted space #3126
Keyboard accelerators are now shown in context menus. This affects Mac and Linux

It also includes:

remove unused getParentMenuDetails method

Auditors

@bbondy @bridiver @bradleyrichter @Sh1d0w

Test Plan

Windows

Styles should match the mockup for the respective OS

Custom rendered menubar

Menubar should be hidden by default with a fresh session
If enabled, the menu will show all the time
When toggled, the lion head should not move horizontally
Menu should be usable in full screen mode (F11)

Custom render caption buttons

Minimize / Maximize / Close
Tooltip shows if you hover over it
Buttons should work
When maximized, maximize button turns into a restore icon
Going full screen (F11) should hide the caption buttons

Using menubar w/ the keyboard

Pressing any arrow keys should not interfere with the frame (unless menu is triggered using ALT)
Menu should toggle between shown and hidden with ALT
Navigation
- When shown, menu will automatically highlight the "File" menu
- When menu is "highlighted", you should be able to use arrow keys to move left and right and also up/down to:
  - show the initial context menu (the items under File or Edit, for example)
  - move your selection up/down, including wrapping around choose a different item
- Once a menu item is highlighted, you should be able to trigger it by pressing ENTER
menu can be hidden at any time (even if it's popped up) by pressing ALT or ESCAPE
Menu should be readable/usable with usability options enabled:
Menu hides after choosing an item (unless auto-hide is disabled)
Keyboard shortcuts making use of ALT should still work fine (and not toggle the menu)

Using menubar w/ the mouse

You can click any menu and it'll show the associated popup
If you clicked File, you should be able to move your mouse another item, like Edit, and it should automatically:
- close the popup associated with File
- open the popup associated with Edit
- change the selection behind the menu name
Menu hides after choosing an item (unless auto-hide is disabled)
Picking copy or paste from Edit menu should not affect focus of the input box on a given page
Clicking in the middle of a page should unselect the menu item and hide any popup menus that were showing

Other style changes

window should have a 1 pixel black border around it now
Context menus now show keyboard shortcuts
Menus should be displayable in other languages without issue
Window should be able to be made smaller without screwing up the URL bar / caption button area

Misc things to try

toggle the auto-hide menu bar (also can be triggered by right clicking in bookmarks toolbar or tab row and picking "Menu bar")
try using the long press back button / forward button. These also use our custom context menus
toggle the magic-titlebar (always show URL) setting. Does the window do anything weird during transition?
resize the window (small, big, push F11 and go fullscreen)

Mac / Linux

Custom titlebar should not be showing
You should see the keyboard shortcuts now in our custom context menus. For example, the hamburger button.
Keyboard shortcuts should be in the proper order (shift before command, etc) and should be displayed, regardless of locale
Keyboard shortcuts should not be affected
- Alt on Linux, option on Mac
- Test keyboard shortcuts that use this
Popup elements should still work correctly
- clicking on lock for security mode
- adding / editing bookmark
- clicking lion head to bring up site specific bravery panel
- adding new autofill data in preferences

NOTE: besides the min/max/close tests (which aren't runnable), there are no other automated tests. I'd like to create some, but webdriver tests on Windows are not working

Known issues (will be working through these)

Issues towards the top of each respective category are higher priority. Issues which are considered show-stoppers are bolded (please feel free to edit this comment).

Issues with the checkbox are FIXED (see follow up commits for fix)

functional issues

style issues

back/forward buttons are oval shaped (not circle) also causes spacing to be messed up:
space between menu text needs to be 22px
Font for the menu can be much larger than app, because it uses system size (we can override this and make it a static size, if we'd like)
Windows caption buttons are supposed to show a title of "Maximize" or "Minimize" when mouse is left hovering over them
Context menu accelerators need restyling (need to be the same size as regular text)
Needs to show border
- Windows 10 always puts a 1 pixel border with the users highlight color around each window... instead, a box shadow is drawn (which looks nice, but isn't consistent with OS)
- Windows 7 doesn't use any effects around the border and when you're on a white page, it can be really confusing- because there is no borders and the white will blend in (making it hard to tell what is in the browser and what isn't)
Window needs a smaller min-width than 700px; currently, when less than that, the caption buttons get messed up.

bbondy · 2016-09-12T00:32:40Z

btw this needs a rebase on master when you get time

bsclifton · 2016-09-12T04:11:34Z

OK properly rebased and verified 😄

bsclifton · 2016-09-13T23:01:40Z

Ready for review- @bbondy @srirambv @darkdh any feedback is appreciated 😄 I'll try to find some Windows contributors who can also try it and give feedback

bsclifton · 2016-09-13T23:36:34Z

Just a heads up- this branch can be tested on Mac, BTW

To make sure I didn't break anything
To see the new sweet keyboard accelerators in the context menus (burger menu for example)

srirambv · 2016-09-14T09:15:20Z

@bsclifton The new look is really good and looks neat.

Some observations:

When custom title bar is disabled and enabled it shows two sets of windows buttons
Block script and Favorite icons are not properly aligned when hide URL bar is enabled
File menu/Hamburger menu text size is very small compared to the current version which could be a problem
Window buttons are shown even if the browser is in fullscreen mode
Mouse over highlight doesn't happen on some of the menu items
If the window is too small then the Sheilds and Password manager icons are hidden. It is displayed when you are on one of the about pages. The window buttons gets stacked one above the other

bsclifton · 2016-09-14T20:49:15Z

Thanks for testing, @srirambv 😄

Feature removed
is a good concern; I'd like to defer to @bbondy and @bradleyrichter. Basically, if you go into settings, I've made this new feature configurable (enabled by default, can be turned off). If you do choose to toggle it, you are prompted by a "Restart needed. Do it now? yes/no". However you're not forced to do this. What do you all think is a good experience?
- ~~Force a restart by bringing up an OK/cancel dialog?~~
- ~~remove it as a configurable option for now?~~
- ~~other options?~~
Already tracked with Block script icon is off location when URL bar is hidden #4007
I'll defer to @bradleyrichter- the font size is 12px (roughly 9pt) which is the default system size. On bigger monitors, this might be unacceptable.
~~great catch! 😄 I'll add that to my TODO list~~ resolved
~~will need to investigate... I definitely did not have that experience~~ tracked with Windows 7 - selection problems with first item in custom menu #4142
is already captured in my help needed, trying to make the window respond better when below 700px width

srirambv · 2016-09-14T22:29:17Z

@clifton Would suggest a Ok/Cancel dialog than the notification bar. Goes along with the standard windows alert messages. This way the window remains as is and only applies the change after a restart is done.

bradleyrichter · 2016-09-14T23:32:28Z

@srirambv These mini-screen captures are priceless. And addictive!

@bsclifton I'm confused about #1. We never want to see the standard non-custom titlebar. That is the whole point of this effort. Am I missing something?

Font size - we need to match the standard default in all cases - somehow copying the approach that WIN uses for it's view settings. Later we can try to support the user-overrides from the system.

bsclifton · 2016-09-14T23:42:00Z

@bradleyrichter for 1, that is a completely acceptable answer 😄 I made it toggle-able just in case there were folks with accessibility requirements (which the custom menu may not work). We can always revisit that later though

edit: this config option has now been removed

bbondy · 2016-09-15T16:30:07Z

Looking a lot better than my last pass.

A couple of things I noticed.

I think people will complain that they can't use the keyboard up / down arrows / right arrow to go to next menu, left for previous menu. Alt + arrows to navigate to item and hit enter to open. This is probably a bigger thing but we should handle it. As a side note, it'd be nice to have this for hamburger menu too.
Items like cut, copy, paste, paste with formatting (possibly others) are taking focus away from the page and then the operation doesn't work. This is both for if your cursor is in the urlbar or just selecting something on a page.
Minor thing but lion head looks like it needs a couple px more padding on the right to me.
The top right corner for resizing the window is really hard to make shot up. Whereas works reliably for other windows.

- Removed border on top since that now works; starting to finalize the X with Brad - Windows 7 fixes - fix button size - new svg for close - fix margin - color updates - updated naming in main.js; menubar text no longer selectable

- Caption buttons and border are now hidden when a window goes fullscreen - Fix context menu accelerator size (windows only)

- selection can be made left and right - menu can be popped up with up/down known issues (what's left) - storing the context menu's selected index in windowStore - properly handling up/down (changing selection) - handling submenus also includes: - small cleanup for custom titlebar in main.js - lint cleanup

Menu fixes - You can now hit enter to pick a selected menu item - Fixed bug where menu opened by keyboard was not clickable Cleanup - Added reducer property which retuns a menu minus the separator items - Moved wrappingClamp method into formatUtils

- Menu now sets default selection when you press ALT to show it - Menu now behaves properly when always displayed (auto-hide=false) - ALT and ESC now are properly handled; providing windows-like behavior (deselect and/or closing menu, etc) - Fix click problem with extension button (it was tagged as draggable) - Windows specific fixes for drag. Tested each area that was changed, comparing this build to 0.12.1 to make sure it's consistent. - menu now returns focus properly to active web frame - the pseudo click now sends over webContents (required for cut/copy) - Bumped up minimum viewport width to 640px for Windows only. Media query strips margin left of the brave button unless you're at 720px or more (again- windows only)

- Properly handle all accelerator modifiers (was missing CommandOrControl and some replacement cases) - Updated code to skip rendering / processing for hidden context menu items

@bridiver

…from appState to windowState (not going to move it after talking w/ @bridiver).

Works well with URL bar always showing and also with title mode.

@bbondy

- capturing the browser's selection changes - converting the activeElement object to a selector - returning focus after menu is interacted with by: - using document.querySelectAll() to return focus to original element - executing window action which actually handles the menu action This commit also cleans up `main.js` by properly encapsulating the handlers required into a new method, `registerCustomTitlebarHandlers` cc: @bbondy

bbondy · 2016-09-21T04:22:43Z

Let's get this merged and do anything else as follow ups. Nice work on all of this 🎉

bsclifton force-pushed the windows-titlebar-diet branch 2 times, most recently from 01989e4 to bb4db62 Compare September 10, 2016 18:36

luixxiul added the OS/Windows label Sep 11, 2016

bsclifton force-pushed the windows-titlebar-diet branch 2 times, most recently from dd23cbd to 5a1299b Compare September 12, 2016 04:11

bsclifton force-pushed the windows-titlebar-diet branch from 9303351 to 920c27f Compare September 12, 2016 08:26

bbondy added this to the 0.12.2dev milestone Sep 12, 2016

bsclifton force-pushed the windows-titlebar-diet branch 8 times, most recently from bea276f to 48d73bf Compare September 13, 2016 21:10

bsclifton force-pushed the windows-titlebar-diet branch from 7d3b488 to d6199cd Compare September 13, 2016 23:52

bsclifton force-pushed the windows-titlebar-diet branch 2 times, most recently from e258902 to 37474ea Compare September 15, 2016 06:56

bsclifton force-pushed the windows-titlebar-diet branch 2 times, most recently from 601904f to ecbf741 Compare September 16, 2016 23:02

bsclifton added 9 commits September 20, 2016 16:51

Fixing known issues:

2e0c1ec

- Removed border on top since that now works; starting to finalize the X with Brad - Windows 7 fixes - fix button size - new svg for close - fix margin - color updates - updated naming in main.js; menubar text no longer selectable

Fixing known issues:

c30a607

- Caption buttons and border are now hidden when a window goes fullscreen - Fix context menu accelerator size (windows only)

Rev2 on the keyboard controlled menus

39fe326

Menu fixes - You can now hit enter to pick a selected menu item - Fixed bug where menu opened by keyboard was not clickable Cleanup - Added reducer property which retuns a menu minus the separator items - Moved wrappingClamp method into formatUtils

Fixes:

02564f1

- Properly handle all accelerator modifiers (was missing CommandOrControl and some replacement cases) - Updated code to skip rendering / processing for hidden context menu items

Fix issue w/ last commit, added cleanup after evaluating moving menu …

c0240a5

…from appState to windowState (not going to move it after talking w/ @bridiver).

Window can be as skinny as 480px wide on Windows now without issues.

75365f0

Works well with URL bar always showing and also with title mode.

bsclifton force-pushed the windows-titlebar-diet branch from 74c77ff to 39d2b0c Compare September 20, 2016 23:58

bbondy merged commit 53adb9d into master Sep 21, 2016

Sh1d0w deleted the windows-titlebar-diet branch September 21, 2016 04:30

This was referenced Sep 22, 2016

Manual tests for OSX 0.12.2 RC1 #4163

Closed

Manual tests for Windows ia-32 0.12.2 RC1 #4164

Closed

Manual tests for Ubuntu 0.12.2 RC1 #4165

Closed

Manual tests for Windows x64 0.12.2 RC1 #4166

Closed

luixxiul added the feature/titlebar label Sep 22, 2016

This was referenced Sep 22, 2016

Manual tests for OS X 0.12.2 RC2 #4204

Closed

Manual tests for Windows ia-32 0.12.2 RC2 #4205

Closed

Manual tests for Windows x64 0.12.2 RC2 #4206

Closed

Manual tests for Ubuntu 0.12.2 RC2 #4207

Closed

luixxiul added the QA/checked-Win32 label Sep 25, 2016

bsclifton mentioned this pull request Sep 29, 2016

Caption buttons are not shown when in fullscreen mode (Windows) #4376

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize titlebar space on Windows #3854

Optimize titlebar space on Windows #3854

bsclifton commented Sep 9, 2016 •

edited

Loading

bbondy commented Sep 12, 2016

bsclifton commented Sep 12, 2016

bsclifton commented Sep 13, 2016

bsclifton commented Sep 13, 2016 •

edited

Loading

srirambv commented Sep 14, 2016

bsclifton commented Sep 14, 2016 •

edited

Loading

srirambv commented Sep 14, 2016

bradleyrichter commented Sep 14, 2016

bsclifton commented Sep 14, 2016 •

edited

Loading

bbondy commented Sep 15, 2016

bbondy commented Sep 21, 2016

Optimize titlebar space on Windows #3854

Optimize titlebar space on Windows #3854

Conversation

bsclifton commented Sep 9, 2016 • edited Loading

Auditors

Test Plan

Windows

Styles should match the mockup for the respective OS

Custom rendered menubar

Custom render caption buttons

Using menubar w/ the keyboard

Using menubar w/ the mouse

Other style changes

Misc things to try

Mac / Linux

Known issues (will be working through these)

functional issues

style issues

bbondy commented Sep 12, 2016

bsclifton commented Sep 12, 2016

bsclifton commented Sep 13, 2016

bsclifton commented Sep 13, 2016 • edited Loading

srirambv commented Sep 14, 2016

bsclifton commented Sep 14, 2016 • edited Loading

srirambv commented Sep 14, 2016

bradleyrichter commented Sep 14, 2016

bsclifton commented Sep 14, 2016 • edited Loading

bbondy commented Sep 15, 2016

bbondy commented Sep 21, 2016

bsclifton commented Sep 9, 2016 •

edited

Loading

bsclifton commented Sep 13, 2016 •

edited

Loading

bsclifton commented Sep 14, 2016 •

edited

Loading

bsclifton commented Sep 14, 2016 •

edited

Loading