Last Comment Bug 524545 - A popup opened with large margins appears offscreen ("ASSERTION: Popup is offscreen")
: A popup opened with large margins appears offscreen ("ASSERTION: Popup is off...
Status: RESOLVED FIXED
[patchlove][not-ready-for-cedar]
: assertion
Product: Core
Classification: Components
Component: XP Toolkit/Widgets: Menus (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Neil Deakin
:
Mentors:
Depends on: 841585 672912
Blocks: 626563 630684
  Show dependency treegraph
 
Reported: 2009-10-26 13:50 PDT by Neil Deakin
Modified: 2014-11-10 04:47 PST (History)
12 users (show)
enndeakin: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
testcase (407 bytes, application/vnd.mozilla.xul+xml)
2009-10-26 13:50 PDT, Neil Deakin
no flags Details
fix (13.88 KB, patch)
2009-10-28 07:44 PDT, Neil Deakin
roc: review+
Details | Diff | Review
Updated patch (12.82 KB, patch)
2010-12-12 11:12 PST, Neil Deakin
no flags Details | Diff | Review
fix the test (8.62 KB, patch)
2011-01-31 12:55 PST, Neil Deakin
no flags Details | Diff | Review

Description Neil Deakin 2009-10-26 13:50:26 PDT
Created attachment 408462 [details]
testcase

This causes an assertion and the popup isn't visible because it is offscreen.
Comment 1 Neil Deakin 2009-10-28 07:44:48 PDT
Created attachment 408843 [details] [diff] [review]
fix

This patch:
  - after flipping or moving the popup, makes sure the position and size are within the screen (or content area) boundaries.
  - fixes the case clauses so that invalid values of popupAnchor or popupAlign are just treated as topleft. I found this while writing the test and caused it to use random values at times.

Note that this bug only occurs when large margins are used on the popup which push the popup offscreen, or cause its size to be negative.
Comment 3 Daniel Holbert [:dholbert] 2009-11-03 14:04:48 PST
Looks like this checkin caused some orange:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1257284533.1257285163.28678.gz

There are 8 test-failures like this:
8331 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_popup_attribute.xul | open popup with large positive margin y position before_start - got 0, expected 406

And 8 test-failures like this:
9028 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_popup_button.xul | open popup with large positive margin y position before_start - got 0, expected 406

Backing out in a minute, unless Neil responds on IRC or in this bug... :)
Comment 4 Daniel Holbert [:dholbert] 2009-11-03 14:05:57 PST
(In reply to comment #3)
> Backing out in a minute, unless Neil responds on IRC or in this bug... :)

Ah, nevermind -- Neil just backed out:
http://hg.mozilla.org/mozilla-central/rev/ee113c74758e
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-11-09 16:57:27 PST
Looks like Neil relanded:
http://hg.mozilla.org/mozilla-central/rev/7d7f59076479
But the same failures are showing up again on Mac, e.g.:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1257810963.1257811323.26188.gz

I will back out.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-11-09 16:59:56 PST
Done:
http://hg.mozilla.org/mozilla-central/rev/86a93c8c6a87
Comment 7 Neil Deakin 2010-12-12 11:12:41 PST
Created attachment 497168 [details] [diff] [review]
Updated patch
Comment 8 Serge Gautherie (:sgautherie) 2011-01-18 10:09:32 PST
Comment on attachment 497168 [details] [diff] [review]
Updated patch

>+  // Make sure that the point in within the screen boundaries and that the

Nit: s/in/is/
Comment 9 Serge Gautherie (:sgautherie) 2011-01-24 22:46:42 PST
Neil, what is the next step here?
Comment 10 Serge Gautherie (:sgautherie) 2011-01-26 17:51:11 PST
"blocking2.0=?":
Bug 616607 was blocking2.0+.
This is "blocking" work on bug 626563 that causes a perma-orange on SeaMonkey.
Comment 11 Mike Beltzner [:beltzner, not reading bugmail] 2011-01-27 08:38:17 PST
I would happily approve this patch if it comes through, but I don't think it blocks the release (as bug 626563 doesn't either)
Comment 12 Neil Deakin 2011-01-31 12:55:18 PST
Created attachment 508507 [details] [diff] [review]
fix the test

This patch just adjusts the offsets in the test a bit. It doesn't change any non-test code from the previously reviewed patch.
Comment 13 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-23 10:18:47 PST
This should land; I'm happy to preserve the approval here. Can we get 'er done?
Comment 14 :Ehsan Akhgari (out sick) 2011-02-23 16:23:17 PST
It's not clear which attachment should be landed here.  Please generate a single patch for landing, and mark everything else as obsolete.
Comment 15 Neil Deakin 2011-02-23 18:28:19 PST
None of it should be. Patches in other related bugs were not approved.
Comment 16 Daniel Holbert [:dholbert] 2011-02-23 18:56:36 PST
(also, it looks like there's a stale "in-testsuite+" flag from when this originally landed - I'm toggling that to "?", since I assume the tests were backed out along with the code)
Comment 17 Johnathan Nightingale [:johnath] 2011-02-28 12:24:20 PST
This has had approval for most of a week - is it landing soon, or missing FF4?
Comment 18 :Ehsan Akhgari (out sick) 2011-03-28 14:20:19 PDT
Neil, please land this on mozilla-central if you want to get it in 2.2.
Comment 19 Serge Gautherie (:sgautherie) 2011-04-23 10:55:24 PDT
Ping for progress.
Comment 20 Marco Bonardo [::mak] 2011-08-06 02:55:26 PDT
http://hg.mozilla.org/mozilla-central/rev/d0f042751940

Note You need to log in before you can comment on or make changes to this bug.