Closed Bug 387990 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: XUL, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jdiggs, Assigned: ginnchen+exoracle)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 7 obsolete files)

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.
Duplicate of this bug: 372778
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
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.
Attached patch patch v0 (obsolete) — Splinter Review
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.
Attachment #276614 - Flags: review?(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.
Attachment #276614 - Flags: review?(roc)
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.
Attachment #276614 - Flags: review?(roc)
Attachment #276614 - Flags: review?(bzbarsky)
Attached patch patch (obsolete) — Splinter Review
Do the check inside nsWindow.cpp.
Assignee: nobody → ginn.chen
Attachment #276614 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #277363 - Flags: review?(roc)
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?
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #277363 - Attachment is obsolete: true
Attachment #277860 - Flags: review?(roc)
Attachment #277363 - Flags: review?(roc)
+    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.
Attached patch patch v3 (obsolete) — Splinter Review
Thanks for your comment!
It's helpful.
Attachment #277860 - Attachment is obsolete: true
Attachment #278017 - Flags: review?(roc)
Attachment #277860 - Flags: review?(roc)
+    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?
Attached patch patch v4 (obsolete) — Splinter Review
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.
Attachment #278017 - Attachment is obsolete: true
Attachment #278383 - Flags: review?(roc)
Attachment #278017 - Flags: review?(roc)
Okay. Why not make sLastMouseEnterWindow an nsIWidget? Other than that, looks good.
Attached patch patch v5 (obsolete) — Splinter Review
What about just use nsWindow * for sLastMouseEnterWindow?

FWIW: I missed nsWindow.h in patch v4.
Attachment #278383 - Attachment is obsolete: true
Attachment #280702 - Flags: review?(roc)
Attachment #278383 - Flags: review?(roc)
Attachment #280702 - Flags: superreview+
Attachment #280702 - Flags: review?(roc)
Attachment #280702 - Flags: review+
Attachment #280702 - Flags: approval1.9+
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.
Depends on: 396567
Ginn, please back this out if you can't get a fix for the regression today.
patch backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #280702 - Attachment is obsolete: true
(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.
Attached patch patch v6 (nsMenuFrame.cpp) (obsolete) — Splinter Review
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.
Attachment #281635 - Flags: review?(enndeakin)
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);
Attached patch patch v 6.1Splinter Review
Attachment #281635 - Attachment is obsolete: true
Attachment #281787 - Flags: review?(enndeakin)
Attachment #281635 - Flags: review?(enndeakin)
Attachment #281787 - Flags: review?(enndeakin) → review+
Attachment #281787 - Flags: superreview?(roc)
Attachment #281787 - Flags: superreview?(roc) → superreview+
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
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.
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.