1276516 - session restore incorrectly determines window to be off-screen in Win 10, leading to unwanted re-positioning

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Simon]

The way window borders work has changed in Windows 10, thus the assumptions and calculations in bug 864107 are not valid on this OS, leading to undesired 'corrections' of the window position.
Specifically, it unnecessarily moves windows away from the edges of the screen.

STR:
1. 'Snap' Firefox to the left side of the screen (WIN+left arrow).
2. Resize horizontally, to make it remember position. Window is perfectly aligned to the edges.
4. Restart Firefox, now you got silly gaps at the edges.

It stems from the fact, I think, that the window border was reduced to 1px with an invisible padding, so you could still easily grab it. Which would be fine for the most part, except the padding seems to be included in window coordinate calculations. So a window at the edge of the screen actually starts at minus the value of whatever the padding is and extends accordingly on the other side. Simply checking whether the coordinates fall outside the resolution produces wrong results in this case. And is now something that goes against the assumption in bug 864107, comment 13, that having windows partly off-screen would not be a widely desired feature. Because, obviously, if you go through the trouble of putting your window at the edge of the screen, you want it to bloody stay there.

Note: The padding is adjustable in HKEY_CURRENT_USER\Control Panel\Desktop\WindowMetrics (Padded)BorderWidth but even setting it to 0 produces gaps:

From sessionstore:
snapped, correct state before:
"height":1051,"screenX":-1 http://i.imgur.com/zLqppCM.png
and bad state after restart:
"height":1050,"screenX":0 http://i.imgur.com/DQyiTla.png

At the default values, the size of the gaps produced are completely bonkers: http://i.imgur.com/u4MBAR0.jpg
"height":1045,"screenX":-5,"screenY":0

"height":1040,"screenX":0,"screenY":0
Not sure why it doesn't bum out on the negative side of the Y-axis.


Vivaldi gets this right and stays where you put it. Chrome also had the problem with small gaps for a while, lately they do bigger gaps regardless of what's set for padding. Not surprisingly MS is the most confused about changes in their own OS, Edge just starts maximized.
Please do the sane thing here.

Comment 1

[Jonathan Kew [:jfkthame]]

Yes, I can see this would be pretty irritating, if you're using a "snapped" window position and then it gets adjusted away from the edge by session restore.

I toyed with the idea of modifying the coordinates returned by the window APIs (screenX, etc) so that they'd ignore the padding, and similarly the move/resize APIs; but I don't think this is a workable solution for a couple of reasons. First, the Windows APIs that should tell us the border width and padding don't appear to return the correct values -- the padding gets added in to the border width, and returned as a single value; the field that should have the padding remains zero. (This is fixed in the latest Win10 insider builds if we use the new ...ForDpi variant of the API, but that's not available on the current release version.)

Moreover, even once we can reliably get those metrics -- or even if we were to read the registry directly to bypass the current Windows api issues -- the trouble is that for "snapped" windows, we really want to ignore not only the padding but also the border itself at the screen edge (on Win10; not on Win8), otherwise we'd still end up shifting the window inwards by one pixel. But if we make the window's .screen* APIs do that, we're going to get into problems when windows *aren't* snapped to the edge -- code that expects to tile windows on the screen, for example, would end up overlapping their borders in an undesirable way.

And making the window apis vary their behavior (include or exclude the border in the metrics?) depending whether the window is "snapped" to an edge seems really hacky and unexpected, and will almost certainly create further confusion.

So.... I think the better way forward here is to relax the available-screen-area checks in SessionRestore slightly, so that it allows the edges of a restored window to be "slightly" off-screen (for some suitable definition of "slightly") before deciding to override the stored position. It'll still need to override the stored position if the system configuration has changed such that the window would be entirely off-screen, but if it just projects by a few pixels, we could safely leave it alone.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: When restoring window positions from a saved session, allow the window bounds to extend a few pixels beyond the screen edges before deciding to override the saved position and force the window entirely within the screen's visible area

As discussed above, I propose we do something like this. We could perhaps make it Windows-only, but I don't see any real downside to allowing it on all platforms; if people place windows such that they extend slightly off-screen, we may as well leave them that way.

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8759155 [details] [diff] [review]
When restoring window positions from a saved session, allow the window bounds to extend a few pixels beyond the screen edges before deciding to override the saved position and force the window entirely within the screen's visible area

Review of attachment 8759155 [details] [diff] [review]:
-----------------------------------------------------------------

I really like what you did here and the code became more readable as a result due to changing to a rect point model. It also makes me wonder a bit if these kind of 2D plane operations should make more use of Geometry.jsm?
Is it possible to capture this in a unit test?

::: browser/components/sessionstore/SessionStore.jsm
@@ +3488,2 @@
>        }
> +      // Then check the resulting right edge, and reduce it if necessary

nit: missing dot.

@@ +3489,5 @@
> +      // Then check the resulting right edge, and reduce it if necessary
> +      let right = aLeft + aWidth;
> +      if (right > screenRightCss + SCREEN_EDGE_SLOP) {
> +        right = screenRightCss;
> +        // See if we can move the left edge leftwards to maintain width

nit: missing dot.

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> I really like what you did here and the code became more readable as a
> result due to changing to a rect point model. It also makes me wonder a bit
> if these kind of 2D plane operations should make more use of Geometry.jsm?

In general, that sounds good (I wasn't familiar with it); but in this case because of the added "slop" in various of the comparisons, I'm not sure it would really gain much.

> Is it possible to capture this in a unit test?

In theory, I expect so; but it would need to be a test that adapts itself to the size of the available screen(s) on the test system, and then performs session save/restore with appropriate window sizes and positions to exercise the various cases. I don't really know how to go about building such a test. If you think it's worthwhile, maybe file a followup bug and see if there's someone more front-end aware who could tackle it?

Comment 5

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6028e2e8233925b4ae440eb68ece9f5015e5c48b
Bug 1276516 - When restoring window positions from a saved session, allow the window bounds to extend a few pixels beyond the screen edges before deciding to override the saved position and force the window entirely within the screen's visible area. r=mikedeboer

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6028e2e82339