Closed Bug 393186 Opened 12 years ago Closed 10 years ago

menupopup appearing in the wrong place

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mossop, Assigned: enndeakin)

References

Details

Attachments

(1 file)

I have a somewhat odd multimonitor setup:

+-------+
|       |
|   1   |
|       |
+-------+
      +-------+
      |       |
      |   2   |
      |       |
      +-------+

Now if I have Firefox open on window 1 towards the bottom and I click one of the bookmark toolbar folders I would expect it to open upwards since there isn't visible room below. Instead it appears at the top left corner of monitor 2.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007082204 Minefield/3.0a8pre ID:2007082204
This is because widgets always appear on the same screen as the parent window. If the parent is across two screens, as in this case, the screen that the parent window is a greater percentage over is used.

nsIWidget would need to provide a means of modifying the screen to use so we can call it in nsMenuPopupFrame::SetPopupPosition, as we would rather have the popup appear on the screen containing the anchor or mouse.

(In reply to comment #1)
> This is because widgets always appear on the same screen as the parent window.
> If the parent is across two screens, as in this case, the screen that the
> parent window is a greater percentage over is used.

That isn't the issue, Firefox is open on screen 1 only. Yet the popup appears on screen 2.
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
The comments here seem conflicting. In the first comment you say you have Firefox open on window 1 at the bottom (so it wraps over the screen?), but in comment 2 you say it is only on screen 1.

Regardless, I think the issue is still as described in my comment, that the popup should always appear on the screen where the mouse or anchor is.
(In reply to comment #3)
> The comments here seem conflicting. In the first comment you say you have
> Firefox open on window 1 at the bottom (so it wraps over the screen?), but in
> comment 2 you say it is only on screen 1.

No, I said it was open towards the bottom of screen 1, it is only visible on that screen.

> Regardless, I think the issue is still as described in my comment, that the
> popup should always appear on the screen where the mouse or anchor is.

I agree.
Dave, does the fix for bug 482928 improve this?
(In reply to comment #5)
> Dave, does the fix for bug 482928 improve this?

Still seeing the problem with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090414 Minefield/3.6a1pre ID:20090414030735
This patch changes the screen retrieval so that the screen for the point where the anchor is located is used instead of the one where the window is located. For menus that open at unanchored screen locations, it uses that point instead.

I can't reproduce the bug exactly, but this seems to improve popup positioning where there are multiple screens such that I suspect it should fix the problem here.

Dave, can you test this patch?
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
(In reply to comment #7)
> Created an attachment (id=373893) [details]
> Dave, can you test this patch?

Yes, the patch fixes the problem for me.
Comment on attachment 373893 [details] [diff] [review]
use anchor point instead

Not sure how to test this, as it requires multiple screens to be connected.
Attachment #373893 - Flags: superreview?(roc)
Attachment #373893 - Flags: review?(roc)
Comment on attachment 373893 [details] [diff] [review]
use anchor point instead

+    // for context shells, get the screen where the root content is located.

typo "context"

+    if (mInContentShell)
+      sm->ScreenForRect(presContext->AppUnitsToDevPixels(rootScreenRect.x),
+                        presContext->AppUnitsToDevPixels(rootScreenRect.y),
+                        1, 1, getter_AddRefs(screen));
+    else
+      sm->ScreenForRect(presContext->AppUnitsToDevPixels(anchorRect.x),
+                        presContext->AppUnitsToDevPixels(anchorRect.y),
+                        1, 1, getter_AddRefs(screen));

Prefer to have single non-control-flow statements in {}. But here I think we should set a local variable nsPoint to rootScreenRect.TopLeft or anchorRect.TopLeft and then have a single call to ScreenForRect.
Attachment #373893 - Flags: superreview?(roc)
Attachment #373893 - Flags: superreview+
Attachment #373893 - Flags: review?(roc)
Attachment #373893 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Version: unspecified → Trunk
I see this bug on my dual-monitor setup on Linux, too, and the patch that was just checked in does fix it for me.  Platform --> ALL/ALL.
OS: Mac OS X → All
Hardware: x86 → All
Depends on: 445749
You need to log in before you can comment on or make changes to this bug.