Closed Bug 524545 Opened 15 years ago Closed 13 years ago

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

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8
Tracking Status
blocking2.0 --- -

People

(Reporter: enndeakin, Assigned: enndeakin)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: assertion, Whiteboard: [patchlove][not-ready-for-cedar])

Attachments

(3 files, 1 obsolete file)

Attached file testcase
This causes an assertion and the popup isn't visible because it is offscreen.
Summary: Popups opened with large margins or using openPopupAtScreen(largeNumber, largeNumber) appear offscreen → A popup opened with large margins appears offscreen
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #408843 - Flags: review?(roc)
http://hg.mozilla.org/mozilla-central/rev/759786bd851a
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Updated patchSplinter Review
Attachment #408843 - Attachment is obsolete: true
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.
blocking2.0: --- → ?
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)
blocking2.0: ? → -
status2.0: --- → ?
Whiteboard: [patchlove]
Attached patch fix the testSplinter Review
This patch just adjusts the offsets in the test a bit. It doesn't change any non-test code from the previously reviewed patch.
Attachment #508507 - Flags: approval2.0?
Depends on: 630684
Blocks: 630684
No longer depends on: 630684
Summary: A popup opened with large margins appears offscreen → A popup opened with large margins appears offscreen ("ASSERTION: Popup is offscreen")
This should land; I'm happy to preserve the approval here. Can we get 'er done?
status2.0: ? → ---
Keywords: checkin-needed
Whiteboard: [patchlove] → [patchlove][needs-landing]
It's not clear which attachment should be landed here.  Please generate a single patch for landing, and mark everything else as obsolete.
Keywords: checkin-needed
Whiteboard: [patchlove][needs-landing] → [patchlove]
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)
Flags: in-testsuite+ → in-testsuite?
This has had approval for most of a week - is it landing soon, or missing FF4?
Attachment #508507 - Flags: approval2.0+
Neil, please land this on mozilla-central if you want to get it in 2.2.
Whiteboard: [patchlove] → [patchlove][not-ready-for-cedar]
Ping for progress.
Depends on: 672912
http://hg.mozilla.org/mozilla-central/rev/d0f042751940
Status: REOPENED → RESOLVED
Closed: 15 years ago13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Flags: in-testsuite? → in-testsuite+
Depends on: 841585
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: