393186 - menupopup appearing in the wrong place

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Dave Townsend [:mossop]]

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

Comment 1

[Neil Deakin]

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.

Comment 2

[Dave Townsend [:mossop]]

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

Comment 3

[Neil Deakin]

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.

Comment 4

[Dave Townsend [:mossop]]

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

Comment 5

[Neil Deakin]

Dave, does the fix for bug 482928 improve this?

Comment 6

[Dave Townsend [:mossop]]

(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

Comment 7

[Neil Deakin]

Attachment: use anchor point instead

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?

Comment 8

[Dave Townsend [:mossop]]

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

[Neil Deakin]

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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 11

[Daniel Holbert [:dholbert]]

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.