Broken Firefox layout when transition from lowdpi to hidpi monitor

VERIFIED FIXED in Firefox 47

Status

()

Core
Widget: Win32
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: bogdan_maris, Assigned: jfkthame)

Tracking

Trunk
mozilla48
All
Windows
Points:
---

Firefox Tracking Flags

(firefox45 unaffected, firefox46 unaffected, firefox47blocking verified, firefox48 verified)

Details

Attachments

(2 attachments)

Created attachment 8740799 [details]
Screenshot showing the issue

[Affected versions]:
- latest Developer Edition 47.0a2
- latest Nightly 48.0a1

[Affected platforms]:
- Windows 8.1 64-bit
- Windows 10 64-bit

[Steps to reproduce]:
- prerequisits: hidpi and lowdpi monitor

1. Open Firefox
2. Drag Firefox on lowdpi monitor
3. Move Firefox to hidpi monitor just a little (Just before it resizes on hidpi).
4. Move cursor to hidpi and from there 'slowly' complete the action of moving Firefox to hidpi. (doing it fast will not reproduce the issue that easy)

extra step for navbar icons hidden in dropdown:

5. Move the window to lowdpi again.

Actual:
Windows resizes when completely moved to hidpi and Firefox layout will be broken (on some cases).
 - resizing Firefox will fix the problems.
Also mouse cursor will be misplaced as well, as in
bug: 1262398

[Expected result]:
- Transition is good.

[Actual result]:
- Windows resizes when completely moved to hidpi and Firefox layout will be broken (on some cases).
- resizing Firefox will fix the problems.
Also mouse cursor will be misplaced as well, as in bug: 1262398

[Regression range]:
- This is not a regression, it reproduces with old Nightly (2016-01-13) build as well even easier.

[Additional notes]:
- This is not reproducible using Firefox 45.0.2 and 46 beta 10.
Component: Widget: Gtk → Widget: Win32
tracking-firefox47: --- → ?

Comment 1

2 years ago
SV has called this out as a potential blocker to transition this feature from Aurora to Beta 47. I will mark it as "blocking" and wait for Jonathan's investigation to complete.
tracking-firefox47: ? → blocking

Updated

2 years ago
Flags: needinfo?(bugs)
(Assignee)

Comment 2

2 years ago
Bogdan, could you please test with the tryserver build from https://treeherder.mozilla.org/#/jobs?repo=try&revision=05445e2ea2ed and let me know whether this makes any difference? I don't think it fully resolves the issue, but it may help with some of the cases where we "miss" a DPI change and end up with incorrect scaling.
Flags: needinfo?(bogdan.maris)
(Assignee)

Comment 3

2 years ago
Created attachment 8741908 [details] [diff] [review]
Add an extra check for DPI change during window drag, because we sometimes miss a WM_DPICHANGED message

This seems a bit hackish, but it makes things a lot better -- it appears that we sometimes don't get a WM_DPICHANGED message when we should, or perhaps the timing of messages doesn't work for us, leaving the window in an inconsistent state. Forcing an extra DPI-synchronization during window moving helps with this. There are still cases where you can get a window into a slightly odd state while it's right at the transition point between two displays, but it now sorts itself out nicely as the move continues. Although this theoretically adds some overhead during a window drag, I don't think that should matter in practice.
Attachment #8741908 - Flags: review?(VYV03354)
(Assignee)

Updated

2 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
Try run with the patch from comment 3:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=61442e4c9962

Bogdan, any testing with this build would be appreciated - thanks!
Comment on attachment 8741908 [details] [diff] [review]
Add an extra check for DPI change during window drag, because we sometimes miss a WM_DPICHANGED message

Review of attachment 8741908 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +5374,5 @@
>      case WM_MOVING:
>        FinishLiveResizing(MOVING);
> +      if (WinUtils::IsPerMonitorDPIAware()) {
> +        // Sometimes, we appear to miss a WM_DPICHANGED message while moving
> +        // a window around, so fix things up here if necessary.

Where is the "if necessary" check done? I couldn't find it.
(Assignee)

Comment 6

2 years ago
There's no check _here_, because I haven't been able to find a way to reliably determine (here in the widget code) whether everything throughout the device context / pres context / view system / etc... is properly in sync. So this unconditionally calls ChangedDPI (which will notify the presContext) and ResetLayout (which will lead to firing OnResize). But both of these operations will ultimately detect if they actually need to change anything, and return harmlessly if things are already as expected.

Specifically, nsPresContext::UIResolutionChangedInternalScale will check whether the context's appUnitsPerDevPixel value has changed, and only call AppUnitsPerDevPixelChanged() -- which will trigger style flushing and reflow -- if there has been a change. If not, this is reasonably cheap.

And ResetLayout will eventually lead to calling WindowResized on the widget listener, which ends up with nsXULWindow::SetPositionAndSize calling nsWindow::Resize, which will check whether the size has actually changed, and return without doing anything if it's already up to date.

The important point here is that these calls do NOT lead to any extra reflow or painting during a window move; this can be confirmed by enabling nglayout.debug.paint_flashing and observing that we do not get extra paint-flashing during a window drag.

Probably the comment here would be better as something like:

  // Sometimes, we appear to miss a WM_DPICHANGED message while moving
  // a window around. Therefore, call ChangedDPI and ResetLayout here,
  // which causes the prescontext and appshell window management code to
  // check the appUnitsPerDevPixel value and current widget size, and
  // refresh them if necessary. If nothing has changed, these calls will
  // return without actually triggering any extra reflow or painting.

Does that make more sense now?
Comment on attachment 8741908 [details] [diff] [review]
Add an extra check for DPI change during window drag, because we sometimes miss a WM_DPICHANGED message

r=me assuming that this patch will fix Bogdan's issue.

(In reply to Jonathan Kew (:jfkthame) from comment #6)
> Probably the comment here would be better as something like:
> 
>   // Sometimes, we appear to miss a WM_DPICHANGED message while moving
>   // a window around. Therefore, call ChangedDPI and ResetLayout here,
>   // which causes the prescontext and appshell window management code to
>   // check the appUnitsPerDevPixel value and current widget size, and
>   // refresh them if necessary. If nothing has changed, these calls will
>   // return without actually triggering any extra reflow or painting.
> 
> Does that make more sense now?

Works for me.
Attachment #8741908 - Flags: review?(VYV03354) → review+
(Assignee)

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a007f40ee34d05c5d752aa6e91cc6ed881c7b7a6
Bug 1264193 - Add extra check for DPI changes during window drag, because we sometimes miss a WM_DPICHANGED message. r=emk
(Assignee)

Comment 9

2 years ago
Landed on inbound, so as to get this into nightlies for further testing.

Bogdan, if you can confirm whether this fixes the issue you were seeing (using the try build from comment 4, or with Nightly in a day or two once this goes out) that would be great - thanks.

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a007f40ee34d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(In reply to Jonathan Kew (:jfkthame) from comment #9)
> Landed on inbound, so as to get this into nightlies for further testing.
> 
> Bogdan, if you can confirm whether this fixes the issue you were seeing
> (using the try build from comment 4, or with Nightly in a day or two once
> this goes out) that would be great - thanks.

Sorry for the delay. 
I checked with the try build from comment 4 and latest tinderbox nightly build and the layout issues during transition seems to be gone. Firefox is still resizes though and cursor will be misplaced but I think that this fix only handles the layout issue. 

Here is a video of the current behavior https://db.tt/RGnxWsWz
Flags: needinfo?(bogdan.maris)
(Assignee)

Comment 12

2 years ago
So it looks like the window repeatedly "jumps" back as you drag further onto the other display? Curious... I haven't seen that behavior, but I'll rearrange my displays and try to reproduce. Please file a separate bug for it, though; I think that's distinct from the sometimes-broken layout we were seeing that was fixed here.

Comment 13

2 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #12)
> Please file a separate bug for it, though; I think that's distinct from the sometimes-broken layout we were seeing that was fixed here.

Agreed. The window resizing appears related to the cursor location getting mapped to a coordinate space other than the screen where the mousedown was initiated. Definitely a separate bug.
Flags: needinfo?(bugs)
(Assignee)

Comment 14

2 years ago
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #11)
> Here is a video of the current behavior https://db.tt/RGnxWsWz

Also (in the new bug about this), could you give full details of the display configuration you're using there? Pixel sizes of the monitors; devicePixelRatio scale factors for each of them (you can use https://people.mozilla.org/~jkew/tests/devPixRatio.html to easily determine this); which is the system's primary monitor. And have you signed out of Windows since any reconfiguration (either attaching/detaching a display, or changing settings in the control panel) happened?
Flags: needinfo?(bogdan.maris)
(Assignee)

Comment 15

2 years ago
Comment on attachment 8741908 [details] [diff] [review]
Add an extra check for DPI change during window drag, because we sometimes miss a WM_DPICHANGED message

Approval Request Comment
[Feature/regressing bug #]: 890156

[User impact if declined]: after dragging window between displays with different DPI, the layout (scaling of chrome & content) is sometimes left in a broken state -- fixed by moving the window back and forth again

[Describe test coverage new/current, TreeHerder]: manual

[Risks and why]: minimal, just adds a "double-check" while moving windows around

[String/UUID change made/needed]: none
Attachment #8741908 - Flags: approval-mozilla-aurora?
(In reply to Jonathan Kew (:jfkthame) from comment #14)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #11)
> > Here is a video of the current behavior https://db.tt/RGnxWsWz
> 
> Also (in the new bug about this), could you give full details of the display
> configuration you're using there? Pixel sizes of the monitors;
> devicePixelRatio scale factors for each of them (you can use
> https://people.mozilla.org/~jkew/tests/devPixRatio.html to easily determine
> this); which is the system's primary monitor. And have you signed out of
> Windows since any reconfiguration (either attaching/detaching a display, or
> changing settings in the control panel) happened?

Logged bug 1265977 on that. I will go ahead and mark this bug as verified fixed in Nightly. Keeping the status as resolved fixed until the fix reaches Aurora as well, also keeping the needinfo as a reminder to verify this in 47 as well.
status-firefox48: fixed → verified
Comment on attachment 8741908 [details] [diff] [review]
Add an extra check for DPI change during window drag, because we sometimes miss a WM_DPICHANGED message

Fix was verified on Nightly, per-monitor DPI related, Aurora47+
Attachment #8741908 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 18

2 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/5c483e91cdc6
status-firefox47: affected → fixed
Verified fixed on Aurora 47 as well.
Status: RESOLVED → VERIFIED
status-firefox47: fixed → verified
Flags: needinfo?(bogdan.maris)
You need to log in before you can comment on or make changes to this bug.