Closed Bug 1242449 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Widget: Win32, defect)

46 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- unaffected
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: neil, Assigned: jfkthame)

References

Details

(Keywords: regression)

Attachments

(1 file)

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?
(In reply to neil@parkwaycc.co.uk 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.
Depends on: 1240085
(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.
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.
Summary: Staggering mechanism moves windows partially off-screen → Staggering mechanism moves windows partially off-screen, or fails to stagger at all
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Attachment #8725438 - Flags: review?(neil)
Here's a try build with the above patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a6f36f7ca1b
Attachment #8725438 - Flags: review?(neil) → review?(VYV03354)
Attachment #8725438 - Flags: review?(VYV03354) → review+
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 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
Attachment #8725438 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c254f951e0ad
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.
I'd like this to stabilize in Nightly for a bit before uplifting to Aurora. Will review uplift again next week.
Blocks: 1255645
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+
Attachment #8725438 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.