Closed Bug 296004 Opened 19 years ago Closed 19 years ago

Context menu popup of a menuitem element opens at the wrong position

Categories

(Core :: XUL, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: alex, Assigned: jsfbbz)

References

Details

(Keywords: regression)

Attachments

(2 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050523 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050523 Firefox/1.0+

When right-clicking on an element that has a context menu, the context menu
popup should appear with its top left corner at the current mouse position. When
doing so on a menuitem (like a bookmark in the Bookmarks menu), the popup window
opens at the wrong position - it is higher than it should be, well above the
current mouse position. Patricularly noticable when the context menu has only
one item, in which case the whole popup is above the mouse position.

Reproducible: Always
Can reproduce easily in "Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.8b2) Gecko/20050531 Firefox/1.0+". Also, bz tried and couldn't reproduce on
Linux, so is probably Windows-only.
The menu appears to be offset by exactly the non-client metrics of the left and
top (yes, the meny appears too far left as well) of the main window.
So.. this seems to be windows-only.  I can't reproduce on Linux, people on IRC
can reproduce on Windows.  The popup is offset by the size of the window
decorations (so titlebar + frame vertically and frame horizontally).

Having some idea of regression range here would be really nice...
Keywords: qawanted
Update thanks to gavin: Regressed between 2004-05-12 and 2004-05-14, bug 135079
is suspected (branch fix didn't regress, trunk fix wasn't the same and did, so
it seems kinda likely.
John, ere, any idea whether the patch for bug 135079 could cause coordinates to
be off by the size of the window decorations?
Fwiw, the trunk and branch diffs in bug 135079 look identical; just have the
files in a different order.
-          RECT r;
-          VERIFY(::GetWindowRect(hWnd, &r));
+          RECT r;
+          VERIFY(::GetClientRect(hWnd, &r));

This change is the only suspicious thing I can find and even then I don't see
how it could cause this problem as it's just a check to see if the mouse is in
the window area. Anyway, someone could try reversing that and in addition
removing the call ::ScreenToClient(hWnd, &cpos); to see if it makes any
difference. I can't do it right now.
(In reply to comment #5)
> John, ere, any idea whether the patch for bug 135079 could cause coordinates to
> be off by the size of the window decorations?

I wouldn't have thought so. FWIW the context menu appears in the right location
in Firefox 0.9 which is post that patch (as the issues I addressed are gone too).

If there is a mistake there, then the popup would tend to be off, not by the
window decoration offset (which is the difference between the client area and
window area) but by the difference between the top left of either the client
area or window rectangle and the desktop 0,0 coordinate. This is easily checked
by moving the browser window so that its top-left is in the middle of the screen
and seeing whether the popup is still offset by the window decoration size or by
the much larger 0,0(desktop)->0,0(window) distance now present. I suspect the
former.

What *can* cause this error is the following: the popup's top-left must be
specified in screen coordinates since it is a top level window, a child of the
desktop itself. (Right click near the right of the browser window, the menu
falls off the right edge of the window. This proves it can't be a child of the
browser window.) The triggering mouse event however delivers coordinates in
*client* coordinates of the browser window. These are relative to the top-left
of the drawable area of the window (minus decorations), not the top-left of the
window frame. If someone has made a change assuming that client coordinates can
be converted to screen coordinates by simply adding the desktop coordinate of
the top-left of the window frame, they would be wrong by exactly the size of the
window decoration:

   POINT mousepos; // initially in client coords
   RECT r;
   ::GetWindowRect(hWnd, &r); // in screen coords
   mousepos += r.topleft; // Wrong!!
   TrackPopupMenu(,, mousepos.x, mousepos.y,,,);

Instead you must always go through the correct Win32 API coordinate transform:

   ::ClientToScreen(hWnd, &mousepos);
   TrackPopupMenu(,, mousepos.x, mousepos.y,,,);

If no one else has enough tuits I can pull an up-to-date source tree and track
this down some time this week. Otherwise looking at where exactly TrackPopupMenu
is getting its coordinates from should reveal the error.
(In reply to comment #8)
> I suspect the former.

It is definately only the decorations it is off by, since my window is
definately not in the top-left of the screen. And from a win32 standpoint, it is
clear what the problem is... but Gecko's widget/view interaction and coordinate
system is really into black-magic territory.
Flags: blocking1.8b3?
Attached image OS/2 screenshot
Bug 295833 indicates that OS/2 has a different coordinate system than win, but
this might be related anyway. It started in today's OS/2 trunk. The last public
OS/2 trunk that worked at all was 2005060115, which has no problem of either
sort. Trying to open bookmarks is useless, as whatever I click is always
different from what loads.
Tracy, could you look into this to try and get good steps to reproduce?
Flags: blocking1.8b4?
Flags: blocking1.8b3?
Flags: blocking1.8b3-
Comment #0 has perfectly good instructions IMHO. You want me to spell it out? :)

1. Make sure your bookmark toolbar is visible, and has at least one non-empty
folder in it.
2. Click to open the folder.
3. Right-click an item in the popup.
4. Observe incorrect positioning of context menu.

It is reproducible every single time, here.
This can be easily reproduced with a bookmark menu item as follows:

-Click Bookmarks
-In the Bookmarks flyout menu, right click on the "Bookmarks Toolbar Folder"
menu item

tested results: The context menu opens with the upper left corner oriented about
a full text line above the pointer and a few pixels left of the pointer.

expected results: The context menu opens with its upper left corner aligned to
the pointer tip.  (just right click any place in a page to see expected context
menu alignment)
Verified same problem for 1.0+.

I tried 1.0.4 first, and can't duplicate there. I can't duplicate this on "an
element" in 1.0+ as the initial report said either.

The issue occurs when right-clicking on an existing popup (so AFAICT a bookmark
folder, whether on the toolbar or from the Bookmarks menu - remembering the
bookmarks menu is itself a folder - is the only place where this occurs.)

So it's either in the popup window implementation or the bookmarks menu
implementation. Investigating...
The problem appears to be related to a bit of coordinate transformation done in
content/events/nsDOMUIEvent.cpp around line 166 nsDOMUIEvent::GetClientPoint().
At the point of failure we are being asked for a point in client coordinates of
some IWidget at which the new popup will appear.

Initially the nsGUIEvent contains a point in widget (client) coordinates of the
IWidget that causes it (see widget/windows/src/nsWindow.cpp DispatchMouseEvent I
guess).

For either the bookmark toolbar or boolmark menu, we find that eventWidget is
eWindowType_popup so nothing is added to this point. docEvent points to some
other IWidget which has a null parent and GetBounds().TopLeft() of, for me, (6,
25) or the window frame thickness. This is subtracted from the point leaving it
to far left and up.

For a right-click within the main browser window, which appears to work fine for
me, I see eventWidget->GetBound().TopLeft() set to (0, 0) and
eventWidget->GetParent()==docEvent therefore nothing is added and the second
pass is skipped.

(This method seems to work, essentially, only when it does nothing!)

This file is new in 1.0+: previously this code was in nsDOMEvent.cpp in the
GetClientX and GetClientY methods. (CVS says nsDOMUIEvent.cpp was created
2004-08-20, but isn't in the 1.0.4 tarball which was, what, 2005-05-11? With
1.0+ itself released 2005-05-23? But the changes I suspect date back to 2004-05
and earlier... It's difficult to tell how these dates interact to figure out
when what code changes hit what release.)

Anyway, I believe the significant change is the addition of the second pass
(subtracting from the coordinate) that was added in nsDOMEvent.cpp revision
1.172 which was to fix bug 242833 - this simply wasn't happening before.

Since there is a clear conflict of interest here perhaps the author of that
revision, Robert O'Callahan (who I see is already CCed) should take another look...
Flags: blocking-aviary1.1?
Keywords: qawantedregression
After a little more thought I think I see what each of the widget pointers I'm
seeing above actually correspond to, and very vaguely why they happen.

The main frame window of Firefox is a top-level HWND (hF). We don't use a
standard HMENU menubar attached to the frame, so everything except the
border/caption area is client area. The frame has precisely one child (hM)
occupying that region - whose own client area draws the
menu/toolbar/statusbar/tabbar areas. Within that is a set of 4 nested HWNDs (hD1
contains hD2 ... hD4) for each document tab. The outer two don't appear to
visibly do much, hD3 implements the scrollbars and hD4 is the main document
canvas. (hD1-hD3 are all exactly the same size.)

When clicking on something in the bookmarks menu, the menu itself is a top-level
HWND (hP) which is what we see wrapped in nsDOMUIEvent::GetClientPoint() as the
initial value of eventWidget. docWidget is initially set to hM here. GetBounds()
works out the offset from the HWND parent hF, however if hF is wrapped in a
widget, it's not actually chained into the hierarchy. (The Get(Foo)Bounds()
functions work directly with the Win32 API on the wrapped HWND, ignoring any
surrounding (if any) nsIWidgets.)

When clicking in the document, docWidget and eventWidget are presumably hD3 and
hD4 respectively.

One thing that I see is that GetClientPoint() uses one method
(GetPresShell()->GetViewManager()->GetWidget()) to determine the coordinate
system to translate into. I believe it gets this wrong by assuming that i)
eventWidget and the resulting docWidget are hierarchically related in some way
which they are not in this case; ii) that the above method returns something
sensible for clicks in global context such as toolbars and menus, rather than
document context; iii) that whatever later consumes the event is expecting
coordinates in the target coordinate system it has just attempted to choose anyway.

Compare with nsMenuPopupFrame.cpp nsMenuPopupFrame::SyncViewWithFrame() which is
I think what later converts the widget coordinates into screen coordinates that
the new popup can use. It goes through a lengthy process, but it doesn't use the
calls above and I bet it boils down to assuming coordinates relative to hP.
John, one problem is that clientX/clientY are defined to be relative to the
document widget or popup widget, whichever comes first when going up the view
hierarchy from the place where the event happened.  This is a silly setup, but
we're stuck with it for 1.8... :(
Well, in this case it's getting it wrong then. It should be leaving it relative
to the popup widget as it is, but instead it's picking some arbitrary
non-document widget which just happens to be the child of the mainframe and
failing to correctly translate to that. Somehow it needs to notice this case and
stop...

(One possibility is I suppose that GetClientX/Y were never designed to be
applicable to global menu/toolbar events but they've been shoehorned in as the
only game in town?)

I presume that at least some doc generated events can also have eventWidget
equal to or descended from an eWindowType_popup so that wouldn't be a sufficient
test. What about a prior check for a common ancestor to both eventWidget and
docWidget?
> Well, in this case it's getting it wrong then

Right.  The code as written is pretty bogus, as you noticed.  Perhaps the best
approach would be clearly have two widgets around -- the original eventWidget
and the "target widget", and convert coords from the coord system of the former
to that of the latter by doing WidgetToScreen() on both?  It'd be slower than
walking the widget chain to the document for the simple non-popup case, but...
I believe roc stated on the other bug that going directly through screen coords
was undesirable because the performance was particularly bad under X
(remembering that this function gets called a lot during mouse moves, whereas
the SyncViewWithFrame code which does it that way is called much less often). I
personally think it would be the cleanest implementation but practicalities must...

In any case it still doesn't solve this bug as in this particular case we don't
want to do any translation at all - the desired "target coordinate space" is the
one belonging to the parent popup widget, which we're already in. That's what
the later popup creating code seems to assume.

(One of the things I tried yesterday was switching to a very simple
WidgetToScreen/ScreenToWidget implementation. The menu appears halfway across
the screen then!) 
Yeah, ok.  Doing screen coords does have performance issues.  Do you think you
could write up some logic that would do the right thing here? You've been
looking at this code enough that I think you know it as well as anyone else at
this point...
(Urk, that doesn't sound like a good thing to me... oh well.)

Try this. It fixes this bug 296004, and appears not to regress bug 242833.
There are almost certainly other patches in-flight which it could potentially
interfere with though...

It's a hack. It uses fine details which AFAICT are accidental to determine
which of two behaviours it should have. It does nothing but mask the bad
assumption that immediately follows it, nor does it fix the fundamental problem
causing the bug that different areas of the codebase have different ideas about
what coordinates space refPoint should be in. However, in this limited case, it
fixes the bug. If accepted now, it's going to have to be revisited in the
future.
While we don't want to be getting screen coordinates too often, I think it's
fine to get screen coordinates once per user event or once per window popup.
Flags: blocking-aviary1.1?
Blocks: 300975
Attachment #188906 - Flags: superreview?(roc)
Attachment #188906 - Flags: review?(roc)
Comment on attachment 188906 [details] [diff] [review]
work around mismatched coordinate assumptions

+    if (nsnull==t) break;

Write this
+    if (!t)
+      break;
Attachment #188906 - Flags: superreview?(roc)
Attachment #188906 - Flags: superreview+
Attachment #188906 - Flags: review?(roc)
Attachment #188906 - Flags: review+
Comment on attachment 188906 [details] [diff] [review]
work around mismatched coordinate assumptions

Requesting 1.8b4 approval.  Risk is medium, but I gather this is a big problem
on Windows due to all the use of context menus on popups in Firefox...

John, if you could say what testing you've done, that would probably help
drivers make a decision here.
Attachment #188906 - Flags: approval1.8b4?
Attachment #188906 - Flags: approval1.8b4? → approval1.8b4+
Assignee: nobody → jsfbbz
Patch checked in.  John, thanks for fixing this!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Flags: blocking1.8b4?
Verified fixed with Windows DP build 2005-07-28-08-trunk
Status: RESOLVED → VERIFIED
Depends on: 304262
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: