524545 - A popup opened with large margins appears offscreen ("ASSERTION: Popup is offscreen")

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

Attachment: testcase

This causes an assertion and the popup isn't visible because it is offscreen.

Comment 1

[Neil Deakin]

Attachment: 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 2

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/759786bd851a

Comment 3

[Daniel Holbert [:dholbert]]

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]]

(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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

Done:
http://hg.mozilla.org/mozilla-central/rev/86a93c8c6a87

Comment 7

[Neil Deakin]

Attachment: Updated patch

Comment 8

[Serge Gautherie (:sgautherie)]

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)]

Neil, what is the next step here?

Comment 10

[Serge Gautherie (:sgautherie)]

"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]]

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]

Attachment: 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]]

This should land; I'm happy to preserve the approval here. Can we get 'er done?

Comment 14

[(no longer active)]

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]

None of it should be. Patches in other related bugs were not approved.

Comment 16

[Daniel Holbert [:dholbert]]

(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]]

This has had approval for most of a week - is it landing soon, or missing FF4?

Comment 18

[(no longer active)]

Neil, please land this on mozilla-central if you want to get it in 2.2.

Comment 19

[Serge Gautherie (:sgautherie)]

Ping for progress.

Comment 20

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/d0f042751940