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 unresponsive app after fullscreen->normal state toggle (Windows) #9645

Merged
merged 5 commits into from Jun 14, 2017

Conversation

Projects
None yet
2 participants
@dharders

dharders commented Jun 1, 2017

Fixes #6036

Summary

In Windows only, starting an app in the fullscreen state i.e. with the BrowserWindow options { fullscreen: true } produces an unresponsive window after toggling to the normal window state. e.g. Pressing F11 or setFullScreen(false). See issue #6036

The window visibility flag WS_VISIBLE is never set for the GWL_STYLE when we start in fullscreen mode. This fix simply detects the fullscreen->normal window state transition and checks if the window is visible (which it always should be). If it isn't, it sets the visibility to true. i.e. :

void NativeWindowViews::SetFullScreen(bool fullscreen) {
   bool fullscreen_to_normal_detected = IsFullscreen() && !fullscreen;
   ...
   if (fullscreen_to_normal_detected && !IsVisible()) {
       LONG frame_style = ::GetWindowLong(GetAcceleratedWidget(), GWL_STYLE);
       frame_style |= WS_VISIBLE;
       ::SetWindowLong(GetAcceleratedWidget(), GWL_STYLE, frame_style);
   }
   ...
}

Includes tests (this branch passes, old behavior fails)

Fixes #6036

@dharders

This comment has been minimized.

dharders commented Jun 1, 2017

Hmmm, Travis is being funky. All tests passed, yet Job #11868.1 build script still exited with 1.

Care to take a look anyone ?

/ping @kevinsawicki

@dharders

This comment has been minimized.

dharders commented Jun 5, 2017

Never mind, re-ran tests and all green now!

@kevinsawicki kevinsawicki self-assigned this Jun 5, 2017

@kevinsawicki

Thanks for figuring this one out, awesome to see the fix for it 👍

Left a few minor comments, let me know what you think.

@@ -480,6 +480,8 @@ void NativeWindowViews::SetFullScreen(bool fullscreen) {
#if defined(OS_WIN)
// There is no native fullscreen state on Windows.
bool fullscreen_to_normal_detected = IsFullscreen() && !fullscreen;

This comment has been minimized.

@kevinsawicki

kevinsawicki Jun 13, 2017

Contributor

What do you think about calling this leaving_fullscreen instead?

if (fullscreen_to_normal_detected && !IsVisible()) {
LONG frame_style = ::GetWindowLong(GetAcceleratedWidget(), GWL_STYLE);
frame_style |= WS_VISIBLE;
::SetWindowLong(GetAcceleratedWidget(), GWL_STYLE, frame_style);

This comment has been minimized.

@kevinsawicki

kevinsawicki Jun 13, 2017

Contributor

This could just be:

FlipWindowStyle(GetAcceleratedWidget(), true, WS_VISIBLE);
assert.equal(w.isVisible(), false)
assert.equal(w.isFullScreen(), false)
done()
}, 1000)

This comment has been minimized.

@kevinsawicki

kevinsawicki Jun 13, 2017

Contributor

Instead of a setTimeout, could this just wait for the leave-full-screen event and verify the state after that fires:

w.once('leave-full-screen', () => {
  assert.equal(w.isVisible(), false)
  assert.equal(w.isFullScreen(), false)
  done()
})
w.setFullScreen(false)
@dharders

This comment has been minimized.

dharders commented Jun 14, 2017

Changes made, as requested.

ping @kevinsawicki

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Jun 14, 2017

Thanks for fixing this @dharders 👍 :shipit: 🎉

@kevinsawicki kevinsawicki merged commit 45dc6fc into electron:master Jun 14, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dharders dharders deleted the dharders:issue6036-fix-windows-fullscreen-startup-toggle branch Jun 15, 2017

@dharders

This comment has been minimized.

dharders commented Jun 29, 2017

FYI: Released in "electron": "1.7.4 beta"

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