257 bytes, text/html
2.53 KB, patch
|Details | Diff | Splinter Review|
3.00 KB, patch
|Details | Diff | Splinter Review|
1.54 MB, image/png
Component: Untriaged → DOM
Product: Firefox → Core
Version: 28 Branch → Trunk
When I try the attached testcase, I get a window that's about 100px by 100px (the min size we allow) at the bottom right corner of my screen (as in, the coordinates did in fact get clamped as expected). But I'm on Mac. It's possible that the behavior is different on Windows....
And in a 32-bit Mac build it's in the top left, not bottom right, presumably because the coords overflow into negative values.
Kyle, can you reproduce on Windows?
Hmm. Some testing on browserstack suggests we may not in fact clamp to the visible screen area.
It's working well both on windows 7 ultimate and also Windows XP..
Confirmed against Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140303030201 CSet: 4cb766685b73. Regression Window: Last good nightly: 2012-10-17 First bad nightly: 2012-10-18 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dac5700acf8b&tochange=cb573b9307e5 Before this regressed the Popup got opened at 0;0 (top left).
Keywords: regression, testcase
Regression window(m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/e7497bc33b3c Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121016124004 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/eaccb5bb50c0 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121016124404 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e7497bc33b3c&tochange=eaccb5bb50c0 Regressed by: bug 794038
Status: UNCONFIRMED → NEW
status-firefox27: --- → affected
status-firefox28: --- → affected
status-firefox29: --- → affected
status-firefox30: --- → affected
status-firefox-esr24: --- → affected
Ever confirmed: true
Version: Trunk → 18 Branch
I assume comment 6 covers what you wanted from me.
Yes, it does. Jonathan, could you take a look, please?
The specified |left| gets clamped to the range of integers by CalcSizeSpec (via use of atoi()), but then the coordinate computation |left + winWidth| overflows here: http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#2024 (or, similarly, the computation of |top + winHeight| just below). This means we fail to detect that the window is being positioned way off-screen, and so we don't fix up its position.
Created attachment 8389165 [details] [diff] [review] check for overflow in nsWindowWatcher coordinate calculations when constraining position.
Attachment #8389165 - Flags: review?(roc)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8389165 [details] [diff] [review] check for overflow in nsWindowWatcher coordinate calculations when constraining position. Review of attachment 8389165 [details] [diff] [review]: ----------------------------------------------------------------- Seems like it shouldn't be hard to write a mochitest for this
Attachment #8389165 - Flags: review?(roc) → review+
Created attachment 8393575 [details] [diff] [review] test for window opened at extreme off-screen coordinates. Here's a test that fails on Windows with current trunk code, and passes once this bug is fixed. (It doesn't fail on OS X because nsCocoaWindow includes code that pulls the window back on-screen; and similarly on Linux, the system's window manager does that. So AFAICT the bug here is Windows-only in practice, although that's more by chance than design.)
Attachment #8393575 - Flags: review?(roc)
Comment on attachment 8393575 [details] [diff] [review] test for window opened at extreme off-screen coordinates. Review of attachment 8393575 [details] [diff] [review]: ----------------------------------------------------------------- Great thanks
Attachment #8393575 - Flags: review?(roc) → review+
Target Milestone: --- → mozilla31
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
I have 2 monitors connected. No matter on what monitor is the main FF window, the popup is displayed at the bottom right corner of the first monitor and exceed a little the first monitor screen. 31.0a1 (2014-03-24), win 7 x64 Thoughts?
That sounds reasonable. The popup location is unrelated to the main FF window; it's being specified as an absolute location that's far off the bottom/right of all the screens. And so to ensure it's visible, we pull it back onto the default monitor. If it slightly exceeds the screen, that suggests we may have a discrepancy in calculating the dimensions of the window chrome, or the inner/outer window dimensions, but that's pretty minor. (AFAICT, prior to bug 794038, it was possible that only a narrow edge of the window would be visible, and the bulk of it off-screen, so we're doing better than that.) Is it only part of the window chrome that exceeds the screen, or does the actual content area spill over as well?
Created attachment 8395703 [details] dual display.png (In reply to Jonathan Kew (:jfkthame) from comment #18) > (AFAICT, prior to bug 794038, it > was possible that only a narrow edge of the window would be visible, and the > bulk of it off-screen, so we're doing better than that.) Agreed > Is it only part of the window chrome that exceeds the screen, or does the > actual content area spill over as well? See attached. The visible part is the same if using only one monitor.
If this gets uplifted to aurora/beta please make sure we get bug 986296 as well.
status-firefox27: affected → wontfix
status-firefox28: affected → wontfix
status-firefox31: --- → fixed
(In reply to Jonathan Kew (:jfkthame) from comment #18) > That sounds reasonable. Marking this bug as verified on FF 31. If you think the issue in comment 19 should be filed separately please say.
Status: RESOLVED → VERIFIED
status-firefox31: fixed → verified
It looks like we're failing to take the thickness of the window border (chrome) into account when horizontally constraining the window to the screen. I'd suggest filing a separate bug on this - it's a minor polish issue rather than a serious bug, but it'd be nice to fix it some day.
This will be in the next ESR (31), wontfixing for ESR24.
status-firefox-esr24: affected → wontfix
status-firefox29: affected → wontfix
status-firefox30: affected → wontfix
You need to log in before you can comment on or make changes to this bug.