Created attachment 408462 [details] testcase This causes an assertion and the popup isn't visible because it is offscreen.
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.
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... :)
(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
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.
Created attachment 497168 [details] [diff] [review] Updated patch
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/
Neil, what is the next step here?
"blocking2.0=?": Bug 616607 was blocking2.0+. This is "blocking" work on bug 626563 that causes a perma-orange on SeaMonkey.
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)
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.
This should land; I'm happy to preserve the approval here. Can we get 'er done?
It's not clear which attachment should be landed here. Please generate a single patch for landing, and mark everything else as obsolete.
None of it should be. Patches in other related bugs were not approved.
(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)
This has had approval for most of a week - is it landing soon, or missing FF4?
Neil, please land this on mozilla-central if you want to get it in 2.2.
Ping for progress.