New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly handle borders on frameless window for DPI > 100% (Windows only) #8404

Merged
merged 3 commits into from Jan 16, 2017

Conversation

Projects
None yet
3 participants
@bsclifton
Member

bsclifton commented Jan 13, 2017

Fixes #4573

cc: @zcbenz, @kevinsawicki, @zeke

The problems this fixes

  • The parent window tries to process non-client events for frameless. This results in borders being added (was not present with Chromium 53, but presented itself with Chromium 54)
  • Because of a bug in the menu adjustment code, the web contents object ends up being incorrectly sized. This causes borders to be added when at a DPI > 100% on Chromium 54 (53 does not show them).

History / description

I had originally submitted a "fix" with #7416, which @zcbenz accepted (and later reverted with #7461, because it failed the tests). I believe this fix masked the issue in Chromium 53.

When upgrading our fork of Electron to Chromium 54 for Brave, we noticed various issues when DPI is greater than 100%. I spent at least a week digging into the Chromium code and discovered the root causes. Remembering the reverted PR above helped when troubleshooting.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Jan 13, 2017

Contributor

@bsclifton thanks for this 👍

To test this out, is there a way to reproduce this issue against Electron master which still uses Chrome 53?

Contributor

kevinsawicki commented Jan 13, 2017

@bsclifton thanks for this 👍

To test this out, is there a way to reproduce this issue against Electron master which still uses Chrome 53?

@bsclifton

This comment has been minimized.

Show comment
Hide comment
@bsclifton

bsclifton Jan 13, 2017

Member

@kevinsawicki I know we chatted over Slack, but wanted to capture here for record:

Should be reproducible on

  • Windows 10 with Anniversary Edition (or newer)
  • Frameless window (frame:false for BrowserWindow)
  • 200% DPI (if in a VM, make sure to crank up that video memory!)
Member

bsclifton commented Jan 13, 2017

@kevinsawicki I know we chatted over Slack, but wanted to capture here for record:

Should be reproducible on

  • Windows 10 with Anniversary Edition (or newer)
  • Frameless window (frame:false for BrowserWindow)
  • 200% DPI (if in a VM, make sure to crank up that video memory!)

@bsclifton bsclifton self-assigned this Jan 13, 2017

@bsclifton bsclifton requested review from zcbenz and kevinsawicki Jan 13, 2017

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Jan 13, 2017

Contributor

More conversation with @bsclifton from Slack posted here:

I tested this out on my Windows VM at 200% scale and am seeing rounded top corners on this branch vs. master. This is an empty page window.

master this branch
screen shot 2017-01-13 at 1 55 27 pm screen shot 2017-01-13 at 1 54 43 pm

Also, I'm also seeing the hover flicker when scrolling a <webview> on master and this branch:

flicker

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <title></title>
    <style>
      webview {
        position: absolute;
        top: 0;
        left: 0;
        right: 0;
        bottom: 0;
      }
    </style>
  </head>
  <body>
    <webview src="https://github.com/electron/electron/issues"></webview>
  </body>
</html>
Contributor

kevinsawicki commented Jan 13, 2017

More conversation with @bsclifton from Slack posted here:

I tested this out on my Windows VM at 200% scale and am seeing rounded top corners on this branch vs. master. This is an empty page window.

master this branch
screen shot 2017-01-13 at 1 55 27 pm screen shot 2017-01-13 at 1 54 43 pm

Also, I'm also seeing the hover flicker when scrolling a <webview> on master and this branch:

flicker

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <title></title>
    <style>
      webview {
        position: absolute;
        top: 0;
        left: 0;
        right: 0;
        bottom: 0;
      }
    </style>
  </head>
  <body>
    <webview src="https://github.com/electron/electron/issues"></webview>
  </body>
</html>
@bsclifton

This comment has been minimized.

Show comment
Hide comment
@bsclifton

bsclifton Jan 14, 2017

Member

@kevinsawicki I meant to ask: do you see the flicker issue w/ Chromium 54?

Member

bsclifton commented Jan 14, 2017

@kevinsawicki I meant to ask: do you see the flicker issue w/ Chromium 54?

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Jan 14, 2017

Member

@kevinsawicki The hover flicker is a known issue on webview elements with high DPI values. The issue for it is #7655 and the known workaround can be seen here.

MarshallOfSound/Google-Play-Music-Desktop-Player-UNOFFICIAL-@d40d80c

Member

MarshallOfSound commented Jan 14, 2017

@kevinsawicki The hover flicker is a known issue on webview elements with high DPI values. The issue for it is #7655 and the known workaround can be seen here.

MarshallOfSound/Google-Play-Music-Desktop-Player-UNOFFICIAL-@d40d80c

@bsclifton

This comment has been minimized.

Show comment
Hide comment
@bsclifton

bsclifton Jan 14, 2017

Member

@MarshallOfSound ahh thanks! I was mixing up issues

We fixed this in Brave the same way. Besides flickering, hit testing was not working as expected (unless we use this flag).

brave/browser-laptop@e8bf42e

Member

bsclifton commented Jan 14, 2017

@MarshallOfSound ahh thanks! I was mixing up issues

We fixed this in Brave the same way. Besides flickering, hit testing was not working as expected (unless we use this flag).

brave/browser-laptop@e8bf42e

@bsclifton

This comment has been minimized.

Show comment
Hide comment
@bsclifton

bsclifton Jan 14, 2017

Member

@kevinsawicki OK I fixed this the alternate way I had found and verified there are no rounded edges 😄 Give it a shot now

Member

bsclifton commented Jan 14, 2017

@kevinsawicki OK I fixed this the alternate way I had found and verified there are no rounded edges 😄 Give it a shot now

bsclifton added a commit to brave/muon that referenced this pull request Jan 15, 2017

Fix for rounded border issue on 200% DPI (Windows).
Was noticed when upstreaming border fixes to electron with electron/electron#8404
After @kevinsawicki saw same issue, I fixed with electron/electron@9e0547b (same as this fix)

Tested on Windows 10 anniversary edition at 200% DPI and confirmed issue is fixed with this

Fixes brave/browser-laptop#4216

Auditors: @bbondy, @darkdh, @bridiver
@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Jan 16, 2017

Contributor

OK I fixed this the alternate way I had found and verified there are no rounded edges 😄 Give it a shot now

Awesome, corners looks good on Windows 10, I kicked off a full CI build for this branch so we'll see if any tests need updating.

Contributor

kevinsawicki commented Jan 16, 2017

OK I fixed this the alternate way I had found and verified there are no rounded edges 😄 Give it a shot now

Awesome, corners looks good on Windows 10, I kicked off a full CI build for this branch so we'll see if any tests need updating.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Jan 16, 2017

Contributor

Specs looks good, http://208.52.191.140:8080/job/electron-win-x64/2200

Thanks for this @bsclifton 🚢 :shipit: 👍

Contributor

kevinsawicki commented Jan 16, 2017

Specs looks good, http://208.52.191.140:8080/job/electron-win-x64/2200

Thanks for this @bsclifton 🚢 :shipit: 👍

@kevinsawicki kevinsawicki merged commit 170f2f6 into electron:master Jan 16, 2017

4 of 6 checks passed

electron-win-x64 Build #2198 failed in 11 min
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #5192903 succeeded in 71s
Details
electron-linux-ia32 Build #5192904 succeeded in 66s
Details
electron-linux-x64 Build #5192905 succeeded in 132s
Details
@bsclifton

This comment has been minimized.

Show comment
Hide comment
@bsclifton

bsclifton Jan 16, 2017

Member

@kevinsawicki my pleasure 😄

Member

bsclifton commented Jan 16, 2017

@kevinsawicki my pleasure 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment