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

Maximized window has a White border around #6258

Closed
srirambv opened this issue Dec 16, 2016 · 12 comments
Closed

Maximized window has a White border around #6258

srirambv opened this issue Dec 16, 2016 · 12 comments

Comments

@srirambv
Copy link
Collaborator

Did you search for similar issues before submitting this one?

Describe the issue you encountered:
Maximzed window has a White border around

Expected behavior:
Should not have thick border when browser window is maximized

  • Platform (Win7, 8, 10? macOS? Linux distro?):
    Windows 10 x64

  • Brave Version:
    0.13.0 Preview 4

  • Steps to reproduce:

    1. Do a clean install of 0.13.0 preivew 4
    2. Maximize the browser window
    3. Thick white border is shown around the browser window
  • Screenshot if needed:
    image

  • Any related issues:
    cc: @bsclifton

@srirambv srirambv added this to the 0.13.0 milestone Dec 16, 2016
@bsclifton bsclifton self-assigned this Dec 19, 2016
@bsclifton bsclifton changed the title Maximzed window has a White border around Maximized window has a White border around Dec 19, 2016
@bsclifton
Copy link
Member

bsclifton commented Dec 20, 2016

I think the issue involves the thickFrame setting for BrowserWindow (which is defaulting to true):

thickFrame Boolean (optional) - Use WS_THICKFRAME style for frameless windows on Windows, which adds standard window frame. Setting it to false will remove window shadow and window animations. Default is true.

@bsclifton
Copy link
Member

Setting thickFrame to false does resolve the padding around the borders... but it introduces a new set of problems. Window is not responding to being double clicked as maximized anymore, like the above states, it removes animations... it doesn't feel right. Also (didn't investigate too much), it doesn't seem to properly fire the maximize/unmaximize events when this is set

Will look into the actual window styles being passed in via NativeWindowViews

@bsclifton
Copy link
Member

Adding a ton more logging in here; here's what I've found so far. Some good clues I think 😄

  • Using Spy++, I determined the Win32 window styles (regular and extended) between Brave 0.12.15 (Chromium 53) and HEAD (Chromium 54) are exactly the same when window is regular and maximized.
  • Also using Spy++, the window sizes appear to be consistent between Brave 0.12.15 (Chromium 53) and HEAD (Chromium 54). This includes the window rect and the client rect.
  • Using HEAD (Chromium 54), the window looks fine when not maximized. When maximized, it gets the borders. However, when set to fullscreen, it does NOT have the borders.
  • NativeWindowViews (electron) is the implementation for electron's BrowserWindow. It uses the underlying Google Chromium views::Widget class to implement the window. For the init params, there is a window type called "TYPE_WINDOW_FRAMELESS" but NativeWindowViews always uses TYPE_WINDOW. If we change this to be frameless, it crashes without logging any errors.

@bsclifton
Copy link
Member

Reproduced in a bare-bones app; I copied the pre-built binaries over the ones included with @jonathansampson's Muon Quick repo and was able to reproduce the issue. Only tweaks were:

  • setting frame:false in the new BrowserWindow call
  • calling maximize explicitly (since there are no min/max buttons)

screen shot 2016-12-26 at 10 25 35 pm

@bsclifton
Copy link
Member

bsclifton commented Dec 27, 2016

OK I'm knee deep in this one (gotta be really close).

I'll be updating this as I trace each of the steps (for my own sanity!):

  • Window is in NORMAL state:

    • [NativeWindowViews::GetBounds]; x/y[-8,-8]; w/h[2560,1279]
    • [Window::GetSize]; w/h[2560,1279]
  • User maximizes window:

    • Maximize event happens
      • NativeWindowViews::Maximize
      • Widget::Maximize
      • DesktopNativeWidgetAura::Maximize
      • DesktopWindowTreeHostWin::Maximize
      • HWNDMessageHandler::Maximize (sends WM_SYSCOMMAND)
    • Event is received by handler
      • [NativeWindowViews::PreHandleMSG]; call DefWindowProc for WM_SYSCOMMAND
      • [NativeWindowViews::PreHandleMSG]; call DefWindowProc for WM_GETMINMAXINFO
      • [NativeWindow::GetContentSizeConstraints]
      • [NativeWindow::GetContentSizeConstraints]
      • [NativeWindowViews::PreHandleMSG]; call DefWindowProc for WM_WINDOWPOSCHANGING
      • [NativeWindowViews::PreHandleMSG]; call DefWindowProc for WM_GETMINMAXINFO
      • [NativeWindow::GetContentSizeConstraints]
      • [NativeWindow::GetContentSizeConstraints]
      • [NativeWindowViews::PreHandleMSG]; call DefWindowProc for WM_NCCALCSIZE
      • [NativeWindowViews::PreHandleMSG]; call DefWindowProc for WM_NCPAINT
      • [NativeWindowViews::PreHandleMSG]; call DefWindowProc for WM_ERASEBKGND
    • Resize happens
      • [NativeWindowViews::PreHandleMSG]; call DefWindowProc for WM_WINDOWPOSCHANGED
      • [NativeWindow::GetContentSizeConstraints]
      • [Window::OnWindowResize]
      • [NativeWindowViews::GetBounds]; x/y[-8,-8]; w/h[2576,1295]
        • Widget::GetWindowBoundsInScreen
        • DesktopNativeWidgetAura::GetWindowBoundsInScreen
        • DesktopWindowTreeHostWin::GetWindowBoundsInScreen
        • HWNDMessageHandler::GetWindowBoundsInScreen
        • win32 API call is made: GetWindowRect
  • End result is the bounds are set incorrectly. For example, on my monitor (100% DPI) with resolution of 2560 x 1319, Maximize is consistently setting its size as 2576 x 1295.

@bsclifton
Copy link
Member

Got it! Fix coming up...

@bsclifton
Copy link
Member

Fixed with brave/muon#134. Closing out issue

@bsclifton
Copy link
Member

Reopening after @jonathansampson found this happens when on 200% DPI. Also there are two DPI related issues (both happening at 125%). I'll need to dig deeper into the Electron code. Here are the two possibly related browser-laptop issues:
#6462
#6468

My best guess at the moment is the root cause may be related to this commit:
brave/muon@182fe6d

I believe I properly solved this issue but the commit above may have masked it with Chromium 53. When I upstreamed it with electron/electron#7416 it was reverted because the tests failed.

@bsclifton bsclifton reopened this Dec 31, 2016
@bsclifton
Copy link
Member

bsclifton commented Jan 1, 2017

I removed the commit references above and it didn't fix the issue... but the fact that it's still shows the borders means there's another bug (which may be the actual root cause for what I was trying to solve in the muon commit in the last post above). Got some debug statements in there, gonna hunt it down 😄

@bsclifton
Copy link
Member

I believe the two issues I referenced above are NOT related to this issue (at least not directly). They may have a different root cause
#6468
#6462

bsclifton added a commit to brave/muon that referenced this issue Jan 12, 2017
- don't process non-client calcsize/paint
- do proper DPI calculations when DPI > 100%

Fixes brave/browser-laptop#6258

Reverts 182fe6d
Which I believe incorrectly fixed the problem with Chromium 53
(see discussion at electron/electron#7416 for more info)

We need to test to confirm, but I believe this change will fix:
brave/browser-laptop#4502
brave/browser-laptop#6462
brave/browser-laptop#6468
brave/browser-laptop#6481
@luixxiul
Copy link
Contributor

Test plan: available on the 1st post.

@luixxiul
Copy link
Contributor

I added release-notes/exclude as the issue appeared and was fixed within the same milestone.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants