Closed Bug 1482920 Opened Last year Closed 9 months ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla65
Tracking Status
firefox-esr60 65+ verified
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 - wontfix
firefox65 + verified

People

(Reporter: ciprian_georgiu, Assigned: agashlin)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [Redstone 5])

Attachments

(5 files)

[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
Summary: Video does not enter correctly in full screen if the FF window is snapped → [Redstone 5] Video does not enter correctly in full screen if the FF window is snapped
This may be an issue with Redstone 5. Can you file an issue with the Feedback Hub to let Microsoft know of the issue?
Flags: needinfo?(ciprian.georgiu)
Filed the issue on Feedback Hub as well.

Please note that the issue is not reproducible on Chrome nor Edge.
Flags: needinfo?(ciprian.georgiu)
Adam, is this on your radar?
Flags: needinfo?(agashlin)
This is the first I've heard of it, I'm getting an RS5 build set up now to investigate.
This smells like bug 1403153. Does the issue go away if you reboot the machine?
Flags: needinfo?(ciprian.georgiu)
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.
Confirmed on 17741, investigating.
Flags: needinfo?(agashlin)
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.
Flags: needinfo?(agashlin)
(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.
Flags: needinfo?(ciprian.georgiu)
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.
Priority: -- → P1
Taken, I'm pretty much treating it as a P1 anyway.
Assignee: nobody → agashlin
Component: Video/Audio Controls → Widget: Win32
Product: Toolkit → Core
Version: Trunk → 61 Branch
Version: 61 Branch → Trunk
Here's a video showing the variations of the odd behavior, so that I can reference this in a message to the Microsoft list.
Source to the demo
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.
Priority: P1 → P2
Duplicate of this bug: 1497030
Duplicate of this bug: 1503781
I am experiencing this bug in the release version of Windows 10 1809 using Firefox 63.0.1.
Maybe https://webcompat.com/issues/20527 is an instance of this issue.
I can confirm this bug still exists in today's re-release of Windows 10 1809.
Duplicate of this bug: 1507584
I cnfirm this too, same on my machine on Windows 1809, second final Version :)
Too late for 64.  Adam, have you had a chance to get back to this?
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.
Flags: needinfo?(agashlin)
Pushed by agashlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bcc079816150
Force fullscreen window position. r=jmathies
https://hg.mozilla.org/mozilla-central/rev/bcc079816150
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
We should probably take this on ESR60 as well, but it can wait for the next cycle after it's had some bake time.
I'm afraid that this bug has broken the fullscreen mode on win7. See Bug 1513066.
Depends on: 1513066
Depends on: 1514501
Flags: qe-verify+
Duplicate of this bug: 1514849

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).

Flags: needinfo?(ryanvm)

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

Flags: needinfo?(ryanvm) → needinfo?(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.

Flags: needinfo?(jcristau) → needinfo?(agashlin)
Attached patch Rollup for esr60Splinter Review

[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

Flags: needinfo?(agashlin)
Attachment #9036747 - Flags: review+
Attachment #9036747 - Flags: approval-mozilla-esr60?

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

fullscreen fix for windows, approved for 60.5esr

Attachment #9036747 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+

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.

Status: RESOLVED → VERIFIED
Flags: needinfo?(ciprian.georgiu)

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

Flags: qe-verify+
Flags: needinfo?(ciprian.georgiu)

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)?

Flags: needinfo?(anca.soncutean)

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

Flags: needinfo?(anca.soncutean)
No longer blocks: 1480928

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.

(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.

You need to log in before you can comment on or make changes to this bug.