microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
162.77k stars 28.73k forks source link

Web: `detectFullscreen` false positive on a display without dock and title area #229058

Open a-stewart opened 5 days ago

a-stewart commented 5 days ago

If you run insiders.vscode.dev on a PWA in Linux with multiple monitors and WCO enabled, the window controls are on top of the icons on the right.

image

The issue is that there is a bad premise in dom.ts.

    if (targetWindow.innerHeight === targetWindow.screen.height) {
        // if the height of the window matches the screen height, we can
        // safely assume that the browser is fullscreen because no browser
        // chrome is taking height away (e.g. like toolbars).
        return { mode: DetectedFullscreenMode.BROWSER, guess: false };
    }

The problem with this is that if you are on a Linux machine with a secondary screen (so no task bar), with the App running as a PWA and with window controls overlay enabled, then the regular - non full screen but maximised window will be the same height as the screen height.

As such it will falsely report being fullscreen and the window controls will render on top of the icons, since the css includes:

.monaco-workbench.fullscreen .part.titlebar .window-controls-container {
    display: none;
    background-color: transparent;
}

There is a secondary issue that window.screen seems to be faulty on Chrome/Linux too which means sometimes it gets the wrong screen, and that results in the PWA actually rendering correctly which makes this tricky to debug.

a-stewart commented 5 days ago

This also causes a second issue when you have `Window: Menu Bar Visibility" which causes the menu bar to be hidden when you maximise VS Code.

bpasero commented 3 days ago

I cannot reproduce the title button issue.

WCO enabled ... and with window controls overlay enabled

This sounds as if I have to configure something, then how do I do that and what?

This also causes a second issue when you have `Window: Menu Bar Visibility" which causes the menu bar to be hidden when you maximise VS Code.

Yes, this I can confirm: on a window where there is no dock and no title area, where VS Code spans the entire space of the monitor, we seem to think VS Code is in fullscreen mode. So we need another strategy how to figure that out.

a-stewart commented 1 day ago

WCO enabled ... and with window controls overlay enabled

This sounds as if I have to configure something, then how do I do that and what?

When you are in a PWA, click on the "up" arrow in the title bar.

image

That should then make it look like

image


There is a secondary bug at play though. Chrome does not pick update the window.screen object when the screen changes on Linux X11 as far as I can see - and when a PWA is opened it sometimes just picks the first screen for some reason which can throw everything off, and oddly that actually fixes things because it thinks it's not a fullscreen app. I have reported that bug with Chrome.

https://g-issues.chromium.org/issues/368281197

Hopefully when that bug is resolved fullscreen issues should be a bit more reliable. But there is not really anything we can do on that side so it's not really relevant to this bug, it just makes things harder to test and repro.


The issue I wanted to raise is this comment in dom.ts:

        // if the height of the window matches the screen height, we can
        // safely assume that the browser is fullscreen because no browser
        // chrome is taking height away (e.g. like toolbars).

In particular we can safely assume. This comment was written in 2020 I think (perhaps earlier). It seems like the Window Controls Overlays were only supported from 2022 onwards based on https://developer.mozilla.org/en-US/docs/Web/API/Window_Controls_Overlay_API#browser_compatibility.

So the reason for this bug is that this method for detecting full screen is no longer valid.

On a Mac and Windows, I believe you have the OS status bar on all screens, so I think this is likely still valid, but I think perhaps we should exclude Linux if PWAs are enabled.

Eg, add this before that command..

    // If Window Controls Overlay is enabled, then on Linux it is likely that a
    // full height window is just a maximised window, rather than full screen.
    // In the event of getting it wrong, it is safer to assume not fullscreen.
    if (platform.isLinux && (navigator as any)?.windowControlsOverlay?.visible) {
        return null;
    }

Or mark it as a guess in that scenario.

(navigator as any)?.windowControlsOverlay?.visible alone should theoretically be perfect. If the Window Controls Overlay is visible, then we know for certain that we are not in a fullscreen app. Except it turns out that this still says visible === true when the app is fullscreen and the controls are not visible. I have raised https://g-issues.chromium.org/issues/368299095 with Chrome for this issue.


There is also a bit slightly further on which says window.innerHeight does not work on Mac or Linux, and instead does another guess with window.outerHeight which will be even more unreliable as that would get all PWAs on a seconard monitor, regardless of whether WCO is turned on.

This was mentioned in the original PR: https://github.com/microsoft/vscode/pull/106479 but when I test now on Chrome on both Linux and Mac OS, window.innerHeight seems to work as expected. It is possible this issue with innerHeight has been fixed in the last 4 years, so perhaps we could look into removing that secondary clause?

Was there a bug at the time with Chrome for that issue?


All that said, looking at the original PR, it seems like this was originally mean for keybindings only, so false positives were acceptable. But it seems like we perhaps use the same signal for UI elements now where false positives do cause issues. We do specify whether the fullscreen detection was a guess or not, but we seem to completely ignore that bit? Perhaps a "correct" fix would be to propagate that bit and then when we make decisions on whether we want fullscreen or not - then we specify if we accept guesses or not?