Closed Bug 1259707 Opened 8 years ago Closed 8 years ago

session restore may put windows off-screen (fails to constrain window placement to available screen space)

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(1 file, 1 obsolete file)

This is a bug caused by incorrectly mixing display (desktop) pixels with CSS px coordinates in the ssi_restoreDimensions function.

STR, on Windows 8.1 (or presumably Win10) with two displays:

* Arrange displays in the Display control panel with the secondary monitor to the right of the primary one

* Launch Nightly, and put its window on the secondary monitor; load some website, maybe even a couple of tabs (to make successful Session-Restore recognizable)

* Quit Nightly

* Rearrange displays to put the secondary monitor on the left of the primary one

* Re-launch Nightly

(Note that until bug 1259492 is fixed, the window will be virtually off-screen at this point, and you'll need to right-click its icon in the taskbar and choose Move, then press left-arrow to bring it into view.)

* Now click the Restore Previous Session button

Expected:
The previous session's tabs should be restored, within the visible window

Actual:
The window disappears, because Session Restore has put it off-screen where the secondary monitor used to be, but there's now nothing.

(To retrieve the window, once again right-click the taskbar icon and Move it. But this is a lousy user experience.)
This fixes the unit mismatch, and properly constrains the restored window to the currently-available space.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Attachment #8734689 - Flags: review?(VYV03354)
Comment on attachment 8734689 [details] [diff] [review]
Fix confusion between desktop and CSS pixels when session-restore is constraining window to the available screen space

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

::: browser/components/sessionstore/SessionStore.jsm
@@ +3448,5 @@
> +      screen.GetAvailRect(screenLeft, screenTop, screenWidth, screenHeight);
> +      // convert screen's device pixels to CSS px dimensions
> +      let scale = screen.defaultCSSScaleFactor;
> +      screenLeft = screenLeft.value / scale;
> +      screenTop = screenTop.value / scale;

Is this calculation correct? .screenX/.screenY is doing something more complex due to bug 1240085.
Comment on attachment 8734689 [details] [diff] [review]
Fix confusion between desktop and CSS pixels when session-restore is constraining window to the available screen space

Ah, you're right of course, that's not correct.

This can be demonstrated by placing a small window in each corner of a secondary lower-dpi monitor, quitting the browser, then relaunching and restoring the session: the windows in the corners further from the primary monitor will be "pulled in" towards the middle.

I've fixed the patch locally, but want to test across other platforms as well before I post an updated one for review.
Attachment #8734689 - Attachment is obsolete: true
Attachment #8734689 - Flags: review?(VYV03354)
OK, I think this is closer to what we need; this restores windows correctly for me across all areas of mixed-dpi screens on both Win8.1 and OS X, afaict. There's a try build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=779a7e3ed4cd.
Attachment #8735577 - Flags: review?(VYV03354)
Comment on attachment 8735577 [details] [diff] [review]
Fix confusion between desktop and CSS pixels when session-restore is constraining window to the available screen space

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

::: browser/components/sessionstore/SessionStore.jsm
@@ +3452,5 @@
> +      // convert screen's device pixel dimensions to CSS px dimensions
> +      screen.GetAvailRect(screenLeft, screenTop, screenWidth, screenHeight);
> +      let cssToDevScale = screen.defaultCSSScaleFactor;
> +      let screenWidthCss = screenWidth.value / cssToDevScale;
> +      let screenHeightCss = screenHeight.value / cssToDevScale;

It would be better to have GetAvailRectCSSPix(), but we can do it in follow-ups.
Attachment #8735577 - Flags: review?(VYV03354) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/09472720d1fba2515e13c829909f4ce5739c566d
Bug 1259707 - Fix confusion between desktop and CSS pixels when session-restore is constraining window to the available screen space. r=emk
https://hg.mozilla.org/mozilla-central/rev/09472720d1fb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.