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

VERIFIED FIXED in Firefox 62

Status

()

defect
VERIFIED FIXED
10 months ago
2 months ago

People

(Reporter: philipp, Assigned: florian)

Tracking

(Regressed 1 bug, {regression})

62 Branch
Firefox 64
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62+ verified, firefox63+ verified, firefox64+ verified)

Details

Attachments

(1 attachment)

Reporter

Description

10 months ago
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.
Reporter

Comment 1

10 months 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.
Blocks: 1336227, 1473142
Flags: needinfo?(florian)
Reporter

Comment 2

10 months 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

9 months 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

9 months ago
Just wanted to add that setting browser.startup.blankWindow to false fixes this issue.
Assignee

Comment 5

9 months 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

9 months ago
Posted patch PatchSplinter 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

Updated

9 months ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
Assignee

Updated

9 months ago
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+
Assignee

Comment 8

9 months 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.

Comment 9

9 months ago
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

9 months ago
I think we should uplift to beta.

Comment 11

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e6c9581ba20e
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Assignee

Comment 12

9 months 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 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+

Comment 15

9 months 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

9 months 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

9 months 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

9 months 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

9 months ago
Flags: qe-verify+
Reporter

Comment 19

9 months 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
(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

9 months 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 on attachment 9008418 [details] [diff] [review]
Patch

approved for 62.0.2, based on comment 21.
Attachment #9008418 - Flags: approval-mozilla-release+

Comment 24

9 months 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

9 months ago
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.