864107 - session restore does not ensure restored windows are actually on-screen

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Jonathan Kew [:jfkthame]]

When Firefox restores a previous session, it tries to position the window(s) at the original screen location.

However, it does not check that the resulting position is actually on-screen. This means that if the user had a window open on a large screen (or a multi-screen extended desktop), and then restores the session with just a single, perhaps much smaller display, windows that -were- fully visible on-screen may be restored to a position that is partially or even completely off-screen.

STR, using FF20 on a single-display machine:

1) Have the display set to its maximum resolution (i.e. pixel dimensions - NOT talking about the Windows "logical dpi" adjustments here). In my case, I have a 1920x1080 display on this laptop.

2) Open Firefox, and ensure it is set to reload the previous session on startup. Make the window small, and position it in the lower right of the display. Quit Firefox.

3) Re-launch Firefox, and confirm that Session Restore puts the window back in the same place at the lower right. Quit again.

4) In the Windows Display control panel, reduce the resolution (e.g. to 1024x768, i.e. simulate swapping a large-screen display for a standard VGA monitor).

5) Re-launch Firefox. Note that Session Restore moves the window to its "original" position, even though that is now completely off-screen due to the reduced amount of screen real-estate available.

I think Session Restore should sanity-check the window position it's restoring, and ensure that the window is actually on-screen. (Entirely, or is partially on-screen sufficient?)

This issue will be exacerbated by the transition to HiDPI support, currently in FF22, as this in effect reduces the available desktop space if the system is configured for a higher "logical DPI" - making everything on the screen bigger is equivalent to making the screen itself smaller, in terms of window placement.

Comment 1

[Jonathan Kew [:jfkthame]]

Dual-monitor STR:

1) With an external display attached to the laptop, and configured as extended desktop space to the right of the internal (main) screen, place the Firefox window on the external display.

2) Quit Firefox, re-launch: confirm that the window re-opens on the external display. Quit again.

3) Now disconnect the external display, and re-launch Firefox. The window is completely off-screen (in the "extended desktop" space that no longer exists).

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: constrain restored window dimensions to available screen area

This patch aims to ensure the restored window is actually constrained to the screen area. Appears to work OK in my local testing so far; I'll push a tryserver build (particularly for testing the issue in bug 852371) when the tree reopens after current maintenance downtime.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: constrain restored window dimensions to available screen area

(v2) added missing variable declarations.

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 740099 [details] [diff] [review]
constrain restored window dimensions to available screen area

Cancelling r? for now, as this fails too many tests...

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: constrain restored window dimensions to available screen area

OK, here's a patch that actually runs without JS errors, afaict :) ... I'll wait to see if tryserver is happy before requesting review, given how little I understand this code.

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 740195 [details] [diff] [review]
constrain restored window dimensions to available screen area

Tryserver build of Aurora (because we'll need this on Aurora for bug 852371) with this patch:
https://tbpl.mozilla.org/?tree=Try&rev=466376ad39bf

Comment 7

[Jonathan Kew [:jfkthame]]

Although this is actually a long-standing issue, nominating specifically for tracking-firefox22 because this is needed to avoid a poor user experience (as described in bug 852371) when Windows hi-dpi support rolls out.

Prior to that, it only affected users who made explicit changes to their system display configuration, but the transition to proper support of Windows display resolution will cause it to affect a large number of "unsuspecting" users at the point when their Firefox updates itself.

Comment 8

[Jonathan Kew [:jfkthame]]

BTW, :markh confirmed (see bug 852371 comment 13) that this fixes the restored-window problem caused by enabling hi-dpi support.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 740195 [details] [diff] [review]
constrain restored window dimensions to available screen area

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

Looks good, thanks!

Comment 10

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dde0f2ca02d8

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking as per comment 7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dde0f2ca02d8

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 740195 [details] [diff] [review]
constrain restored window dimensions to available screen area

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Longstanding bug, but the introduction of hi-dpi support for Windows will mean more users experience it.

User impact if declined: Restored windows may not fit on screen.

Testing completed (on m-c, etc.): In Nightly; also confirmed to fix bug 852371 using Aurora tryserver build (see comment 8).

Risk to taking this patch (and alternatives if risky): Low risk - code change is simple and well understood.

If there are users who have *deliberately* placed windows partly/fully off-screen, and expect Session Restore to maintain that position, this will change that behavior, and so could be seen as a regression - but it seems unlikely that is a widely-desired feature.

String or IDL/UUID changes made by this patch: None

Comment 14

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/69ec6f252570

Comment 15

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (X11; Linux i686; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20100101 Firefox/22.0

Verified as fixed on a single-display machine with Firefox 22 beta 2 (buildID: 20130521223249) and latest Nightly (buildID: 20130523030935).

Comment 16

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:23.0) Gecko/20100101 Firefox/23.0

Verified as fixed on a single-display machine with Firefox 23 beta 8 (buildID: 20130722172257) and latest Nightly (buildID: 20130723030205).