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

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.

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?

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

For what it's worth, bug 242833 is in the regression range mentioned earlier: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-05-12+0%3A00&maxdate=2004-05-15+0%3A00&cvsroot=%2Fcvsroot

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.

Comment on attachment 188906 [details] [diff] [review] work around mismatched coordinate assumptions + if (nsnull==t) break; Write this + if (!t) + break;

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.

Patch checked in. John, thanks for fixing this!

Verified fixed with Windows DP build 2005-07-28-08-trunk