The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla8

Status

()

Core
XP Toolkit/Widgets: Menus
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: Neil Deakin, Assigned: Neil Deakin)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {assertion})

Trunk
mozilla8
assertion
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 -)

Details

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

Attachments

(3 attachments, 1 obsolete attachment)

407 bytes, application/vnd.mozilla.xul+xml
Details
12.82 KB, patch
Details | Diff | Splinter Review
8.62 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
Created attachment 408462 [details]
testcase

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

Updated

8 years ago
Summary: Popups opened with large margins or using openPopupAtScreen(largeNumber, largeNumber) appear offscreen → A popup opened with large margins appears offscreen
(Assignee)

Comment 1

8 years ago
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.
Attachment #408843 - Flags: review?(roc)
Attachment #408843 - Flags: review?(roc) → review+
(Assignee)

Comment 2

8 years ago
http://hg.mozilla.org/mozilla-central/rev/759786bd851a
Status: NEW → RESOLVED
Last Resolved: 8 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 → ---
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.
Done:
http://hg.mozilla.org/mozilla-central/rev/86a93c8c6a87
(Assignee)

Comment 7

6 years ago
Created attachment 497168 [details] [diff] [review]
Updated patch
Attachment #408843 - Attachment is obsolete: true
Blocks: 626563
Keywords: assertion
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]
(Assignee)

Comment 12

6 years ago
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.
Attachment #508507 - Flags: approval2.0?
Attachment #508507 - Flags: approval2.0? → 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]
(Assignee)

Comment 15

6 years ago
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?
(Assignee)

Updated

6 years ago
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.
(Assignee)

Updated

6 years ago
Depends on: 672912
http://hg.mozilla.org/mozilla-central/rev/d0f042751940
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
(Assignee)

Updated

6 years ago
Flags: in-testsuite? → in-testsuite+

Updated

2 years ago
Depends on: 841585
You need to log in before you can comment on or make changes to this bug.