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

Fix frameless window overflow on Windows #9167

Merged
merged 4 commits into from Apr 13, 2017

Conversation

Projects
None yet
4 participants
@kevinsawicki
Contributor

kevinsawicki commented Apr 11, 2017

This pull request is an attempt to fix #5267 and #8728.

It removes the custom inset handling in Electron and instead uses Chrome's default inset handling in hwnd_message_handler.cc.

This previously did not work because Electron was using different a different linker flag than Chrome that resulted in the system metrics returning incorrect values.

I'm not sure if lowering the SUBSYSTEM flag to 5.02 has any other consequences, but it does seem to prevent the overflow of maximized frameless windows.

Fixes #5267
Fixes #8728
Refs #9140
Refs #8808

/cc @poiru @bsclifton @thesbros @juturu

@kevinsawicki kevinsawicki requested review from bsclifton, poiru and juturu Apr 11, 2017

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Apr 11, 2017

Contributor

I looked into this initially because I was seeing that Chrome app maximized frameless windows did not exhibit the overflow behavior and traced it down to GetSystemMetrics(SM_CXSIZEFRAME) returning 5 in Electron and 13 in Chrome which led me to the SUBSYSTEM linker flag being different between the two apps.

Contributor

kevinsawicki commented Apr 11, 2017

I looked into this initially because I was seeing that Chrome app maximized frameless windows did not exhibit the overflow behavior and traced it down to GetSystemMetrics(SM_CXSIZEFRAME) returning 5 in Electron and 13 in Chrome which led me to the SUBSYSTEM linker flag being different between the two apps.

@poiru poiru referenced this pull request Apr 11, 2017

Closed

Fix frameless window overflow on Windows #9140

8 of 24 tasks complete
@poiru

poiru approved these changes Apr 11, 2017

Great find, thank you! 👏

@odensc

This comment has been minimized.

Show comment
Hide comment
@odensc

odensc Apr 11, 2017

Member

Still getting some overflow, 150% DPI on Windows 10:

Unmaximized (height: 5px on top):

Maximized (only 1 pixel is shown):

Member

odensc commented Apr 11, 2017

Still getting some overflow, 150% DPI on Windows 10:

Unmaximized (height: 5px on top):

Maximized (only 1 pixel is shown):

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Apr 11, 2017

Contributor

Still getting some overflow, 150% DPI on Windows 10:

@thesbros Just to confirm, did you run npm run bootstrap -- --dev -v before building this branch? A re-bootstrap is needed since this pull request changes the linker flags.

Contributor

kevinsawicki commented Apr 11, 2017

Still getting some overflow, 150% DPI on Windows 10:

@thesbros Just to confirm, did you run npm run bootstrap -- --dev -v before building this branch? A re-bootstrap is needed since this pull request changes the linker flags.

@odensc

This comment has been minimized.

Show comment
Hide comment
@odensc

odensc Apr 11, 2017

Member

@kevinsawicki I ran python script/bootstrap.py -v - but I just ran that command and the same thing happens.

Member

odensc commented Apr 11, 2017

@kevinsawicki I ran python script/bootstrap.py -v - but I just ran that command and the same thing happens.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Apr 11, 2017

Contributor

I ran python script/bootstrap.py -v - but I just ran that command and the same thing happens.

Hmm, having trouble reproducing this on a Windows 10 VM, can you check all the edges with the following:

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <style>
      body, html {
        padding: 0;
        margin: 0;
      }

      #outer {
        border: 5px solid red;
        position: absolute;
        top: 0;
        left: 0;
        right: 0;
        bottom: 0;
      }

      #inner {
        border: 5px solid blue;
        position: absolute;
        top: 0;
        left: 0;
        right: 0;
        bottom: 0;
        padding: 20px;
      }
    </style>
  </head>
  <body>
    <div id="outer">
      <div id="inner">
        <button onclick="require('electron').remote.getCurrentWindow().maximize()">Maximize</button>
        <button onclick="require('electron').remote.getCurrentWindow().unmaximize()">Unmaximize</button>
        <button onclick="window.close()">Close</button>
      </div>
    </div>
  </body>
</html>
Contributor

kevinsawicki commented Apr 11, 2017

I ran python script/bootstrap.py -v - but I just ran that command and the same thing happens.

Hmm, having trouble reproducing this on a Windows 10 VM, can you check all the edges with the following:

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <style>
      body, html {
        padding: 0;
        margin: 0;
      }

      #outer {
        border: 5px solid red;
        position: absolute;
        top: 0;
        left: 0;
        right: 0;
        bottom: 0;
      }

      #inner {
        border: 5px solid blue;
        position: absolute;
        top: 0;
        left: 0;
        right: 0;
        bottom: 0;
        padding: 20px;
      }
    </style>
  </head>
  <body>
    <div id="outer">
      <div id="inner">
        <button onclick="require('electron').remote.getCurrentWindow().maximize()">Maximize</button>
        <button onclick="require('electron').remote.getCurrentWindow().unmaximize()">Unmaximize</button>
        <button onclick="window.close()">Close</button>
      </div>
    </div>
  </body>
</html>
@poiru

This comment has been minimized.

Show comment
Hide comment
@poiru

poiru Apr 11, 2017

Member

Note that this doesn't fully fix the overflow (but that is seemingly impossible). On Win10, a maximized window will overflow on the top/left/bottom/right as follows:

  • 100%: 1px T/L/B/R
  • 125%: 1px T/L, 2px B/R
  • 150%: 2px T/L, 2px B/R
  • 175%: 2px T/L, 3px B/R
  • 200%: 0.5px T/L/B/R

So @thesbros, I would expect you too see 2px missing from the top, but 4px seems high.

Member

poiru commented Apr 11, 2017

Note that this doesn't fully fix the overflow (but that is seemingly impossible). On Win10, a maximized window will overflow on the top/left/bottom/right as follows:

  • 100%: 1px T/L/B/R
  • 125%: 1px T/L, 2px B/R
  • 150%: 2px T/L, 2px B/R
  • 175%: 2px T/L, 3px B/R
  • 200%: 0.5px T/L/B/R

So @thesbros, I would expect you too see 2px missing from the top, but 4px seems high.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Apr 11, 2017

Contributor

150%: 2px T/L, 2px B/R

Interesting, I'm only seeing it overflow 1px on each side at this scale.

Contributor

kevinsawicki commented Apr 11, 2017

150%: 2px T/L, 2px B/R

Interesting, I'm only seeing it overflow 1px on each side at this scale.

@odensc

This comment has been minimized.

Show comment
Hide comment
@odensc

odensc Apr 11, 2017

Member

@kevinsawicki Not sure what's up - just deleted Electron and cloned a fresh repo, now it works.

I'm also getting a 2px overflow on all sides, as @poiru said. Definitely better than before.

Member

odensc commented Apr 11, 2017

@kevinsawicki Not sure what's up - just deleted Electron and cloned a fresh repo, now it works.

I'm also getting a 2px overflow on all sides, as @poiru said. Definitely better than before.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Apr 11, 2017

Contributor

I'm also getting a 2px overflow on all sides, as @poiru said. Definitely better than before.

So with the following tweaked page, you don't see a red border at all when maximized?

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <style>
      body, html {
        padding: 0;
        margin: 0;
      }

      #outer {
        border: 2px solid red;
        position: absolute;
        top: 0;
        left: 0;
        right: 0;
        bottom: 0;
      }

      #inner {
        border: 5px solid blue;
        position: absolute;
        top: 0;
        left: 0;
        right: 0;
        bottom: 0;
        padding: 20px;
      }
    </style>
  </head>
  <body>
    <div id="outer">
      <div id="inner">
        <button onclick="require('electron').remote.getCurrentWindow().maximize()">Maximize</button>
        <button onclick="require('electron').remote.getCurrentWindow().unmaximize()">Unmaximize</button>
        <button onclick="window.close()">Close</button>
      </div>
    </div>
  </body>
</html>
Contributor

kevinsawicki commented Apr 11, 2017

I'm also getting a 2px overflow on all sides, as @poiru said. Definitely better than before.

So with the following tweaked page, you don't see a red border at all when maximized?

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <style>
      body, html {
        padding: 0;
        margin: 0;
      }

      #outer {
        border: 2px solid red;
        position: absolute;
        top: 0;
        left: 0;
        right: 0;
        bottom: 0;
      }

      #inner {
        border: 5px solid blue;
        position: absolute;
        top: 0;
        left: 0;
        right: 0;
        bottom: 0;
        padding: 20px;
      }
    </style>
  </head>
  <body>
    <div id="outer">
      <div id="inner">
        <button onclick="require('electron').remote.getCurrentWindow().maximize()">Maximize</button>
        <button onclick="require('electron').remote.getCurrentWindow().unmaximize()">Unmaximize</button>
        <button onclick="window.close()">Close</button>
      </div>
    </div>
  </body>
</html>
@poiru

This comment has been minimized.

Show comment
Hide comment
@poiru

poiru Apr 11, 2017

Member

@kevinsawicki This is what I see:
corners

Perhaps it's not exactly 2px, but definitely in that ballpark. Also note the differently sized red parts.

Member

poiru commented Apr 11, 2017

@kevinsawicki This is what I see:
corners

Perhaps it's not exactly 2px, but definitely in that ballpark. Also note the differently sized red parts.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Apr 11, 2017

Contributor

Perhaps it's not exactly 2px, but definitely in that ballpark. Also note the differently sized red parts.

Yeah, I'm seeing the different sized parts as well at certain DPIs, so frustrating.

Contributor

kevinsawicki commented Apr 11, 2017

Perhaps it's not exactly 2px, but definitely in that ballpark. Also note the differently sized red parts.

Yeah, I'm seeing the different sized parts as well at certain DPIs, so frustrating.

@odensc

This comment has been minimized.

Show comment
Hide comment
@odensc

odensc Apr 11, 2017

Member

@kevinsawicki I'm talking in terms of 2px display pixel overflow. (e.g. when opening the image in GIMP and looking how many pixels there are)

With 150% DPI, 1 CSS pixel == 1.5 display pixels. I'm still seeing a 2px overflow with that HTML - but there's still a pixel of red left on each side because it's actually 3 pixels.

Member

odensc commented Apr 11, 2017

@kevinsawicki I'm talking in terms of 2px display pixel overflow. (e.g. when opening the image in GIMP and looking how many pixels there are)

With 150% DPI, 1 CSS pixel == 1.5 display pixels. I'm still seeing a 2px overflow with that HTML - but there's still a pixel of red left on each side because it's actually 3 pixels.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Apr 11, 2017

Contributor

I'm talking in terms of 2px display pixel overflow.

Ah okay, 👍 💻

Contributor

kevinsawicki commented Apr 11, 2017

I'm talking in terms of 2px display pixel overflow.

Ah okay, 👍 💻

@juturu

juturu approved these changes Apr 13, 2017

This change looks much better. Haven't seen any issues with this change yet.

@odensc

odensc approved these changes Apr 13, 2017

🎉

@poiru poiru merged commit 0c1d603 into master Apr 13, 2017

0 of 4 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
electron-mas-x64 Build #3904 in progress...
Details
electron-win-x64 Build #2867 in progress...
Details

@poiru poiru deleted the frameless-overflow3 branch Apr 13, 2017

@poiru

This comment has been minimized.

Show comment
Hide comment
@poiru

poiru Apr 13, 2017

Member

@kevinsawicki I rebased this on top of #9166 and also went ahead and merged it. Hopefully that was OK as this has been approved by everyone and I need this for another PR.

Member

poiru commented Apr 13, 2017

@kevinsawicki I rebased this on top of #9166 and also went ahead and merged it. Hopefully that was OK as this has been approved by everyone and I need this for another PR.

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