Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Add custom title bar Fixes #3126 #3036 #3133

Closed
wants to merge 6 commits into from
Closed

Add custom title bar Fixes #3126 #3036 #3133

wants to merge 6 commits into from

Conversation

Sh1d0w
Copy link

@Sh1d0w Sh1d0w commented Aug 11, 2016

Reviewers: @bbondy @luixxiul @bradleyrichter

Preview is available here: http://webm.land/media/g4DR.webm

@bsclifton
Copy link
Member

After digging in more, maybe we can work together to accept this 😄 The electron route I was looking at is a very long-term kind of change.

I don't think it would be too hard to save the native windowing environment's bitmaps and then have this code reference the image. I'm working on some issues related to History, but afterwards I'd love to take your commits and see if we can make a great solution together 😄

@bsclifton bsclifton self-assigned this Aug 13, 2016
@bbondy
Copy link
Member

bbondy commented Aug 13, 2016

@bsclifton I think we could just draw it with css instead of using bitmaps directly. It might be better for dpi scaling reasons if we do it manually without a bitmap. Tweaking the styles if needed on what we have in this PR.

@@ -71,6 +71,7 @@
"homepage": "https://www.brave.com/",
"dependencies": {
"abp-filter-parser-cpp": "1.1.x",
"electron-drag": "^1.2.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this since we don't use it already for macOS?

@Sh1d0w
Copy link
Author

Sh1d0w commented Aug 13, 2016

@bbondy Thanks. I removed not needed deps and tweaked the code as you suggested. Will rebase once we agree this PR is ready for merge.

@bsclifton Cool man. Let me know once you are ready to work on the styling of the buttons.

Meanwhile if you just hint me how I can get the OS theme styles for the buttons I can do it by myslef. I did a lot of digging but so far I could not find how I can achieve it.

I was thinking to implement predefined CSS for each platform's buttons and to apply the right css style depending on the platform. This way it will work for most windows users as the UI is almost the same, but for Linux you have plenty of themes outthere, so we need to fetch the theme styles somehow.

@bsclifton
Copy link
Member

@Sh1d0w starting off with the stock look and feel would be a great start 😄

Windows 7 would have this kind of styling (similar to stock, except for colors):
image

Windows 10 would be stock. Here's an example of how that looks:
79fd43dc-5d46-11e6-9550-c4d41179e733

I have Windows 7 and 10 VMs ready to go, let me know if you need screenshots to help w/ the styling (how each looks with the mouse over, dimensions of each, etc).

Getting actual OS themes represented via CSS is a much larger task which I think is safe to defer.

@bsclifton
Copy link
Member

bsclifton commented Aug 13, 2016

Here is how you can check for the OS version. The process.platform checks are already in place, which return 'win32'. To narrow down which version of Windows, you can use the os module:

const os = require('os')
const isWindows10 = /10.0./.test(os.release())
const isWindows7 = /6.1./.test(os.release())

@Sh1d0w
Copy link
Author

Sh1d0w commented Aug 13, 2016

@bsclifton I could really use your help, if you can provide me with a codepen with the windows styled buttons? I have made some styling but I am not familiar with the animations of the windows environment, etc. Or at least if you can provide me with HQ screenshots as these are not with good resolution to crop the symbols for the buttons?

I have made some example styles:

Default look on Linux:

http://webm.land/media/n4G2.webm

Default look on Win7 and Win10:

http://webm.land/media/0bzf.webm

In the future for Linux platfrom we can further customize the look and feel for Unity, Gnome, XFCE and KDE looks, but for now I think mac os look fit very good for Linux. Several other electon apps like Nylas, Kitematic use it as default look.

Of course if you provide any other default design you wish to put there I can change it :)

@luixxiul
Copy link
Contributor

@Sh1d0w it looks nice for Windows! I would like to see where and how the menubar will appear.

@Sh1d0w
Copy link
Author

Sh1d0w commented Aug 13, 2016

@luixxiul Thanks. I am really excited as well about this compact look. Very polished and professional looking.

Good you mentioned menus. I havent thinked of it. Since we hide the native window frame, we have no more the native menus as well, just noticed that.

I guess I will have to do custom component for menus as well, with support for keyboard shortcuts.

Question: Since we duplicate the native menu actions in the hamburger menu under the brave shields icon on the top right, can't we just use this menu item? Or you want to have the horizontal native menu at the top as well when you hit ALT? Let me know what do you think will be better guys.

cc @bbondy

@bbondy
Copy link
Member

bbondy commented Aug 13, 2016

I think that's why Chrome doesn't do a top menu. If we had everything in the hamburger then I think that would be ok but I don't think we have everything today. CC'ing @bradleyrichter for thoughts.

BTW about the bitmap stuff, whatever is easiest, just note that if you use bitmaps then we'll need different dpi images. Maybe you can convince @bradleyrichter to create some svg for you or something that match win7 and win10.

@Sh1d0w
Copy link
Author

Sh1d0w commented Aug 14, 2016

@bbondy Yes that was exactly what I was thinking. I can add all menu elements in the native menu to the hamburger menu (these that are not currently presented there)? Let me know if you want me to do that.

@bradleyrichter Can you create some SVGs for Windows 7 and Windows 10, that I can use in the styling, please? 😄

@bsclifton
Copy link
Member

bsclifton commented Aug 14, 2016

This is looking great; awesome job, @Sh1d0w! 😄

Here's something I put together real quick with Windows 10 (note: the blue outlines are my theme's color- even with a frameless window, we already get that)
image

The first 7 rows are when the window is not maximized.
Min/max/close are each the same size at 45 pixels wide by 29 pixels tall

  • row 1 is default
  • row 2, 3, 4 show hover state
  • row 5, 6, 7 show click state

The next two rows are when the window is maximized.
Min/max/close are still the same size as each other, but this time 45 wide x 21 tall
Note that the maximize icon is different than the one in the non-maximized state

@Sh1d0w
Copy link
Author

Sh1d0w commented Aug 14, 2016

@bsclifton Thanks man, really helpful! I have tinkered a little bit with your image and converted the buttons to SVGs, then applied some CSS. Here is the final result:

https://gfycat.com/RequiredHappygoluckyDuckbillcat

Let me know what you think 😄

cc @bbondy @luixxiul

PS. Tonight I will convert the Linux buttons to SVGs as well. I just need confirmation if you guys are ok with mac style buttons for Linux? I think it fits pretty well.

@bradleyrichter
Copy link
Contributor

bradleyrichter commented Aug 14, 2016

This is looking great guys.

Windows 7 will need something a bit different in order to blend in. I'll work on that today.

Also, we are moving the brave button and hamburger so let's time this carefully so that we don't end up with several changes on the way to 1.0.

On Aug 14, 2016, at 2:24 AM, Radoslav Vitanov notifications@github.com wrote:

@bsclifton Thanks man, really helpful! I have tinkered a little bit with your image and converted the buttons to SVGs, then applied some CSS. Here is the final result:

https://gfycat.com/RequiredHappygoluckyDuckbillcat

Let me know what you think ??

cc @bbondy @luixxiul


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[--- Commented from Asana.com
#commenter brad richter
---[aa]

@Sh1d0w
Copy link
Author

Sh1d0w commented Aug 14, 2016

@bradleyrichter Thanks. The new buttons are implemented as a single component, which can be moved wherever you wish in the template, so I don't think that will cause any conflicts with the changes you are already implementing 😄

No one mentioned the Linux version look, so I guess we will go with the macOS like buttons that I proposed then?

@bsclifton
Copy link
Member

The Linux version will be tricky. Let's leave that alone for now until we have a more clear strategy. We can treat that as a separate issue, because of the design considerations

@bsclifton
Copy link
Member

bsclifton commented Aug 14, 2016

BTW- awesome job wrapping this up in a component 😄 This will make enabling on Linux easy once we figure out how we'd like it

One of the challenges we'll have to solve before accepting the PR is allowing for the menu bar. Mac is already fine, but Windows and Linux (Unity excluded) need to have the menu at the top of the BrowserWindow object. Electron doesn't support frameless with a menu yet (that I know of). I'll start looking into this today

@Sh1d0w
Copy link
Author

Sh1d0w commented Aug 14, 2016

@bsclifton One possible solution is to implement custom component for the top menu. But I was left with the impression that we will go Chrome's way with only a hamburger menu from the last comment from @bbondy ?

About the Linux styling, we can try to detect the Desktop environmnet of the current session on startup with executing simple bash script like this one (eigther from whitin node or bash):

if [ "$XDG_CURRENT_DESKTOP" = "" ]
then
  desktop=$(echo "$XDG_DATA_DIRS" | sed 's/.*\(xfce\|kde\|gnome\).*/\1/')
else
  desktop=$XDG_CURRENT_DESKTOP
fi

desktop=${desktop,,}  # convert to lower case
echo "$desktop"

There is probably more native way to do this with

os.release()

But I have to check the return values on all major distros atm, to make sure I can use it.

Once we know the DE used I can write button theme for each DE, like KDE, Unity, Gnome, XFCE etc. The problem will come if they have custom themes not the default one, but for me the problem with the double title bar is more annoying than a theme mismatch for 3 buttons, as it is for a lot other users. Also if Windows allows theming (which I think it does), this same problem is valid for Windows as well.

I think this problem can be fully solved only when custom theming is implemented in Brave, so it will allow the user to pick from a list of predefined CSS themes to match your Desktop Environment styles. It will be awesome also if users can create custom css themes, and just install them in the browser.

@bradleyrichter
Copy link
Contributor

There seems to be quite a bit of backlash against Chrome's removal of the traditional menu bar. The hamburger is also becoming less popular over time, especially for web sites, and this has a trickle-down effect for general app UI.

A hamburger menu which contains all of the menu items can be cumbersome to use because it requires 3 levels. The burger is meant to be filled with a collection of only the most used menu items from all menus, using minimal hierarchy.

So what to do? Ideally we can find a way to have both menus like Firefox.

Is there a way to estimate the task length to add the normal menu bar that appears with the Alt key?

@bsclifton
Copy link
Member

bsclifton commented Aug 14, 2016

I'll dig into the frameless window with menu later today- I think that one is doable but will require an electron change

edit: sorry it's taking so long to get going- I am not able to get Brave working from source on Windows 😦

@Sh1d0w
Copy link
Author

Sh1d0w commented Aug 16, 2016

Hey guys. Just quick update regarding Linux platform.

From the Electron API docs it seems we can use ELECTRON_FORCE_WINDOW_MENU_BAR so we can force to show the global menu, even if we have frame set to false. Do you think this will be a suitable solution for Linux users? Also it will worth trying it under different desktop envrinments like KDE, Gnome and Unity so I can confirm the behaviour.

Note that this is not the top menu rather than a global menu for the app shown in the panel.

@bsclifton Hey man, any info from your side? Let me know if you find something that we can use to complete this PR 😄

cc @bbondy @bradleyrichter

@bsclifton
Copy link
Member

For now, we should only consider Windows (exploring what's available on Linux is good though)

The option you mentioned is a little different- we want to have the menu work like it should natively. For example, I think Unity on Ubuntu uses a system menu, but other distros don't.

The bad news: it looks like adding a menu via calls to electron is not possible. I'm digging into electron and chromium to understand what options we have (other than faking a menu, like we did with hamburger menu)

@bsclifton
Copy link
Member

bsclifton commented Aug 24, 2016

Good news

Creating a frameless window with a menu is possible! We just need to make an electron change and then accepting this should be a no-brainer.

Bad news

I've been having trouble getting electron compiling on Windows 😦

  • won't compile with versions of VS2015 before SP2 because of a C++ language fix
  • compiles but won't link with VS2015 SP3 (latest).

Here are logs from a fresh clone:

C:\Users\brian\Documents\GitHub\upstream\electron [master ≡]> npm run bootstrap

> electron@1.3.4 bootstrap C:\Users\brian\Documents\GitHub\upstream\electron
> python ./script/bootstrap.py

C:\Users\brian\Documents\GitHub\upstream\electron [master ≡]> npm run build

> electron@1.3.4 build C:\Users\brian\Documents\GitHub\upstream\electron
> python ./script/build.py -c D

ninja: Entering directory `out\D'
[1291/1291] LINK_EMBED brave.exe
FAILED: C:\Python27\python.exe gyp-win-tool link-with-manifests environment.x64 True brave.exe "C:\Python27\python.exe gyp-win-tool link-wrapper environment.x64 False link.exe /nologo /OUT:brave.exe @brave.exe.rsp" 1 mt.exe rc.exe "obj\brave.brave.exe.intermediate.manifest" obj\brave.brave.exe.generated.manifest ..\..\atom\browser\resources\win\atom.manifest
LINK : fatal error LNK1104: cannot open file 'C:/Users/brian/Documents/GitHub/upstream/electron/vendor/brightray/vendor/download/libchromiumcontent/shared_library/addressinput_util.lib'
Traceback (most recent call last):
  File "gyp-win-tool", line 323, in <module>
    sys.exit(main(sys.argv[1:]))
  File "gyp-win-tool", line 29, in main
    exit_code = executor.Dispatch(args)
  File "gyp-win-tool", line 71, in Dispatch
    return getattr(self, method)(*args[1:])
  File "gyp-win-tool", line 179, in ExecLinkWithManifests
    subprocess.check_call(ldcmd + add_to_ld)
  File "C:\Python27\lib\subprocess.py", line 541, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command 'C:\Python27\python.exe gyp-win-tool link-wrapper environment.x64 False link.exe /nologo /OUT:brave.exe @brave.exe.rsp brave.exe.manifest.res' returned non-zero exit status 1104
ninja: build stopped: subcommand failed.

@bsclifton
Copy link
Member

bsclifton commented Aug 24, 2016

I noticed there are similar libraries, possibly one was renamed. For example:
addressinput_util.lib is missing, but there IS a file libaddressinput_util.lib

I tried to resolve that like so

cd "C:\Users\brian\Documents\GitHub\upstream\electron\vendor\brightray\vendor\download\libchromiumcontent\shared_library"
cp libaddressinput_util.lib addressinput_util.lib

When you resolve that one issue, another one pops up for phonenumber.lib and phonenumber_without_metadata.lib. I followed suit with those and then the linker moves along. It then runs into this:

os_crypt.lib(os_crypt.os_crypt_win.obj) : error LNK2019: unresolved external symbol CryptProtectData referenced in function "public: static bool __cdecl OSCrypt::EncryptString(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > *)" (?EncryptString@OSCrypt@@SA_NAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@PEAV23@@Z)
os_crypt.lib(os_crypt.os_crypt_win.obj) : error LNK2019: unresolved external symbol CryptUnprotectData referenced in function "public: static bool __cdecl OSCrypt::DecryptString16(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,class std::basic_string<wchar_t,struct std::char_traits<wchar_t>,class std::allocator<wchar_t> > *)" (?DecryptString16@OSCrypt@@SA_NAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@PEAV?$basic_string@_WU?$char_traits@_W@std@@V?$allocator@_W@2@@3@@Z)
brave.exe : fatal error LNK1120: 2 unresolved externals

I'm curious if this was fixed upstream (since this is a specific version of brightray with a submodule for libchromiumcontent

@Sh1d0w
Copy link
Author

Sh1d0w commented Aug 25, 2016

@bsclifton Good job, man! Let me know when you figure that out we can do the necessary changes to the PR if needed :)

Just merged master into my branch to keep up with the recent fixes and features.

@bsclifton
Copy link
Member

bsclifton commented Aug 25, 2016

The issues I was experienced have been resolved with:
brave/muon@6750644
brave/muon#45

I'll start looking at the Electron menu changes tomorrow morning 😄

@Sh1d0w Sh1d0w added design A design change, especially one which needs input from the design team. polish Nice to have — usually related to front-end/visual tasks. labels Aug 26, 2016
@Sh1d0w
Copy link
Author

Sh1d0w commented Sep 3, 2016

@bsclifton Hey man, any info on this menu stuff? Let me know if I can help you with something :)

@bsclifton
Copy link
Member

The problem is that with electron updated, it will show the menu above the titlebar. Here's a screenshot comparing this PR with menu enabled (top) with Firefox (bottom).
screen shot 2016-08-30 at 4 38 30 pm

Windows carves out the area with the menu bar as non-client area, which means we can't push the client area min/max/close buttons into there. I spent a lot of time trying different configurations with the raw win32 calls using a simple test program.

  • I tried an approach of taking over the non-client area and claiming it as client area. This functionally works great- the menu bar is overlaid on top of the client area. The problem is that it won't render (it gets overwritten by rendering in the client area). If you click where it's supposed to be, Windows will pop up the menu, rendering it. I cannot find any documentation to manually render the menu bar.
  • I am currently working through a different approach- defining a non-client area at the top of the window which encapsulates both the menu bar and taking ownership of this. This approach allows us to manually render the min/max/close buttons. I am seeing progress with this approach and I do think it can be applied to electron. This result (when complete) would look similar to Firefox. Here's a current screenshot:
    screen shot 2016-09-02 at 1 29 50 am

I'm going to keep digging on this one; I had opened an issue upstream in electron and will likely share details there too as I start trying to implement this.

For the time being, I'm going to close the PR. Thanks again for not only providing the change but rebasing and checking on it. I wish I had better news 😢

@bsclifton bsclifton closed this Sep 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
design A design change, especially one which needs input from the design team. polish Nice to have — usually related to front-end/visual tasks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants