Closed Bug 864107 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 23
Tracking Status
firefox20 --- affected
firefox21 --- affected
firefox22 + verified
firefox23 + verified

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Depends on 2 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

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.
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).
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.
Attachment #740095 - Flags: review?(gavin.sharp)
Assignee: nobody → jfkthame
(v2) added missing variable declarations.
Attachment #740099 - Flags: review?(gavin.sharp)
Attachment #740095 - Attachment is obsolete: true
Attachment #740095 - Flags: review?(gavin.sharp)
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...
Attachment #740099 - Flags: review?(gavin.sharp)
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.
Attachment #740099 - Attachment is obsolete: true
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
Attachment #740195 - Flags: review?(gavin.sharp)
Attachment #740195 - Flags: review?(gavin.sharp) → review?(ttaubert)
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.
BTW, :markh confirmed (see bug 852371 comment 13) that this fixes the restored-window problem caused by enabling hi-dpi support.
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!
Attachment #740195 - Flags: review?(ttaubert) → review+
OS: Windows 7 → All
Hardware: x86_64 → All
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/dde0f2ca02d8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
Attachment #740195 - Flags: approval-mozilla-aurora?
Attachment #740195 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: FF2SM
Blocks: 872324
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).
No longer blocks: FF2SM
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).
Status: RESOLVED → VERIFIED
Keywords: verifyme
Blocks: 890125
No longer blocks: 890125
Depends on: 890125
Depends on: 1112374
Depends on: 891930
Depends on: 1276516
You need to log in before you can comment on or make changes to this bug.