If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 47

Status

()

Core
Widget: Win32
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: jfkthame)

Tracking

({regression})

46 Branch
mozilla48
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 unaffected, firefox47 fixed, firefox48 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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?
(Assignee)

Comment 1

2 years ago
(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
(Reporter)

Comment 2

2 years ago
(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.
(Assignee)

Comment 3

2 years ago
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)

Comment 4

2 years ago
Created 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
(Assignee)

Updated

2 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Attachment #8725438 - Flags: review?(neil)
(Assignee)

Comment 5

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

Updated

2 years ago
Attachment #8725438 - Flags: review?(neil) → review?(VYV03354)
Attachment #8725438 - Flags: review?(VYV03354) → review+
(Assignee)

Comment 6

2 years ago
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
(Assignee)

Comment 7

2 years ago
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?

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c254f951e0ad
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Comment 9

2 years ago
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.
status-firefox46: affected → unaffected
status-firefox47: --- → affected
I'd like this to stabilize in Nightly for a bit before uplifting to Aurora. Will review uplift again next week.
(Assignee)

Updated

2 years ago
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+
(Assignee)

Comment 12

2 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/306fbf9bddda
status-firefox47: affected → fixed
You need to log in before you can comment on or make changes to this bug.