1482920 - [Redstone 5] Video does not enter correctly in full screen if the FF window is snapped

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[Ciprian Georgiu, Desktop QA]

Attachment: screenshot showing the issue.PNG

[Affected versions]:
- latest Nightly 63.0a1
- Beta 62.0b16
- Release 61.0.2
- Esr 60.1.1

[Affected platforms]:
- Windows 10 x64, Version 1803 (OS Build 17735.1000) 

[Steps to reproduce]:
1. Launch Firefox.
2. Access the following website and play any video: https://www.youtube.com/ 
3. Drag the Firefox window via mouse, and drop it to the right (or left) side of the screen.
4. Enter Full screen, using the dedicated video button or double click on video.

[Expected result]:
- The video properly enters in full screen, there is no border displayed on the left side and the taskbar is hidden.  

[Actual result]:
- The video does not enter correctly in full screen, the taskbar overlaps the video and a border is displayed on the left side.
 
[Regression range]:
- It doesn't seem to be a recent regression, I was able to reproduce it on 61.0.2 as well. I will investigate further in case of an old regression and provide more info.

[Additional notes]:
- see the attached screenshot
- not repro on the current Windows 10 release
- additionally you can press Windows Key + Left/Right arrow to snap a window

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

This may be an issue with Redstone 5. Can you file an issue with the Feedback Hub to let Microsoft know of the issue?

Comment 2

[Ciprian Georgiu, Desktop QA]

Filed the issue on Feedback Hub as well.

Please note that the issue is not reproducible on Chrome nor Edge.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Adam, is this on your radar?

Comment 4

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

This is the first I've heard of it, I'm getting an RS5 build set up now to investigate.

Comment 5

[:Gijs (he/him)]

This smells like bug 1403153. Does the issue go away if you reboot the machine?

Comment 6

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

It may be related but it's a bigger issue: The taskbar overlaps fullscreen.

I haven't been able to reproduce this yet on 17713, I'm trying to get a newer version.

Comment 7

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Confirmed on 17741, investigating.

Comment 8

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

This seems to be due to a change introduced somewhere between Windows 10 builds 17713 and 17735. It can be reproduced by just snapping the window and hitting F11 for fullscreen.

When we disable window decorations (by clearing WS_CAPTION | WS_THICKFRAME via SetWindowLongPtrW, see HideWindowChrome), we normally get a WS_WINDOWPOSCHANGED with SWP_FRAMECHANGED that just repeats the window position. The window appears to get larger due to the window manager adding an invisible 7 pixels to the left, right, and bottom as long as WC_CAPTION | WS_THICKFRAME are set.

But if the window is snapped then there is another WM_WINDOWPOSCHANGED which gives the position of the window as if it were shrunk by 7 pixels on the right, left, and bottom; this prevents the window from appearing to expand beyond the bounds of the snap region. Only one WM_MOVE and WM_SIZE are generated as a result of the two changes.

The bug seems to be that now, if the window is resized before the second, shrinking WM_WINDOWPOSCHANGED, another shrinking WM_WINDOWPOSCHANGED message will be delivered, but based on the new size that has been requested. So after the resize the window doesn't occupy the whole screen, which throws off the taskbar hiding heuristics.

However it isn't clear why the ITaskbarList2::MarkFullscreenWindow call isn't taking effect, that should be sufficient to draw us over the taskbar anyway (though it won't fix the bars on the sides). Oddly it does work if I only resize to half the full width of the screen...

We could fix this in a few ways, generally we'd have a delay between hiding chrome and resizing the window. Chrome seems to avoid this by always setting the window manager non-client rendering policy to "disabled", this avoids all the issues with disabling window decorations. I don't know if we can directly do that, I'll look into it.

Comment 9

[Ciprian Georgiu, Desktop QA]

(In reply to :Gijs (he/him) from comment #5)
> This smells like bug 1403153. Does the issue go away if you reboot the
> machine?

No, unfortunately the issue is still present.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Is it safe for us to assign this to you for now, :agashlin?

I'm unsure of how to prioritize this, since we don't know the release date of Redstone 5. Out of an abundance of caution, I'm going to P1 this (since Windows is Tier 1 and video sites like YouTube are pretty popular and important), but if that seems to aggressive, please feel free to ratchet it down.

Comment 11

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Taken, I'm pretty much treating it as a P1 anyway.

Comment 12

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Demo of the bug in a simpler program

Here's a video showing the variations of the odd behavior, so that I can reference this in a message to the Microsoft list.

Comment 13

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Source for the test program

Source to the demo

Comment 14

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

I'm moving this to P2 because:
- I don't think snapping is used very widely
- This affects Microsoft apps as well (at least IE and Console Window), so it has a good chance of being fixed

But I'll keep the ni? so it stays on my radar to check again as we near RS5 release, whenever that will be. I do have a workaround but I'd rather avoid any more complexity in fullscreen.

Comment 17

[pringo4]

I am experiencing this bug in the release version of Windows 10 1809 using Firefox 63.0.1.

Comment 18

[Karl Dubost💡 :karlcow]

Maybe https://webcompat.com/issues/20527 is an instance of this issue.

Comment 19

[pringo4]

I can confirm this bug still exists in today's re-release of Windows 10 1809.

Comment 21

[Mathias Humpeler]

I cnfirm this too, same on my machine on Windows 1809, second final Version :)

Comment 22

[Julien Cristau [:jcristau]]

Too late for 64.  Adam, have you had a chance to get back to this?

Comment 23

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

It turns out that Chrome *is* doing something explicit [1] which fixes this, thought it wasn't put there with this glitch in mind: When we're supposed to be full screen, override the position given in WM_WINDOWPOSCHANGING. This way the broken position update just gets ignored. This is kind of cheating, but, without Microsoft fixing the DWM, the other approach (see hack below) is no better.

Patch incoming.

[1] https://cs.chromium.org/chromium/src/ui/views/win/hwnd_message_handler.cc?l=2667&rcl=f1b12ddf4c132dd2e846b0ee2d08e23b83581bcb

---

Another approach:

Top level widget\windows\nsWindow.cpp
+DWORD WINAPI
+ResizeThread(LPVOID lpParam)
+{
+  LPWINDOWPOS pos = static_cast<LPWINDOWPOS>(lpParam);
+  return SetWindowPos( pos->hwnd, pos->hwndInsertAfter,
+      pos->x, pos->y, pos->cx, pos->cy,
+      pos->flags);
+}

In nsWindow::Resize(double aX, double aY, double aWidth, double aHeight, bool aRepaint)
-    VERIFY(::SetWindowPos(mWnd, nullptr, x, y,
-                          width, GetHeight(height), flags));
+    WINDOWPOS resizeParam = {mWnd, nullptr, x, y, width, GetHeight(height),
+                             flags | SWP_ASYNCWINDOWPOS};
+    HANDLE hResizeThread = CreateThread(nullptr, 0, ResizeThread, &resizeParam, 0, 0);
+    WaitForSingleObject(hResizeThread, INFINITE);

This has the effect of forcing SetWindowPos to post its message (because we're calling it with SWP_ASYNCWINDOWPOS on the new thread) rather than sending it on the same thread. I guess the delay in processing makes the resize happen later, after the window for the bug.

This is also less desirable as we wouldn't want to do this on *every* size change, so some extra machinery would be needed, and we'd need to clean up the thread and pass along the return code of SetWindowPos.

Comment 24

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Bug 1482920: Force fullscreen window position. r?jmathies

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bcc079816150
Force fullscreen window position. r=jmathies

Comment 26

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/bcc079816150

Comment 27

[Ryan VanderMeulen [:RyanVM]]

We should probably take this on ESR60 as well, but it can wait for the next cycle after it's had some bake time.

Comment 28

[Krzysztof]

I'm afraid that this bug has broken the fullscreen mode on win7. See Bug 1513066.

Comment 30

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Is this still wanted for esr60? I can roll up this and bug 1513066 and bug 1514501 into a single patch for easier landing (otherwise 1482920, 1513066, and 1514501 will have to be applied in that order to fix regressions from this).

Comment 31

[Ryan VanderMeulen [:RyanVM]]

I'd be in favor of it, but Julien should probably make the call.

Comment 32

[Julien Cristau [:jcristau]]

Yes, let's do that. Uplift request for a combined patch is probably easiest if you have cycles, otherwise just put in the request for this patch and I'll take is as applying to all three.

Comment 33

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Rollup for esr60

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: This fixes a new issue in a widely used feature (fullscreen).

User impact if declined: On recent versions of Windows 10 (October 2018 Update), fullscreen will not work correctly if the Firefox window had previously been snapped in place.

Fix Landed on Version: 65

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Some risk due to extra complications on Windows 7, but I think those have been addressed in the followups which have been rolled into this patch.

String or UUID changes made by this patch: none

Comment 34

[Julien Cristau [:jcristau]]

Comment on attachment 9036747 [details] [diff] [review]
Rollup for esr60

fullscreen fix for windows, approved for 60.5esr

Comment 35

[Ciprian Georgiu, Desktop QA]

This is verified fixed on 65.0b11 under Windows 10 Version 1809, x64.

Ni? myself as a reminder to verify this on esr as well once it's fixed.

Comment 36

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/5d96340ff731

Comment 37

[Anca Soncutean, Desktop QA]

I can also confirm the fix with 60.4.1esr (20190117142900) on Windows 10 x64 (version 1809).

Comment 38

[Julien Cristau [:jcristau]]

Thanks for the verification. Can you also maybe check on win7, since that's where we had regressions when this landed on trunk (1513066, 1514501)?

Comment 39

[Anca Soncutean, Desktop QA]

Sure, all good on Win 7 (32 bit), the issue is not reproducible with Fx 65.0b12 (20190117232427) and Fx 60.4.1esr (20190117142900).

Comment 40

[Tin]

Apologies if I'm not doing this correctly, first time I've tried to report something.

I don't think this has been fully rectified.

On a number of Firefox browsers running 66.02 I've noticed the following behaviour when snapping the browser vertically, (win+shift+up), and switching to full screen mode for videos, (eg Youtube). When you return from fullscreen the firefox window has grown slightly. It will now extend by a few pixels under the taskbar and the windows will have grown a little to right. On youtube, if you keep pressing F to toggle in and out of full screen it will continue to slowly grow, extending below the taskbar and eventually off the right side of the screen.

This only does this for firefox windows that have been snapped to the edges, (vertically or on the sides). And I only noticed it doing it after this bug was reported as being fixed.

I've tried disabling addons as well as starting with add ons disabled. It didn't seem to help. I've reproduced this in Firefox 66.02 on three computers running Windows 10 x64 1809.

Comment 41

[Fanolian]

(In reply to Tin from comment #40)

When you return from fullscreen the firefox window has grown slightly. It will now extend by a few pixels under the taskbar and the windows will have grown a little to right.

This is bug 1511215.