387990 - [a11y] mouse pointer position can prevent keyboard access to submenus

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Joanmarie Diggs]

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.

Comment 1

[Ria Klaassen (not reading all bugmail)]

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

Comment 2

[Joanmarie Diggs]

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

Comment 4

[Ginn Chen]

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

Comment 5

[Ginn Chen]

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.

Comment 6

[Ginn Chen]

Attachment: patch v0

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 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

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

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

Comment 10

[Ginn Chen]

Attachment: patch

Do the check inside nsWindow.cpp.

Comment 11

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

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.

Comment 12

[Ginn Chen]

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

Comment 13

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

I guess so.

Comment 14

[Ginn Chen]

Attachment: patch v2

Comment 15

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

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

Comment 16

[Ginn Chen]

Attachment: patch v3

Thanks for your comment!
It's helpful.

Comment 17

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

+    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?

Comment 18

[Ginn Chen]

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

Comment 19

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

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

Comment 20

[Ginn Chen]

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

Comment 21

[Ginn Chen]

Attachment: patch v4

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.

Comment 22

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

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

Comment 23

[Ginn Chen]

Attachment: patch v5

What about just use nsWindow * for sLastMouseEnterWindow?

FWIW: I missed nsWindow.h in patch v4.

Comment 24

[[:Aleksej]]

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

Comment 25

[[:Aleksej]]

The fix may have caused bug 396567.

Comment 26

[Boris Zbarsky [:bzbarsky]]

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

Comment 27

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

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

Comment 28

[Ginn Chen]

patch backed out.

Comment 29

[Ginn Chen]

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.

Comment 30

[Ginn Chen]

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

Comment 31

[Ginn Chen]

Attachment: patch v6 (nsMenuFrame.cpp)

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 32

[Neil Deakin]

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);

Comment 33

[Ginn Chen]

Attachment: patch v 6.1

Comment 34

[[:Aleksej]]

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.

Comment 35

[Ginn Chen]

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

Comment 36

[Ginn Chen]

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.