1242449 - Staggering mechanism moves windows partially off-screen, or fails to stagger at all

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[[e-mail]]

When you create more than one window of the same window type, nsXULWindow::StaggerPosition tries to ensure that they don't overlap. To do this it has to know the correct size of the window, otherwise it will move windows off-screen. Unfortunately bug 890156 reversed the order of sizing and positioning a window, presumably we are now in a chicken-and-egg position in that we need to know the position to size it correctly but we need to know the size to position it correctly?

Comment 1

[Jonathan Kew [:jfkthame]]

(In reply to [e-mail] from comment #0)
> When you create more than one window of the same window type,
> nsXULWindow::StaggerPosition tries to ensure that they don't overlap. To do
> this it has to know the correct size of the window, otherwise it will move
> windows off-screen. Unfortunately bug 890156 reversed the order of sizing
> and positioning a window, presumably we are now in a chicken-and-egg
> position in that we need to know the position to size it correctly but we
> need to know the size to position it correctly?

Is that the change in nsXULWindow::OnChromeLoaded() you're referring to? IIRC, that was because we need to know the position of the window (or at least which screen it's on) in order to size it properly, in cases (I'm looking at you, Win8.1) where the behavior of window-sizing APIs is dependent on the DPI scaling factor of the screen.

But as you point out, we also need to know the size in order to position it safely within the bounds of the screen. So yes, this sounds rather chicken-and-egg-ish. I suppose we need to do a three-stage process more like:
(a) position the window on the appropriate screen, or at least figure out which screen we're aiming to put it on;
(b) size the window properly, using that screen's resolution;
(c) position it, checking that it is within the bounds of its screen.

Some of this stuff may get touched again in the course of fixing bug 1240085, so I'd like to get that one finished first, and then see where we stand here. Marking depends-on for now, though it may end up needing a separate fix.

Comment 2

[[e-mail]]

(In reply to Jonathan Kew from comment #1)
> Is that the change in nsXULWindow::OnChromeLoaded() you're referring to?
Indeed it is, thanks for following up so quickly.

Comment 3

[Jonathan Kew [:jfkthame]]

I'm also noticing (on Win8.1, but I assume this would be true on any version) that staggering is broken entirely on displays with >100% scaling; successive windows just keep opening at the same position. This suggests to me that StaggerPosition() is suffering from some desktop-pixel vs device-pixel vs CSS-pixel confusion since the coordinate-system changes in bug 890156.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Fix confusion among CSS, desktop and device pixel units in nsXULWindow position/size and window staggering so as to work consistently across mixed resolution displays

Comment 5

[Jonathan Kew [:jfkthame]]

Here's a try build with the above patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a6f36f7ca1b

Comment 6

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c254f951e0ad68f047763f8e2c2fa9385b74a798
Bug 1242449 - Fix confusion among CSS, desktop and device pixel units in nsXULWindow position/size and window staggering so as to work consistently across mixed resolution displays. r=emk

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 8725438 [details] [diff] [review]
Fix confusion among CSS, desktop and device pixel units in nsXULWindow position/size and window staggering so as to work consistently across mixed resolution displays

Looks like this missed the mozilla-47 train; requesting approval for aurora so that we can avoid shipping this regression.

Approval Request Comment
[Feature/regressing bug #]: 890156

[User impact if declined]: Positioning errors when opening new windows -- may fail to stagger (depending on screen resolution), and/or be placed partially off-screen.

[Describe test coverage new/current, TreeHerder]: Tested manually with various single- and multiple-display configurations; I don't think we have good facilities for this in automation.

[Risks and why]: Moderate; this reworks coordinate management in the window placement code, so regressions could manifest as misplaced/mis-sized windows -- but that's what is already broken here, and we're aiming to fix it. Nothing else should be affected.

[String/UUID change made/needed]: n/a

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 9

[Jonathan Kew [:jfkthame]]

Updated status flags: I don't believe this affects 46, because bug 890156 and its followups (the source of the regression) was backed of aurora-46 (see bug 890156 comment 168).

Mozilla-47 is affected, however; hence the uplift request in comment 7 here.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I'd like this to stabilize in Nightly for a bit before uplifting to Aurora. Will review uplift again next week.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8725438 [details] [diff] [review]
Fix confusion among CSS, desktop and device pixel units in nsXULWindow position/size and window staggering so as to work consistently across mixed resolution displays

This code has been in Nightly for over a week now, Aurora47+

Comment 12

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/306fbf9bddda