Closed Bug 1489214 Opened 6 years ago Closed 6 years ago

Window isn't restored in prior position on external-monitor with > 100% DPI

Categories

(Firefox :: Session Restore, defect)

62 Branch
Unspecified
Windows
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 64
Tracking Status
firefox-esr60 --- unaffected
firefox62 + verified
firefox63 + verified
firefox64 + verified

People

(Reporter: philipp, Assigned: florian)

References

(Regressed 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

there is feedback from some users on various support channels after the 62 update yesterday that their browser windows aren't restored in the last position in a new session anymore - all from users with a dual-monitor set-up so far: https://old.reddit.com/r/firefox/comments/9d6kcw/firefox_620_got_released_release_notes/e5g80wb/ https://support.mozilla.org/questions/1232497 https://support.mozilla.org/questions/1232388 the privacy.resistFingerprinting pref isn't in play here apparently.
feedback of affected users indicates that this is related to bug 1336227 and bug 1473142 as setting browser.startup.blankWindow to false can fix this behaviour.
Blocks: 1336227, 1473142
Flags: needinfo?(florian)
reproducible steps that show a somewhat similar problem with just a single monitor on win10 are: - open a window and set it to a custom size - make a note where its borders are on the screen - maximise that window and close firefox - restart firefox - click the button on the top right next to the x to restore firefox from maximised into the custom windowed mode. instead of ending up at the exact size set in the last session, firefox grows slightly wider to the right...
I'm also affected by this dreadful bug, it's clearly a regression somewhere with 62.0 as going back to 61.0.2 has no issue. As philipp says with the steps it's easy to reproduce 100% of the time.
Just wanted to add that setting browser.startup.blankWindow to false fixes this issue.
Comment 2 describes bug 1489852 for which I already attached a fix. There's a missing step to reproduce in comment 0: this bug only occurs when the window is on an external screen configured with a DPI setting > 100%.
Summary: Window isn't restored in prior position on multi-monitor set-ups after Firefox 62 update → Window isn't restored in prior position on external-monitor with > 100% DPI
Attached patch Patch (deleted) — Splinter Review
The top / left values that can be provided in the feature string when opening a window are in CSS pixels, but the screenX/screenY values that are persisted are in device pixels. There was already a hack to move the window when the screen was hidpi. That hack doesn't work when the window needs to be positionned on an external screen that doesn't have a 100% DPI. This patch simplifies by setting the window position using the screenX/screenY attributes, so the conversions are handled automatically by the platform code. My initial implementation of the blank window tried to set the size and position in the feature string to avoid moving/resizing the window, but it turns out that was already unavoidable, and seems to be cheap anyway before the window's first paint.
Attachment #9008418 - Flags: review?(mconley)
Assignee: nobody → florian
Status: NEW → ASSIGNED
Flags: needinfo?(florian)
Comment on attachment 9008418 [details] [diff] [review] Patch Review of attachment 9008418 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! It'd be nice if we could somehow add tests for this at some point, but I imagine that's not the easiest thing to do (especially for the multi-monitor scenario)
Attachment #9008418 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) (:⚙️) from comment #7) > Comment on attachment 9008418 [details] [diff] [review] > Patch > > Review of attachment 9008418 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good! Thanks! > It'd be nice if we could somehow add tests for this at some > point, but I imagine that's not the easiest thing to do (especially for the > multi-monitor scenario) I agree! I was actually wondering if I could come up with a way to get nsBrowserGlue.js to be loaded a second time in a different context during a bc test, so that it opens a blank window, and then open a browser window inside the blank window, and verify that the new window has the same size and position as the existing window. But that feels a bit too artificial, and I don't have a better idea. And also, that wouldn't help at all for the multi-monitor with mixed-DPI cases. For now I'm afraid I'll just have to ask QA to have a look at the result.
Pushed by florian@queze.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/e6c9581ba20e Set the position of the early blank window with the screenX/Y attributes rather than with the left/top features to avoid broken CSS <-> device pixel conversions in mixed DPI environments, r=mconley.
I think we should uplift to beta.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Comment on attachment 9008418 [details] [diff] [review] Patch Approval Request Comment [Feature/Bug causing the regression]: bug 1336227 [User impact if declined]: Browser windows on external screens with a DPI setting set to > 100% will be restored at incorrect positions. [Is this code covered by automated tests?]: No, this is hard to test. [Has the fix been verified in Nightly?]: Not yet. [Needs manual test from QE? If yes, steps to reproduce]: Yes, please. Steps to reproduce: - This only on Windows. Ensure an external screen is connected, and has a DPI setting set to > 100%. - Move the browser window to that screen. - Quit Firefox - Restart Firefox, and verify that the window is at the same position. It would be good to do some exploratory testing around possible variations (maximized windows, different DPI settings, different screen dispositions). I only tested on Win10, so I would appreciate if QA could also test other Windows versions. [List of other uplifts needed for the feature/fix]: The current patch applies on top of the patch from bug 1489852. [Is the change risky?]: Low risk. [Why is the change risky/not risky?]: It's a simplification of the existing code, to rely on the platform to convert coordinates between CSS pixels and device pixels, it removes an existing hack. [String changes made/needed]: none.
Attachment #9008418 - Flags: approval-mozilla-beta?
Comment on attachment 9008418 [details] [diff] [review] Patch Low risk patch for a regression, uplift approved for 63 beta 6. I'd like QE to verify the fix on Beta. thanks.
Flags: needinfo?(brindusa.tot)
Attachment #9008418 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I verified the bug on Nightly 64.0a1 (2018-09-13) and Beta 63.0b6 and the issue is fixed, I'd say partially. If i position Firefox between the 2 screens (a part of the window on Display 1 and a part of the window on Display 2), after restarting it, the Firefox window remains the same size but it is re-positioned to only one screen on the left/right edge. Pascal, do you think that this is the expected behaviour and I should mark the issue as verified, or it should be fixed?
Flags: needinfo?(pascalc)
(In reply to David Olah from comment #15) Thanks for testing the fix! > If i position Firefox between the 2 screens (a part of the window on Display > 1 and a part of the window on Display 2), after restarting it, the Firefox > window remains the same size but it is re-positioned to only one screen on > the left/right edge. This seems to be the behavior intentionally implemented in bug 1276516. > Pascal, do you think that this is the expected behaviour and I should mark > the issue as verified, or it should be fixed? Another way to ask the question is: has this behavior changed since Firefox 61? Or is the behavior different when browser.startup.blankWindow is false?
I retested the issue on Nightly 64.0a1 (2018-09-13) and Beta 63.0b6, Windows 10, 8.1 and 7 with dpi set to 125 and 150% with these flows: 1. re-position window on main and secondary screen 2. change the size of the window 3. re-position and change the side of the window 4. change the size of the window and maximize, restart the browser and minimize on main and secondary screens 5. position the window between the screens 6. position with a part outside the screen 7. all above with 100% dpi I managed to reproduce the issue on Beta 63.0b5. Point 6. and 7. behave the same in case browser.startup.blankWindow is set to false (the window is re-positioned to the margin of the screen) Regarding the above information, I will mark the issue as verified on 63 and 64.
Flags: needinfo?(pascalc)
Flags: needinfo?(brindusa.tot)
(In reply to David Olah from comment #17) > I retested the issue on Nightly 64.0a1 (2018-09-13) and Beta 63.0b6, Windows > 10, 8.1 and 7 with dpi set to 125 and 150% Thanks!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
should we consider this for a ridealong for a 62 dot release as well? it's an issue that came up a number of times in support: https://support.mozilla.org/en-US/questions/firefox?owner=all&tagged=bug1489214&show=all
(In reply to [:philipp] from comment #19) > should we consider this for a ridealong for a 62 dot release as well? > it's an issue that came up a number of times in support: > https://support.mozilla.org/en-US/questions/ > firefox?owner=all&tagged=bug1489214&show=all Florian what do you think?
Flags: needinfo?(florian)
(In reply to Julien Cristau [:jcristau] from comment #20) > (In reply to [:philipp] from comment #19) > > should we consider this for a ridealong for a 62 dot release as well? > > it's an issue that came up a number of times in support: > > https://support.mozilla.org/en-US/questions/ > > firefox?owner=all&tagged=bug1489214&show=all > > Florian what do you think? If we have enough time to do QA before shipping, I think both this patch and the patch from bug 1489852 would make sense to ship to release sooner than later. That's optional though, I don't think they should be part of deciding if we want to do a dot release or not.
Flags: needinfo?(florian)
Comment on attachment 9008418 [details] [diff] [review] Patch approved for 62.0.2, based on comment 21.
Attachment #9008418 - Flags: approval-mozilla-release+
Flags: qe-verify+
I verified the issue on Firefox 62.0.2, Windows 7/10 and it is fixed. I reteasted based on the same steps as noted in comment 17. Marking it as verified.
Flags: qe-verify+
Depends on: 1493472
No longer depends on: 1493472
Regressions: 1493472
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: