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)
Tracking
()
VERIFIED
FIXED
Firefox 64
People
(Reporter: philipp, Assigned: florian)
References
(Regressed 1 open bug)
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
patch
|
mconley
:
review+
pascalc
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
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.
Updated•6 years ago
|
tracking-firefox62:
--- → +
Reporter | ||
Comment 1•6 years ago
|
||
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.
Reporter | ||
Comment 2•6 years ago
|
||
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...
Comment 3•6 years ago
|
||
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.
Comment 4•6 years ago
|
||
Just wanted to add that setting browser.startup.blankWindow to false fixes this issue.
Assignee | ||
Comment 5•6 years ago
|
||
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
Assignee | ||
Comment 6•6 years ago
|
||
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 | ||
Updated•6 years ago
|
Assignee: nobody → florian
Status: NEW → ASSIGNED
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(florian)
Comment 7•6 years ago
|
||
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+
Assignee | ||
Comment 8•6 years ago
|
||
(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.
Assignee | ||
Comment 10•6 years ago
|
||
I think we should uplift to beta.
Comment 11•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Assignee | ||
Comment 12•6 years ago
|
||
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 13•6 years ago
|
||
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+
Updated•6 years ago
|
Comment 14•6 years ago
|
||
bugherder uplift |
Comment 15•6 years ago
|
||
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)
Assignee | ||
Comment 16•6 years ago
|
||
(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?
Comment 17•6 years ago
|
||
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)
Assignee | ||
Comment 18•6 years ago
|
||
(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
Updated•6 years ago
|
Flags: qe-verify+
Reporter | ||
Comment 19•6 years ago
|
||
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
Comment 20•6 years ago
|
||
(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)
Assignee | ||
Comment 21•6 years ago
|
||
(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 22•6 years ago
|
||
Comment on attachment 9008418 [details] [diff] [review]
Patch
approved for 62.0.2, based on comment 21.
Attachment #9008418 -
Flags: approval-mozilla-release+
Comment 23•6 years ago
|
||
bugherder uplift |
Updated•6 years ago
|
Flags: qe-verify+
Comment 24•6 years ago
|
||
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+
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•