Steps to reproduce: 1. Launch Firefox or Thunderbird 2. Use the mouse to open the View and hover the mouse pointer over one of the items in that menu (e.g. over "Reload" in FF). 3. Leave the mouse pointer where it is, switch to the keyboard, and use Up/Down Arrow to move focus to Toolbars. 4. Use Right Arrow to open the Toolbars menu. Expected results: The Toolbars menu would open and remain open. Actual results: The Toolbars menu opens but immediate collapses. Marking this bug as an accessibility issue because users who are blind/visually impaired might not know where their mouse pointer happens to be resting; they simply won't be able to access certain submenus for some seemingly inexplicable reason.

This WFM with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071208 Minefield/3.0a7pre

Yup, I can only reproduce this in Linux. It WFM on the Mac and in Windows.

It's not a regression. We've this problem since Mozilla. Here's the stack of closing Popup. #0 nsXULPopupManager::HidePopupAfterDelay (this=0x8211ed0, aPopup=0x91e15c4) at nsXULPopupManager.cpp:652 #1 0xb4a1b05d in nsMenuPopupFrame::ChangeMenuItem (this=0x89f4b14, aMenuItem=0x0, aSelectFirstItem=0) at nsMenuPopupFrame.cpp:1378 #2 0xb4a21667 in nsMenuFrame::HandleEvent (this=0x91e4abc, aPresContext=0x8876da8, aEvent=0xbf911320, aEventStatus=0xbf9112bc) at nsMenuFrame.cpp:497 #3 0xb4b2a460 in nsESMEventCB::HandleEvent (this=0xbf91136c, aVisitor=@0xbf9112b0) at nsEventStateManager.cpp:2664 #4 0xb4b3fa7b in nsEventTargetChainItem::HandleEventTargetChain ( this=0x93667b8, aVisitor=@0xbf9112b0, aFlags=6, aCallback=0xbf91136c) at nsEventDispatcher.cpp:303 #5 0xb4b40164 in nsEventDispatcher::Dispatch (aTarget=0x88a7b88, aPresContext=0x8876da8, aEvent=0xbf911320, aDOMEvent=0x0, aEventStatus=0xbf911374, aCallback=0xbf91136c) at nsEventDispatcher.cpp:473 #6 0xb4b23a56 in nsEventStateManager::DispatchMouseEvent (this=0x8877040, aEvent=0xbf911a04, aMessage=332, aTargetContent=0x88a7b88, aRelatedContent=0x0) at nsEventStateManager.cpp:2695 #7 0xb4b23d3d in nsEventStateManager::NotifyMouseOut (this=0x8877040, aEvent=0xbf911a04, aMovingInto=0x0) at nsEventStateManager.cpp:2761 #8 0xb4b241af in nsEventStateManager::GenerateMouseEnterExit (this=0x8877040, aEvent=0xbf911a04) at nsEventStateManager.cpp:2857 #9 0xb4b27103 in nsEventStateManager::PreHandleEvent (this=0x8877040, aPresContext=0x8876da8, aEvent=0xbf911a04, aTargetFrame=0x91e4abc, aStatus=0xbf911878, aView=0x8325ce0) at nsEventStateManager.cpp:703 #10 0xb4811ac3 in PresShell::HandleEventInternal (this=0x8878b48, aEvent=0xbf911a04, aView=0x8325ce0, aStatus=0xbf911878) at nsPresShell.cpp:5721 #11 0xb48126b2 in PresShell::HandlePositionedEvent (this=0x8878b48, aView=0x8325ce0, aTargetFrame=0x91e4abc, aEvent=0xbf911a04, aEventStatus=0xbf911878) at nsPresShell.cpp:5620 #12 0xb4812d1c in PresShell::HandleEvent (this=0x8878b48, aView=0x8325ce0, aEvent=0xbf911a04, aEventStatus=0xbf911878) at nsPresShell.cpp:5463 #13 0xb4cd42e4 in nsViewManager::HandleEvent (this=0x8878860, aView=0x8325ce0, aPoint=@0xbf91192c, aEvent=0xbf911a04, aCaptured=0) at nsViewManager.cpp:1292 #14 0xb4cd89c5 in nsViewManager::DispatchEvent (this=0x8878860, aEvent=0xbf911a04, aStatus=0xbf9119bc) at nsViewManager.cpp:1248 #15 0xb4cce47d in HandleEvent (aEvent=0xbf911a04) at nsView.cpp:168 #16 0xb5b9287f in nsCommonWidget::DispatchEvent (this=0x91d88d0, aEvent=0xbf911a04, aStatus=@0xbf911a50) at nsCommonWidget.cpp:225 #17 0xb5b7c3a1 in nsWindow::OnLeaveNotifyEvent (this=0x91d88d0, aWidget=0x89b8658, aEvent=0x8070de8) at nsWindow.cpp:1922 #18 0xb5b7c438 in leave_notify_event_cb (widget=0x89b8658, event=0x8070de8) at nsWindow.cpp:4359

We do nsMenuPopupFrame::ChangeMenuItem(null, 0) for leave_notify_event / NS_MOUSE_EXIT_SYNTH. This bug only occurs when mouse is on menuitem. Because we file NS_MOUSE_EXIT_SYNTH to last mouse over element. See nsEventStateManager.cpp:2761 2759 // Fire mouseout 2760 DispatchMouseEvent(aEvent, NS_MOUSE_EXIT_SYNTH, 2761 mLastMouseOverElement, aMovingInto); If mouse is not over menu item, we will not get into nsMenuFrame::HandleEvent. Maybe we should not file NS_MOUSE_EXIT_SYNTH to mLastMouseOverElement, if it is triggered by keyboard.

I could not do it in nsWindow.cpp because I need to compare mousePtr with mLastMouseOverElement. Comparing mousePtr with the leaving GdkWindow doesn't work, because when submenu closes, we're leaving submenu window, in this case we don't want to emit MOUSE_EXIT event to mLastMouseOverElement, either. I didn't do any testing on other platform yet.

Comment on attachment 276614 [details] [diff] [review] patch v0 I'm not the best reviewer for this. This needs to get review from roc, and maybe Mats.

My point is that I'm not really qualified to review this code. Please ask mats, ok?

GetScreenRect/WidgetToScreen are slow on Linux, we should avoid using them.

Do the check inside nsWindow.cpp.

Couldn't there be a situation where the mouse enters the window (firing mouse-enter), doesn't move, and then exits the window, in which case this will fail to fire the mouse-exit event? Also, keeping sLastMotionWindow after the window may have died seems like a problem. A new window could be created at the same address in which case this code would become confused.

(In reply to comment #11) > Couldn't there be a situation where the mouse enters the window (firing > mouse-enter), doesn't move, and then exits the window, in which case this will > fail to fire the mouse-exit event? > I think it's more reasonable to have a sLastMouseEnterWindow rather than sLastMotionWindow. Patch is on the way. > Also, keeping sLastMotionWindow after the window may have died seems like a > problem. A new window could be created at the same address in which case this > code would become confused. > Is the window have died, we'll get a leave_notify, and sLast...Window will be cleared in OnLeaveNotifyEvent. Won't we?

I guess so.

+ if (aEvent->window != gdk_window_at_pointer(nsnull, nsnull)) { + return; + } Hmm ... what if event processing is running behind, so the mouse moves through some windows and by the time we process this event it's moved out again? I think this code could get confused; I think you're assuming that gdk_window_at_pointer reflects the state when the event fired, but it doesn't. Also http://osdir.com/ml/gnome.lib.gtk+.devel.apps/2003-04/msg00268.html > It's an astonishingly expensive operation, involving a server > grab and multiple round trips to the X server.

Thanks for your comment! It's helpful.

+ gint width, height; + gdk_window_get_size(aEvent->window, &width, &height); Why don't you just use mBounds instead? + aEvent->x > width || aEvent->y > height) { Should be >=. + // Do not fire MOUSE_EXIT event if the last MOUSE_ENTER event was not for + // the leaving window, or the mouse pointer is still on the leaving window. + // In this case, leave_notify_event is not triggered by mouse. What if this leave-notify is caused by the window being destroyed?

(In reply to comment #17) > + // Do not fire MOUSE_EXIT event if the last MOUSE_ENTER event was not for > + // the leaving window, or the mouse pointer is still on the leaving > window. > + // In this case, leave_notify_event is not triggered by mouse. > > What if this leave-notify is caused by the window being destroyed? > 1) If the window is being destroyed and the mouse is on this window, it will pass our checking. It's same as current behavior. 2) If the window is being destroyed and the mouse is not on this window, we will not fire MOUSE_LEAVE event. (Currently it is being fired) I think it is OK, right? 3) If we enter window A by mouse, then enter window B by keyboard, and then closing window B or re-focusing window A. We will get MOUSE_ENTER for window A twice. Do you think we should avoid it?

> 1) If the window is being destroyed and the mouse is on this window, it will > pass our checking. > It's same as current behavior. No it's not. Currently we will send a leave-notify; with this patch, we won't.

(In reply to comment #19) > > 1) If the window is being destroyed and the mouse is on this window, it will > > pass our checking. > > It's same as current behavior. > > No it's not. Currently we will send a leave-notify; with this patch, we won't. > You mean case 2), right? Are you suggesting we should always send MOUSE_EXIT on destroying window?

roc, I found the MOUSE_EXIT event is not guaranteed on window destroy before the patch. e.g. Tools->Error Console, move mouse into the Error Console window will fire MOUSE_ENTER event, press Ctrl+W to close it will not fire MOUSE_EXIT. So I think we just need to make sure sLastMouseEnterWindow is cleared on window destroy.

Okay. Why not make sLastMouseEnterWindow an nsIWidget? Other than that, looks good.

What about just use nsWindow * for sLastMouseEnterWindow? FWIW: I missed nsWindow.h in patch v4.

Verified fixed with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007091804 Minefield/3.0a8pre

The fix may have caused bug 396567.

And in fact did. Tested by local application of this patch.

Ginn, please back this out if you can't get a fix for the regression today.

patch backed out.

We could not judge whether the mouse pointer is on the window only by (x,y). We should consider z-axis. patch v2 using gdk_window_at_pointer seems don't have this problem.

(In reply to comment #29) > patch v2 using gdk_window_at_pointer seems don't have this problem. > patch v2 still have similar problem. open Firefox menu and click an app outside, we didn't emit mouse_exit for menu window, because last mouse enter window is not menu window. no idea how to fix this bug now.

Patch for nsMenuFrame.cpp Hopefully it is safer. Unlike patch v2-v5, this patch doesn't fix Bug 396869. It is a minor issue, I will propose another patch for it.

Comment on attachment 281635 [details] [diff] [review] patch v6 (nsMenuFrame.cpp) >- else >+ else { >+ nsMenuFrame *realCurrentItem = mMenuParent->GetCurrentMenuItem(); >+ if (realCurrentItem != this) { >+ // we already leave this menuitem by keyboard >+ // ignore MOUSE_EXIT >+ return NS_OK; >+ } >+ > mMenuParent->ChangeMenuItem(nsnull, PR_FALSE); >+ } > } I would just do: if (mMenuParent->GetCurrentMenuItem() == this) mMenuParent->ChangeMenuItem(nsnull, PR_FALSE);

Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9a9pre) Gecko/2007100304 Minefield/3.0a9pre When the pointer is on a menu item, and its submenu is open, then closed using keyboard, the menu item becomes unselected.

(In reply to comment #34) > Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9a9pre) Gecko/2007100304 > Minefield/3.0a9pre > > When the pointer is on a menu item, and its submenu is open, then closed using > keyboard, the menu item becomes unselected. > I didn't see that, but I think bug 396869 can fix that problem. If not, we'd better file another bug to track it.

Aleksej, I've confirmed the problem you reported. (I was putting my mouse pointer on another menu item without submenu when testing this bug.) And I've confirmed Bug 396869 should fix that.