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

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[[:philipp]]

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.

Comment 1

[[:philipp]]

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.

Comment 2

[[:philipp]]

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

[mr_whassup]

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

[mr_whassup]

Just wanted to add that setting browser.startup.blankWindow to false fixes this issue.

Comment 5

[Florian Quèze [:florian]]

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%.

Comment 6

[Florian Quèze [:florian]]

Attachment: Patch

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.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

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)

Comment 8

[Florian Quèze [:florian]]

(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

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 10

[Florian Quèze [:florian]]

I think we should uplift to beta.

Comment 11

[Daniel Varga [:dvarga]]

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

Comment 12

[Florian Quèze [:florian]]

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.

Comment 13

[Pascal Chevrel:pascalc (PTO until April 26)]

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.

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c954d9ac0d5c

Comment 15

[David Olah]

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?

Comment 16

[Florian Quèze [:florian]]

(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

[David Olah]

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.

Comment 18

[Florian Quèze [:florian]]

(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!

Comment 19

[[:philipp]]

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

[Julien Cristau [:jcristau] (back April 22)]

(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?

Comment 21

[Florian Quèze [: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.

Comment 22

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9008418 [details] [diff] [review]
Patch

approved for 62.0.2, based on comment 21.

Comment 23

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-release/rev/d835a5290ae3

Comment 24

[David Olah]

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.