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

RESOLVED FIXED

Status

()

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: jdiggs, Assigned: ginnchen+exoracle)

Tracking

(Blocks: 1 bug, {access})

Trunk
x86
Linux
access
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

11 years ago
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
(Reporter)

Comment 2

11 years ago
Yup, I can only reproduce this in Linux.  It WFM on the Mac and in Windows.
(Assignee)

Updated

11 years ago
Duplicate of this bug: 372778

Updated

11 years ago
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
(Assignee)

Comment 4

11 years ago
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
(Assignee)

Comment 5

11 years ago
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.
(Assignee)

Comment 6

11 years ago
Created attachment 276614 [details] [diff] [review]
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.
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.
(Assignee)

Updated

11 years ago
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.
(Assignee)

Updated

11 years ago
Attachment #276614 - Flags: review?(roc)
Attachment #276614 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

11 years ago
Created attachment 277363 [details] [diff] [review]
patch

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

Comment 12

11 years ago
(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?
(Assignee)

Comment 14

11 years ago
Created attachment 277860 [details] [diff] [review]
patch v2
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.
(Assignee)

Comment 16

11 years ago
Created attachment 278017 [details] [diff] [review]
patch v3

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

Comment 18

11 years ago
(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.
(Assignee)

Comment 20

11 years ago
(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?
(Assignee)

Comment 21

11 years ago
Created attachment 278383 [details] [diff] [review]
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.
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.
(Assignee)

Comment 23

11 years ago
Created attachment 280702 [details] [diff] [review]
patch v5

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+
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 24

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

Comment 25

11 years ago
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.
(Assignee)

Comment 28

11 years ago
patch backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 29

11 years ago
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.
(Assignee)

Updated

11 years ago
Attachment #280702 - Attachment is obsolete: true
(Assignee)

Comment 30

11 years ago
(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.
(Assignee)

Comment 31

11 years ago
Created attachment 281635 [details] [diff] [review]
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.
Attachment #281635 - Flags: review?(enndeakin)

Comment 32

11 years ago
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);
(Assignee)

Comment 33

11 years ago
Created attachment 281787 [details] [diff] [review]
patch v 6.1
Attachment #281635 - Attachment is obsolete: true
Attachment #281787 - Flags: review?(enndeakin)
Attachment #281635 - Flags: review?(enndeakin)

Updated

11 years ago
Attachment #281787 - Flags: review?(enndeakin) → review+
(Assignee)

Updated

11 years ago
Attachment #281787 - Flags: superreview?(roc)
Attachment #281787 - Flags: superreview?(roc) → superreview+
(Assignee)

Updated

11 years ago
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 34

11 years ago
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.
(Assignee)

Comment 35

11 years ago
(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.
(Assignee)

Comment 36

11 years ago
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.

Updated

10 years ago
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.