fix: on macOS show BrowserWindow on maximize if not currently shown

[dsanders11]

Description of Change

Fixes #32947.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed behavior of BrowserWindow.maximize on macOS for not shown windows

[release-clerk]

Release Notes Persisted

Fixed behavior of BrowserWindow.maximize on macOS for not shown windows

[trop]

I was unable to backport this PR to "17-x-y" cleanly;
you will need to perform this backport manually.

[trop]

I was unable to backport this PR to "18-x-y" cleanly;
you will need to perform this backport manually.

[trop]

I was unable to backport this PR to "15-x-y" cleanly;
you will need to perform this backport manually.

[trop]

I was unable to backport this PR to "16-x-y" cleanly;
you will need to perform this backport manually.

[bpasero]

Fyi this is a somewhat breaking change and results in issues for VSCode. VSCode explicitly sets show: false when creating a browser window when the window is maximized or fullscreen to reduce flicker. We then call window.maximize() and only call window.show() if window.isVisible() returns false. With this change, window.maximize() will make window.isVisible() return true, even though the window is behind the key window and essentially not visible.

[bpasero]

As a workaround, the current thinking is to also check for window.isFocused() to mitigate.

//cc @deepak1556

[dsanders11]

@bpasero, sorry it caused some problems for VSCode.

With this change, window.maximize() will make window.isVisible() return true, even though the window is behind the key window and essentially not visible.

Despite the name, window.isVisible() only refers to if the window is shown, not that it is visibly unobscured by other windows. Setting an interval to output the value of window.isVisible() shows that it is always true even when the window is obscured behind other windows, and this is true across Windows, Linux, and macOS. The only thing that may not be working as intended here is the window when shown by window.maximize() is not coming to the top of the z-order. I'll discuss that on your comment on #32947.

I'm assuming this is the relevant VSCode code, post-workaround?

As a workaround, the current thinking is to also check for window.isFocused() to mitigate.

If the intention is for the window to be focused, then I'd argue that isn't a workaround, but a more correct implementation. The original check for window.isVisible() feels like it was a workaround to begin with, since it suggests that sometimes it's true, sometimes it's false, and there's no comment explaining that discrepancy. It was effectively a workaround for #32947, as other platforms did show an unshown window after maximizing it, so that code branch was only being taken on macOS.

I would also argue that the previous code in VSCode was already buggy. On platforms other than macOS, the behavior of window.maximize() on a unshown window was consistent with the documentation, meaning it would be shown without being focused. Since window.isVisible() is only checking if it is shown, then on non-macOS platforms you would never call window.show(), leading to a shown window without focus, and on macOS you were getting a shown window with focus.

I think a possibly better way to accomplish the end goal there would be to set a one-time event listener for the 'maximize' event and take action once that fires, and don't bother with the conditional checks. The tests in Electron for BrowserWindow wait on events a lot of the time to prevent races. Checking window.isVisible() immediately after calling window.maximize() is potentially racey, so the existing code had some race conditions as well.

Here's what I'm thinking:

if (isFullscreenOrMaximized) {
	mark('code/willMaximizeCodeWindow');

	// to reduce flicker from the default window size to maximize, we only focus after maximize
	this._win.once('maximize', () => {
		this._win.focus();
		mark('code/didMaximizeCodeWindow');
	});

	this._win.maximize();

	if (this.windowState.mode === WindowMode.Fullscreen) {
		this.setFullScreen(true);
	}
}

Might want to throw in a backstop to remove the event listener after a timeout in case the event doesn't fire for some reason, to prevent weird behavior if the window is later maximized. Unfortunately there are a lot of bugs around BrowserWindow in general and events across platforms, and I imagine new ones will appear in the future, so might pay to be defensive there.

[trop]

@dsanders11 has manually backported this PR to "19-x-y", please check out #34365

[dsanders11]

Just an FYI, @bpasero, noticed as a result of your comment that due to a process edge case this wasn't in 19-x-y, so I'm backporting it there so that everything is consistent. If we decide changes are needed then everything will be on the same starting point.

[bpasero]

@dsanders11 thanks for chiming in ❤️ . To clarify, the code we run with here is not new, we have been using it probably ever since we adopted Electron 7y ago. As I indicated before, it is an ugly workaround for not being able to set a window to be maximised or fullscreen when creating it to reduce flicker for the transition (more details in #34368).

The only change that I pushed now was to throw in an additional isFocused only on macOS: microsoft/vscode@6682d2f

Maybe we are just lucky that we did not hear issues on other platforms around window focus. The fact that I now got 5 issues in 3 weeks on macOS given this recent breakage makes me quite confident that on other platforms we are actually doing fine, so I am hesitant to make any further changes here without understanding the consequences.

I think your change is not wrong, but I think to make the experience consistent across platforms, maximize should show the window as active on macOS or provide an option to do so, so that we do not need the ugly event listening hack. When I test this on Linux for example, I see the window put to front and not behind the active window, so it seems inconsistent now.

Irrespective, maybe the best change to do here is to simply always call window.show() when we know that we had set show: false previously. Then in the end we are not checking on some conditions that may suffer from race conditions, what do you think? Are there any negative side effects of calling window.show on a window that is already visible and has keyboard focus?

[dsanders11]

I think your change is not wrong, but I think to make the experience consistent across platforms, maximize should show the window as active on macOS or provide an option to do so, so that we do not need the ugly event listening hack. When I test this on Linux for example, I see the window put to front and not behind the active window, so it seems inconsistent now.

There's a good chance that the documentation is inaccurate, and this PR was written to the documentation. 🙂 With some quick testing it looks like both Windows and Linux are focusing the window when it is shown via maximize(), despite what the documentation says. I'll run some more manual tests and update the test coverage to check focus state for the window. If it is indeed focusing on other platforms, and has been for a long time, then I figure we just make macOS match that behavior and update the documentation to match the actual behavior.

Irrespective, maybe the best change to do here is to simply always call window.show() when we know that we had set show: false previously. Then in the end we are not checking on some conditions that may suffer from race conditions, what do you think? Are there any negative side effects of calling window.show on a window that is already visible and has keyboard focus?

I can't think of a negative side effect off the top of my head.

[bpasero]

👍 I suggest to just ShowActive in macOS maximize implementation.