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 for Window hidden behind taskbar after maximize #7672 #7765

Merged
merged 8 commits into from Jan 9, 2017

Conversation

Projects
None yet
6 participants
@liusy182
Contributor

liusy182 commented Oct 27, 2016

Fix for issue #7672
This bug is because boolean thick_frame_ is not updated in line with windows style WS_THICKFRAME. NativeWindowViews::Maxmize() uses a different routine when it finds thick_frame_ is not set.

@kevinsawicki kevinsawicki self-assigned this Oct 27, 2016

Show outdated Hide outdated atom/browser/native_window_views.cc
FlipWindowStyle(GetAcceleratedWidget(), resizable, WS_THICKFRAME);
thick_frame_ = !thick_frame_;

This comment has been minimized.

@levrik

levrik Oct 29, 2016

Contributor

If this flag only gets reversed if it's true would it possibly better to set it explicitly to false?

@levrik

levrik Oct 29, 2016

Contributor

If this flag only gets reversed if it's true would it possibly better to set it explicitly to false?

@zeke

This comment has been minimized.

Show comment
Hide comment
@zeke

zeke Oct 30, 2016

Member

Kicked off another Appveyor build, in case it was a flaky failure.

Member

zeke commented Oct 30, 2016

Kicked off another Appveyor build, in case it was a flaky failure.

Show outdated Hide outdated atom/browser/native_window_views.cc
FlipWindowStyle(GetAcceleratedWidget(), resizable, WS_THICKFRAME);
thick_frame_ = false;

This comment has been minimized.

@kevinsawicki

kevinsawicki Oct 31, 2016

Contributor

I'm not sure this is the right approach here, calling setResizable twice, first with false and then with true will not re-enable the thick frame style so window animations won't work as expected.

Another approach could be to check the window style instead of thick_frame_ other places so that custom logic for maximize/unmaximize checks something like has_thick_frame() instead of thick_frame_ directly.

@kevinsawicki

kevinsawicki Oct 31, 2016

Contributor

I'm not sure this is the right approach here, calling setResizable twice, first with false and then with true will not re-enable the thick frame style so window animations won't work as expected.

Another approach could be to check the window style instead of thick_frame_ other places so that custom logic for maximize/unmaximize checks something like has_thick_frame() instead of thick_frame_ directly.

@kevinsawicki

Window style should be checked instead of initial thick frame setting.

@liusy182

This comment has been minimized.

Show comment
Hide comment
@liusy182

liusy182 Nov 3, 2016

Contributor

@kevinsawicki I was experimenting and noticed that we cannot replace all existing thick_frame_ check with style check on WS_THICKFRAME. The reason is that when an app is in full screen mode, WS_THICKFRAME is removed automatically. It is reset back when the app exits from full screen. Relying on WS_THICKFRAME causes issue when toggling full screen.
I think the solution is to still rely on thick_frame_, but this value will be updated with SetResizable call.

Contributor

liusy182 commented Nov 3, 2016

@kevinsawicki I was experimenting and noticed that we cannot replace all existing thick_frame_ check with style check on WS_THICKFRAME. The reason is that when an app is in full screen mode, WS_THICKFRAME is removed automatically. It is reset back when the app exits from full screen. Relying on WS_THICKFRAME causes issue when toggling full screen.
I think the solution is to still rely on thick_frame_, but this value will be updated with SetResizable call.

Show outdated Hide outdated atom/browser/native_window_views.cc
@@ -571,8 +571,8 @@ void NativeWindowViews::SetContentSizeConstraints(
void NativeWindowViews::SetResizable(bool resizable) {
#if defined(OS_WIN)
if (thick_frame_)
FlipWindowStyle(GetAcceleratedWidget(), resizable, WS_THICKFRAME);
FlipWindowStyle(GetAcceleratedWidget(), resizable, WS_THICKFRAME);

This comment has been minimized.

@kevinsawicki

kevinsawicki Nov 3, 2016

Contributor

If you create a window with thickFrame option set to false, won't this enable it the first time you call setResizable with true?

@kevinsawicki

kevinsawicki Nov 3, 2016

Contributor

If you create a window with thickFrame option set to false, won't this enable it the first time you call setResizable with true?

liusi
@liusy182

This comment has been minimized.

Show comment
Hide comment
@liusy182

liusy182 Nov 8, 2016

Contributor

Thanks @kevinsawicki I realized that I did not consider the case of frameless window at all. Corrected now.

Contributor

liusy182 commented Nov 8, 2016

Thanks @kevinsawicki I realized that I did not consider the case of frameless window at all. Corrected now.

Show outdated Hide outdated atom/browser/native_window_views.cc
@@ -571,8 +571,9 @@ void NativeWindowViews::SetContentSizeConstraints(
void NativeWindowViews::SetResizable(bool resizable) {
#if defined(OS_WIN)
if (thick_frame_)
if(has_frame()) {

This comment has been minimized.

@zeke

zeke Nov 9, 2016

Member

This if needs a space after it.

atom/browser/native_window_views.cc:574: Missing space before ( in if( [whitespace/parens] [5]

@zeke

zeke Nov 9, 2016

Member

This if needs a space after it.

atom/browser/native_window_views.cc:574: Missing space before ( in if( [whitespace/parens] [5]

This comment has been minimized.

@zeke

zeke Nov 9, 2016

Member

(That is what is causing the current CI failure)

@zeke

zeke Nov 9, 2016

Member

(That is what is causing the current CI failure)

liusi added some commits Nov 11, 2016

liusi
liusi
@liusy182

This comment has been minimized.

Show comment
Hide comment
@liusy182

liusy182 Dec 6, 2016

Contributor

@kevinsawicki I have made requested changes. Please let me know if anything else is needed :)

Contributor

liusy182 commented Dec 6, 2016

@kevinsawicki I have made requested changes. Please let me know if anything else is needed :)

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Dec 6, 2016

Contributor

I have made requested changes. Please let me know if anything else is needed :)

Sorry about the delay, just need to test this out and then I'll merge it 👍

Contributor

kevinsawicki commented Dec 6, 2016

I have made requested changes. Please let me know if anything else is needed :)

Sorry about the delay, just need to test this out and then I'll merge it 👍

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Dec 13, 2016

Contributor

@liusy182 sorry for the delay, I tried out the this branch and am still seeing an issue, can you confirm?

With the following app, I'm seeing the window expand over the taskbar when maximized.

const {app, BrowserWindow} = require('electron')

let window

app.once('ready', function () {
  window = new BrowserWindow({
    show: true,
    frame: false
  });
  browserWindow.loadURL('about:blank')
})

Then open the dev tools in the window and run the following:

w = require('electron').remote.getCurrentWindow()
w.setResizable(false);
w.setMinimumSize(500, 500);
w.maximize();

This maximizes the window over the taskbar for me on Windows 10.

Contributor

kevinsawicki commented Dec 13, 2016

@liusy182 sorry for the delay, I tried out the this branch and am still seeing an issue, can you confirm?

With the following app, I'm seeing the window expand over the taskbar when maximized.

const {app, BrowserWindow} = require('electron')

let window

app.once('ready', function () {
  window = new BrowserWindow({
    show: true,
    frame: false
  });
  browserWindow.loadURL('about:blank')
})

Then open the dev tools in the window and run the following:

w = require('electron').remote.getCurrentWindow()
w.setResizable(false);
w.setMinimumSize(500, 500);
w.maximize();

This maximizes the window over the taskbar for me on Windows 10.

@liusy182

This comment has been minimized.

Show comment
Hide comment
@liusy182

liusy182 Dec 15, 2016

Contributor

thanks @kevinsawicki You are right. I checked for all the places that still refer to the old thick_frame_ and made some changes:

  • For Maximize() and Unmaximize(), we can directly check for existence of WS_THICKFRAME.

  • For SetFullScreen(bool), I found that we do not need to make special treatment for no thick_frame_ case. I have tested around with that part of the code removed and it seemed to work fine for me.

Contributor

liusy182 commented Dec 15, 2016

thanks @kevinsawicki You are right. I checked for all the places that still refer to the old thick_frame_ and made some changes:

  • For Maximize() and Unmaximize(), we can directly check for existence of WS_THICKFRAME.

  • For SetFullScreen(bool), I found that we do not need to make special treatment for no thick_frame_ case. I have tested around with that part of the code removed and it seemed to work fine for me.

@kutu

This comment has been minimized.

Show comment
Hide comment
@kutu

kutu Dec 15, 2016

is this issue related to what i have noticed?

simple app, as described @kevinsawicki

  1. win+up, to maximize window
  2. i see that white glow behind taskbar
    1
  3. then i press task bar icon to minimize the app, and then press again to bring it back, and i see that behind taskbar
    2

after that, if i unmaximize window, and maximize it again, white glow is back

Windows7 64bit, electron prebuilt 1.4.12

kutu commented Dec 15, 2016

is this issue related to what i have noticed?

simple app, as described @kevinsawicki

  1. win+up, to maximize window
  2. i see that white glow behind taskbar
    1
  3. then i press task bar icon to minimize the app, and then press again to bring it back, and i see that behind taskbar
    2

after that, if i unmaximize window, and maximize it again, white glow is back

Windows7 64bit, electron prebuilt 1.4.12

@@ -486,19 +486,6 @@ void NativeWindowViews::SetFullScreen(bool fullscreen) {
NotifyWindowLeaveFullScreen();
}
// For window without WS_THICKFRAME style, we can not call SetFullscreen().
if (!thick_frame_) {

This comment has been minimized.

@kevinsawicki

kevinsawicki Jan 4, 2017

Contributor

I think this might still be needed (or some other fix).

On this branch, with the following app, the window becomes unusable once full screen state is left:

const {app, BrowserWindow} = require('electron')

let browserWindow

app.once('ready', function () {
  browserWindow = new BrowserWindow({
    show: true,
    fullscreen: true,
    transparent: true,
    thickFrame: false
  });
  browserWindow.loadURL(`about:blank`)
})

Then open the dev tools and run:

require('electron').remote.getCurrentWindow().setFullScreen(false)

The window does resize but if you click on it, it isn't usable and will disappear from the taskbar.

@kevinsawicki

kevinsawicki Jan 4, 2017

Contributor

I think this might still be needed (or some other fix).

On this branch, with the following app, the window becomes unusable once full screen state is left:

const {app, BrowserWindow} = require('electron')

let browserWindow

app.once('ready', function () {
  browserWindow = new BrowserWindow({
    show: true,
    fullscreen: true,
    transparent: true,
    thickFrame: false
  });
  browserWindow.loadURL(`about:blank`)
})

Then open the dev tools and run:

require('electron').remote.getCurrentWindow().setFullScreen(false)

The window does resize but if you click on it, it isn't usable and will disappear from the taskbar.

This comment has been minimized.

@kevinsawicki

kevinsawicki Jan 5, 2017

Contributor

Looks like this might be #6036 happening now since this block is removed.

@kevinsawicki

kevinsawicki Jan 5, 2017

Contributor

Looks like this might be #6036 happening now since this block is removed.

This comment has been minimized.

@radencode

radencode Feb 24, 2017

Where do I find atom/browser/native_window_views.cc ?

@radencode

radencode Feb 24, 2017

Where do I find atom/browser/native_window_views.cc ?

@kevinsawicki kevinsawicki merged commit e5aad98 into electron:master Jan 9, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Jan 9, 2017

Contributor

Thanks for this @liusy182, I really appreciate you taking the time to look into this so thoroughly 👍

Contributor

kevinsawicki commented Jan 9, 2017

Thanks for this @liusy182, I really appreciate you taking the time to look into this so thoroughly 👍

@radencode

This comment has been minimized.

Show comment
Hide comment
@radencode

radencode Feb 24, 2017

Where do I fine atom/browser/native_window_views.cc ?

radencode commented Feb 24, 2017

Where do I fine atom/browser/native_window_views.cc ?

@radencode

This comment has been minimized.

Show comment
Hide comment
@radencode

radencode Feb 26, 2017

@kevinsawicki @liusy182 Has the issue with the frameless window going under the task bar been fixed? I am running into the same issue right now and if you can help I would appreciate that.

radencode commented Feb 26, 2017

@kevinsawicki @liusy182 Has the issue with the frameless window going under the task bar been fixed? I am running into the same issue right now and if you can help I would appreciate that.

@liusy182 liusy182 deleted the liusy182:maximize-fix branch Aug 18, 2017

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