Closed Bug 1787079 Opened 3 years ago Closed 3 years ago

Moving window to new monitor does not always cause a redraw

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Platform:
Unspecified
Windows
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
106 Branch
Tracking Flags:
Tracking Status
firefox-esr91 --- unaffected
firefox-esr102 --- unaffected
firefox104 --- unaffected
firefox105 + wontfix
firefox106 --- verified

People

(Reporter: rkraesig, Assigned: hiro)

References

(Regression)

Details

(Keywords: regression)

Attachments

(6 files)

Bug 1787079 - Use mDefaultScale in BrowserParent::GetDimensionInfo for consistency. r?rkraesig
48 bytes, text/x-phabricator-request
Details | Review
Bug 1787079 - Invalidate BrowserParent's dimensions when the corresponding default scale factor is changed. r?rkraesig
48 bytes, text/x-phabricator-request
Details | Review
Bug 1787079 - Expose gfx::GetDisplayConfig(). r?#gfx-reviewers
48 bytes, text/x-phabricator-request
Details | Review
Bug 1787079 - Implement nsIDOMWindowUtils.setHiDPiMode and restoreHiDPIMode on Windows. r?rkraesig
48 bytes, text/x-phabricator-request
Details | Review
Bug 1787079 - Tweak browser_bug1757410.js to be able to run on Windows and run it on Windows 10 x64. r?tnikkel
48 bytes, text/x-phabricator-request
Details | Review
Bug 1787079 - Call UIResolutionChangedInternal instead of AppUnitsPerDevPixelChanged in nsPresContext::PreferenceChanged. r?#layout
48 bytes, text/x-phabricator-request
Details | Review

When moving a fullscreen window from one monitor to another, the window contents may not necessarily update correctly.

Steps to reproduce:

  • Start Firefox on a Windows machine with two or more monitors of different sizes and DPI scaling settings.
  • Go to the URL data:text/html,<body bgcolor=magenta>.
  • Enter fullscreen mode by pressing F11.
  • Cycle the window from one monitor to the next by pressing Win + Shift + Left/Right-Arrow.

Expected results:
The Firefox window will always be fullscreen and everywhere magenta (possibly after a frame or two for reflow and redraw).

Actual results:
The Firefox window will always be fullscreen, but after at least one of the transitions it will be white with a magenta upper-left corner.

This was seen on a Windows laptop with a 3456x2160 screen at 175% DPI and a 1920x1200 screen at 100% DPI.

mozregression's regression range is a single commit in length:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c1245a74819a71fdcbdf0196d65552a582d98295&tochange=3459b021f7a5f37433d9784e1421e571e73a6113

Set release status flags based on info from the regressing bug 1757410

:hiro, since you are the author of the regressor, bug 1757410, could you take a look?
For more information, please visit auto_nag documentation.

status-firefox104: --- → unaffected
status-firefox105: --- → affected
status-firefox106: --- → affected
status-firefox-esr102: --- → unaffected
status-firefox-esr91: --- → unaffected
Flags: needinfo?(hikezoe.birchill)

I finally got a setup to reproduce this issue. FWIW I initially tried to reproduce on Mac, but I can't find any shortcuts to move Firefox window between monitors.

The change in question is;

--- a/layout/base/nsDocumentViewer.cpp
+++ b/layout/base/nsDocumentViewer.cpp
@@ -1975,17 +1975,17 @@ nsDocumentViewer::SetBoundsWithFlags(con
     mWindow->Resize(aBounds.x, aBounds.y, aBounds.width, aBounds.height, false);
   } else if (mPresContext && mViewManager) {
     // Ensure presContext's deviceContext is up to date, as we sometimes get
     // here before a resolution-change notification has been fully handled
     // during display configuration changes, especially when there are lots
     // of windows/widgets competing to handle the notifications.
     // (See bug 1154125.)
     if (mPresContext->DeviceContext()->CheckDPIChange()) {
-      mPresContext->UIResolutionChanged();
+      mPresContext->UIResolutionChangedSync();
     }

I'd want to think that the original async version of UIResolutionChanged had wall-papered this issue. In fact, with dumping mBounds in SetBoundsWithFlags when I moved a fullscreen-ed window to 1920x1080 external monitor, the last value was 1097x617 which apparently looks wrong. Moreover just before the wrong value, I see the correct value, 1920x1080. Messy..

Severity: -- → S3
Component: Panning and Zooming → Layout
Priority: -- → P3

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)

I finally got a setup to reproduce this issue. FWIW I initially tried to reproduce on Mac, but I can't find any shortcuts to move Firefox window between monitors.

While apparently no such shortcut exists on Mac by default, I found this mechanism to create one while researching a related issue. (I have not personally tested it, however.)

[Tracking Requested - why for this release]: user-facing regression in 105, would be nice to fix before we ship.

tracking-firefox105: --- → ?

(In reply to Ray Kraesig [:rkraesig] from comment #4)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)

I finally got a setup to reproduce this issue. FWIW I initially tried to reproduce on Mac, but I can't find any shortcuts to move Firefox window between monitors.

While apparently no such shortcut exists on Mac by default, I found this mechanism to create one while researching a related issue. (I have not personally tested it, however.)

Yeah, I tried it but unfortunately it can't be applied to Firefox window.

Anyways, what I've found so far when a fullscreen-ed Firefox window is moved to an external monitor ;

  1. a WM_WINDOWPOSCHANGED message is delivered prior to a WM_DPICHANGED message, at the moment the DPI (I queried it by using WinUtils::LogToPhysFactor) is still the one for the original monitor
  2. in response to the WM_WINDOWPOSCHANGED message, we end up sending a new dimensions to the top level document, it ends up calling the SetBoundsWithFlags in question
  3. unfortunately with the wrong DPI we incorrectly calculate the dimension, it would result the incorrect mBounds
  4. when we receive a WM_DPICHANGED, we also try to update the dimension with the new correct DPI via this ResetLayout call
  5. but unfortunately it doesn't happen because none of these metrics hasn't been changed since the last update, these metrics are not affected by the DPI.

I would say, 1) is a bug in Windows, if it's intentional, I would say it's horribly error-prone.

I will figure out a way to fix this issue.

Assignee: nobody → hikezoe.birchill
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe.birchill)
OS: Unspecified → Windows
See Also: → 1787987

I tried to write a mochitest on Mac with nsIDOMWIndowUtils.setHiDPIMode (which I added in bug 1757410) since I found a similar issue on Mac (bug 1787987), but it turned out that the setHiDPIMode isn't applicable for this bug since this bug needs to involve window size change notification from the OS when the DPI is changed.

For Windows, there are undocumented APIs to set/get the DPI (for a given monitor I suppose) so that we could probably use the APIs and write a mochitest with the APIs. That's said, I hesitate to add code using such undocumented APIs. Ray, I believe you have the expertise about Windows, any suggestions?

Flags: needinfo?(rkraesig)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)

  1. a WM_WINDOWPOSCHANGED message is delivered prior to a WM_DPICHANGED message, at the moment the DPI (I queried it by using WinUtils::LogToPhysFactor) is still the one for the original monitor

[...]

  1. but unfortunately it doesn't happen because none of these metrics hasn't been changed since the last update, these metrics are not affected by the DPI.

I would say, 1) is a bug in Windows, if it's intentional, I would say it's horribly error-prone.

I believe 1) is necessary to get correct behavior from pre-Windows-8.1 applications which are not per-monitor-DPI-aware. It will appear to them as though they've been moved to another monitor and then unrelatedly that the system DPI has changed.

But that virtual sequence of events is still possible as a physical sequence of events in modern Windows -- and the fact that we don't handle it properly when it's virtual suggests that we won't handle it properly when it's real! And, indeed, I can eliminate the second monitor from the steps-to-reproduce for this bug:

Steps to reproduce:

  • Start Firefox on a Windows machine with at least one monitor whose current DPI scaling is larger than its minimum DPI scaling. Make sure Firefox is on that monitor.
  • Go to the URL data:text/html,<body bgcolor=magenta>.
  • Enter fullscreen mode by pressing F11.
  • From Control Panel > System > Display, reduce the DPI of the relevant monitor.

Expected results:
The Firefox window will still be fullscreen and everywhere magenta (possibly after a frame or two for reflow and redraw).

Actual results:
The Firefox window will still be fullscreen, but it will be white with a magenta upper-left corner.

I think the bug here is therefore in 5) -- that the conditional in BrowserParent::UpdateDimensions isn't sufficiently DPI-aware, and is incorrectly preventing the update, in both cases.

 

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)

For Windows, there are undocumented APIs to set/get the DPI (for a given monitor I suppose) so that we could probably use the APIs and write a mochitest with the APIs. That's said, I hesitate to add code using such undocumented APIs. Ray, I believe you have the expertise about Windows, any suggestions?

I'd prefer to avoid use of undocumented APIs in Firefox itself unless absolutely necessary.

In test code, on the other hand, use of undocumented APIs is probably fine so long as there's a clear explanation of what and why. (It might not be a bad idea to link to, e.g., https://stackoverflow.com/a/57397039, or whatever unofficial documentation seems most appropriate.)

(Incidentally, I'm very interested to see how one writes a mochitest with this capability! I had thought this sort of OS-integration test to be beyond the reach of mochitests.)

Flags: needinfo?(rkraesig)
See Also: → 1722085

The bug is marked as tracked for firefox105 (beta). However, the bug still has low priority and has low severity.

:fgriffith, could you please increase the priority and increase the severity for this tracked bug? Given that it is a regression and we know the cause, we could also simply backout the regressor. If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit auto_nag documentation.

Flags: needinfo?(fgriffith)

Bumping up priority P2. That's said, though I've already written the fix, we will not uplift this fix to beta. I'd like to add a mochitest to avoid further regressions. Writing the mochitest is a challenge, and it will take some amount of time, but it's worth trying I believe. We've regressed this area repeatedly.

Thank you, Ray. I will try to use the undocumented API.

status-firefox105: affected → wontfix
Flags: needinfo?(fgriffith)
Priority: P3 → P2

Would it be reasonable to backout bug 1757410 from Beta then to avoid shipping this regression?

Flags: needinfo?(hikezoe.birchill)

I don't think backing out is reasonable because bug 1757410 fixed more common cases than this bug. Bug 1757410 covers cases where moving Firefox window by dragging window's bar, whereas this bug happens only in the case moving the window by keyboard shortcut on full-screened state, as far as I can tell it happens only on Windows (GNOME may also have a shortcut though).

Note that the another step to reproduce Ray found in comment 10 is a pre-existing issue (bug 1787987), it's not a regression.

Flags: needinfo?(hikezoe.birchill)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)

as far as I can tell it happens only on Windows (GNOME may also have a shortcut though).

GNOME's default shortcuts are patterned after Windows': Super + Shift + ← and Super + Shift + →. (I have not tested there.) KDE has no default shortcut, but one can be configured, and I would expect its behavior in this matter to be identical to that of GNOME.

Note that the another step to reproduce Ray found in comment 10 is a pre-existing issue (bug 1787987), it's not a regression.

It may be preexisting on Mac, but it is a regression on Windows; it does not occur in Fx104.0 release on Windows 10.

(In reply to Ray Kraesig [:rkraesig] from comment #15)

Note that the another step to reproduce Ray found in comment 10 is a pre-existing issue (bug 1787987), it's not a regression.

It may be preexisting on Mac, but it is a regression on Windows; it does not occur in Fx104.0 release on Windows 10.

Thanks, that's a kinda good news. The mochitest I was going to add is exactly the same case. Another good news is I've confirmed the mochitest using the undocumented APIs works expected both on our CIs and my local laptop (Thinkpad T460p).

We'd like to use it for changing DPI settings for the primary monitor.

This also includes a change to make sure restoreHiDPIMode gets called even if
the test failed.

Depends on D156265

Attachment #9292200 - Attachment description: WIP: Bug 1787079 - Use mDefaultScale in BrowserParent::GetDimensionInfo for consistency. → Bug 1787079 - Use mDefaultScale in BrowserParent::GetDimensionInfo for consistency. r?rkraesig
Attachment #9292201 - Attachment description: WIP: Bug 1787079 - Invalidate BrowserParent's dimensions when the corresponding default scale factor is changed. → Bug 1787079 - Invalidate BrowserParent's dimensions when the corresponding default scale factor is changed. r?rkraesig

It turned out that D155946 makes fixed-width-viewport-inflation.html fail. https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=e25Val8rSgOt470PgrY8Nw.0&revision=681540b49446f593701f070234bec5dfa2f1c98b

The reason is, the test changes layout.css.devPixelsPerPx, but the change isn't propagated into BrowserParent, so that we use wrong BrowserParent::mDefaultScale value there.

I will upload a change to fix it.

AppUnitsPerDevPixelChanged doesn't propagate the change to BrowserParent, thus
BrowserParent::mDefaultScale will be stale. UIResolutionChangedInternal
propagates the change into BrowserParent properly so that the mDefaultScale
will be properly updated.

Another approach is to observe preference changes in BrowserParent itself, but
observing layout specific preferences, "layout.css.dpi" or
"layout.css.devPixelsPerPx" in BrowserParent is a bit odd.

Without this change D155946 makes fixed-width-viewport-inflation.html fail
because the test changes "layout.css.devPixelsPerPx" value.

Depends on D156266

Pushed by hikezoe.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3580b308ec59 Expose gfx::GetDisplayConfig(). r=gfx-reviewers,lsalzman https://hg.mozilla.org/integration/autoland/rev/1a9828bbf04b Implement nsIDOMWindowUtils.setHiDPiMode and restoreHiDPIMode on Windows. r=rkraesig https://hg.mozilla.org/integration/autoland/rev/650f8d455e0e Tweak browser_bug1757410.js to be able to run on Windows and run it on Windows 10 x64. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/dce92508b5a3 Call UIResolutionChangedInternal instead of AppUnitsPerDevPixelChanged in nsPresContext::PreferenceChanged. r=emilio https://hg.mozilla.org/integration/autoland/rev/d5d0f8bb0be4 Use mDefaultScale in BrowserParent::GetDimensionInfo for consistency. r=rkraesig https://hg.mozilla.org/integration/autoland/rev/b6075adb01a7 Invalidate BrowserParent's dimensions when the corresponding default scale factor is changed. r=rkraesig

Backed out 6 changesets (bug 1787079) for causing browser-chrome failures in layout/base/tests/browser_bug1787079.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/c5ad9cb06bf1bcc837beff050095e2e8944f2188

Push with failures

Failure log

TEST-UNEXPECTED-FAIL | layout/base/tests/browser_bug1787079.js | window.innerWidth on a lower DPI should be greater than the original -
Flags: needinfo?(hikezoe.birchill)

I wonder why it failed so frequent on our CIs. I finally got a failure in 1977 runs locally. :/

The test in question didn't wait for devicePixelRatio change in the content process, it rather waited in the parent process. :/

I wonder why it succeeds on my local machine?

Flags: needinfo?(hikezoe.birchill)
Pushed by hikezoe.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7cf9cb2d96e5 Expose gfx::GetDisplayConfig(). r=gfx-reviewers,lsalzman https://hg.mozilla.org/integration/autoland/rev/33e0dada0ccb Implement nsIDOMWindowUtils.setHiDPiMode and restoreHiDPIMode on Windows. r=rkraesig https://hg.mozilla.org/integration/autoland/rev/607122339f2f Tweak browser_bug1757410.js to be able to run on Windows and run it on Windows 10 x64. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/03f45da3ba4e Call UIResolutionChangedInternal instead of AppUnitsPerDevPixelChanged in nsPresContext::PreferenceChanged. r=emilio https://hg.mozilla.org/integration/autoland/rev/da8934f507a8 Use mDefaultScale in BrowserParent::GetDimensionInfo for consistency. r=rkraesig https://hg.mozilla.org/integration/autoland/rev/81eeb8a7fcbd Invalidate BrowserParent's dimensions when the corresponding default scale factor is changed. r=rkraesig
Flags: qe-verify+

Hi Ray! Can you please help us verifying this issue on Beta 106.0b9?

I am not able to reproduce the bug with my current setup, which is a a 1080p laptop attached to a 2k monitor.

Flags: needinfo?(rkraesig)

I can confirm that the bug reproduces on my setup on 105.0.2 release, but not on 106.0b9.

Flags: needinfo?(rkraesig)

Thank you! Marking this as verified fixed per comment 29.

Status: RESOLVED → VERIFIED
status-firefox106: fixed → verified
Flags: qe-verify+
Blocks: 1572995
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: