wrong window sizing when session restore moves between lo- and hi-dpi screens

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

({regression})

Trunk
mozilla47
Unspecified
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

AFAIK so far, this is OS X-only...

STR:
Use a retina MacBook with a lo-dpi external monitor attached, so there are two screens, one with 2x devicePixelRatio and one with 1x.

Run Nightly with one window open, and position it on the main monitor.

Ensure prefs are set to NOT immediately restore the previous session when launching, but instead present the home page with the Restore button.

Quit the browser.

Re-launch the browser; the window should re-open in its previous position on the main display.

*BEFORE* clicking the Restore button, move the window to the other display.

Then click Restore; note how the window moves back to the display where it was when the session was saved (correct) but is sized incorrectly (double size if it was moved from hi- to lo-dpi; half size in the opposite case).


This doesn't happen with release Firefox. I haven't specifically bisected, but I'm confident it's a regression from bug 890156.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8719018 [details] [diff] [review]
patch 1 - Provide a desktop-pixel variant of SetPosition on nsIBaseWindow and its implementations

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

::: docshell/base/nsDocShell.cpp
@@ +5821,5 @@
> +nsDocShell::SetPositionDesktopPix(int32_t aX, int32_t aY)
> +{
> +  double scale = 1.0;
> +  GetDevicePixelsPerDesktopPixel(&scale);
> +  return SetPosition(NSToIntRound(aX * scale), NSToIntRound(aY * scale));

Is this calculation correct when mixed DPI monitors are present on the system?

::: dom/base/nsGlobalWindow.cpp
@@ +7058,5 @@
> +
> +    screen->GetContentsScaleFactor(&scale);
> +    DesktopPoint deskPos = devPos / DesktopToLayoutDeviceScale(scale);
> +    aError = treeOwnerAsWin->SetPositionDesktopPix(screenLeftDeskPx + deskPos.x,
> +                                                   screenTopDeskPx + deskPos.y);

Why this code changes device pixels to desktop pixels, then changing them back to device pixels (in SetPositionDesktopPix)?
(In reply to Masatoshi Kimura [:emk] from comment #4)
> Comment on attachment 8719018 [details] [diff] [review]
> patch 1 - Provide a desktop-pixel variant of SetPosition on nsIBaseWindow
> and its implementations
> 
> Review of attachment 8719018 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: docshell/base/nsDocShell.cpp
> @@ +5821,5 @@
> > +nsDocShell::SetPositionDesktopPix(int32_t aX, int32_t aY)
> > +{
> > +  double scale = 1.0;
> > +  GetDevicePixelsPerDesktopPixel(&scale);
> > +  return SetPosition(NSToIntRound(aX * scale), NSToIntRound(aY * scale));
> 
> Is this calculation correct when mixed DPI monitors are present on the
> system?

I'm not sure, TBH.... but as far as I know this method is not currently called on an actual nsDocShell instance. So this is really just a placeholder implementation, to be reconsidered if we have examples where it is actually needed and can be tested. (I should add a comment to this effect.)

(Another option might be to make this fail with NS_ERROR_NOT_IMPLEMENTED, until/unless we find an actual need for it?)

The same applies to the implementations in nsDocShellTreeOwner and nsWebBrowser. It's the nsChromeTreeOwner and nsContentTreeOwner implementations that matter here, and those forward to nsXULWindow's SetPositionDesktopPix to avoid doing any conversion.


> 
> ::: dom/base/nsGlobalWindow.cpp
> @@ +7058,5 @@
> > +
> > +    screen->GetContentsScaleFactor(&scale);
> > +    DesktopPoint deskPos = devPos / DesktopToLayoutDeviceScale(scale);
> > +    aError = treeOwnerAsWin->SetPositionDesktopPix(screenLeftDeskPx + deskPos.x,
> > +                                                   screenTopDeskPx + deskPos.y);
> 
> Why this code changes device pixels to desktop pixels, then changing them
> back to device pixels (in SetPositionDesktopPix)?

In practice, treeOwnerAsWin is an instance of nsChromeTreeOwner or nsContentTreeOwner, which forwards the parameters directly as desktop pixels; this is necessary because converting through device pixels and back on OS X can lead to incorrect behavior, whereas desktop pixels are globally safe.
Revised version of patch 1, with the unused(?) methods returning unimplemented. Try run with this version: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd93e4cb9319.
Attachment #8719935 - Flags: review?(VYV03354)
Attachment #8719018 - Attachment is obsolete: true
Attachment #8719018 - Flags: review?(VYV03354)
Attachment #8719935 - Flags: review?(VYV03354) → review+
Attachment #8719019 - Flags: review?(VYV03354) → review+
Attachment #8719020 - Flags: review?(VYV03354) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/291cf27e6e55e20279fdb53219038b171abc945a
Bug 1247335 - patch 1 - Provide a desktop-pixel variant of SetPosition on nsIBaseWindow and its implementations. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/134d63b3e942785c325d7c1a35d9c46c9775a072
Bug 1247335 - patch 2 - Use desktop pixel coordinates when loading a nsXULWindow position. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/9feea2de5e1d4c39d2f1d0a69b77a493fa09c4a0
Bug 1247335 - patch 3 - Check for potential DPI change after moving or resizing nsGlobalWindow. r=emk
https://hg.mozilla.org/mozilla-central/rev/291cf27e6e55
https://hg.mozilla.org/mozilla-central/rev/134d63b3e942
https://hg.mozilla.org/mozilla-central/rev/9feea2de5e1d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1252347
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.