395400 - OBJ_FOCUS events no longer fired in context menus (effective August 31, 2007)

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[Marco Zehe (:MarcoZ)]

Inside context menus in Firefox and Thunderbird, OBJ_FOCUS events are no longer fired when the active menu item changes. The order of events is:
SYS_MENUPOPUPSTART
OBJ_FOCUS on the document
OBJ_FOCUS on the document
SYS_MENUPOPUPEND

Note that the OBJ_FOCUS on the document is really fired twice, no typo. While recording the events, I went from top to bottom of the context menu of a link, and JAWS would not speak a single menu item.
The August 30 build (the one where accessibility was broken alltogether), this particular issue was not yet present. It got introduced with the August 31, 2007 trunk builds.

Comment 1

[Marco Zehe (:MarcoZ)]

Note that the normal menu system does work properly. This is only a problem in context menus (for example when right-clicking any link in Firefox, or a message in Thunderbird).

Comment 2

[Joanmarie Diggs]

Changing this from Windows XP to All.  It's confirmed in Linux.

Comment 3

[Aaron Leventhal]

When a context menu opens there is no context menu item that has focus at first. So I'm not sure why we fired focus events on menu items before August 31.

I'll look into it more, but that's the first thing I wonder when reading about this bug. Did we really fire a focus event for the first menu item in a context menu as it opens before August 31? 

Comment 4

[Marco Zehe (:MarcoZ)]

(In reply to comment #3)
> Did we really fire a focus event for the first menu item in a context
> menu as it opens before August 31? 

No, but:
a) JAWS has a mechanism that automatically highlights the first menu item of a context menu or the Windows Start menu, for example. So with JAWS running, if a context menu pops up, the first menu item gets highlighted automatically.
b) If pressing DownArrow to walk the menus, one should get speech as the focused menu item changes. With builds of August 30 and earlier, we do, with builds later than August 30, we no longer do.

Comment 5

[Tim Keenan]

Actually, this stopped working after 08-28-05.
Here's an Accevent log of me arrowing through a link context menu from that build:

SYS_MENUPOPUPSTART	 Name="" Role=popup menu State=invisible,offscreen
OBJ_FOCUS	 Name="Open Link in New Window" Role=menu item State=focused
OBJ_FOCUS	 Name="Open Link in New Tab" Role=menu item State=focused
OBJ_FOCUS	 Name="Bookmark This Link..." Role=menu item State=focused
OBJ_FOCUS	 Name="Save Link As..." Role=menu item State=focused
OBJ_FOCUS	 Name="Send Link..." Role=menu item State=focused
OBJ_FOCUS	 Name="Copy Link Location" Role=menu item State=focused
OBJ_FOCUS	 Name="Properties" Role=menu item State=focused

Comment 6

[Marco Zehe (:MarcoZ)]

(In reply to comment #5)
> Actually, this stopped working after 08-28-05.

Confirmed, but it is definitely temporarily working in the August 30 build that is otherwise busted for accessibility. So something that was checked in on the 28th, was then twisted a couple of times so it temporarily worked on August 30, then stopped working again on August 31.

Comment 7

[Aaron Leventhal]

As Marco says in bug 393094, that was the cause of this.

Comment 8

[Aaron Leventhal]

Attachment: Properly calculate state_collapsed for popups

Evan, make sure I didn't break your fix for bug 393094.

Comment 9

[Evan Yan]

Comment on attachment 280296 [details] [diff] [review]
Properly calculate state_collapsed for popups

sorry, but it does break the fix of bug 393094

Comment 10

[Aaron Leventhal]

Evan, aren't you going to help debug why? :) It's your regression that I'm helping you with here.

Comment 11

[Joanmarie Diggs]

It seems that XUL combo boxes may have also been impacted by the fix for bug 393094.  If a XUL combo box is collapsed, we get no events as we arrow among the choices.   Aaron, want a new bug for that, or would you prefer to just deal with it here?

Comment 12

[Aaron Leventhal]

Evan can just deal with it here.

Comment 13

[Evan Yan]

Attachment: don't fire focus when with STATE_INVISIBLE

based on Aaron's former patch.

Comment 14

[Aaron Leventhal]

Marco says this fixes the problem, but I see a lot of places the code can be improved.

Comment 15

[Aaron Leventhal]

Comment on attachment 281485 [details] [diff] [review]
don't fire focus when with STATE_INVISIBLE

Can we just check STATE_INVISIBLE instead of doing the isCollapsed/isHidden check?
e.g.
+ if (*aState && nsIAccessibleStates::STATE_INVISIBLE) {
+   isActive = PR_FALSE;
+ }  
+ else if (!isActive) {
  ...
(and remove this part)
Index: accessible/src/xul/nsXULMenuAccessible.cpp
>   if (!isActive) {
>+    // If a menu popup is not hidden, then it is active
>+    // XXX Is this test enough? Can we remove the other tests
>+    // in this method?
>+    nsCOMPtr<nsIDOMXULElement> xulElement = do_QueryInterface(element);
>+    NS_ENSURE_TRUE(xulElement, NS_ERROR_FAILURE);
>+    if (xulElement) {
>+      PRBool isCollapsed, isHidden;
>+      xulElement->GetCollapsed(&isCollapsed);
>+      xulElement->GetHidden(&isHidden);
>+      if (!isCollapsed && !isHidden) {
>+        isActive = PR_TRUE;
>+      }
>+    }
>+  }
>+  if (!isActive) {

If you do that the below code can be combined. into 1 statement then ( don't need the new part)
> 
>   if (!isActive)
>     *aState |= (nsIAccessibleStates::STATE_OFFSCREEN |
>                 nsIAccessibleStates::STATE_INVISIBLE |
>                 nsIAccessibleStates::STATE_COLLAPSED);
>+  else if (*aState == nsIAccessibleStates::STATE_INVISIBLE)
>+    *aState |= nsIAccessibleStates::STATE_OFFSCREEN |
>+               nsIAccessibleStates::STATE_COLLAPSED;
> 
>   return NS_OK;
> }


Why is this change needed if nsXULMenuPopupAccesible::GetState() makes sure all 3 states are set?
>Index: accessible/src/base/nsRootAccessible.cpp
>-        if (State(containerAccessible) & nsIAccessibleStates::STATE_COLLAPSED)
>+        PRUint32 state = State(containerAccessible);
>+        if (state & nsIAccessibleStates::STATE_COLLAPSED
>+            || state & nsIAccessibleStates::STATE_INVISIBLE
>+            || state & nsIAccessibleStates::STATE_OFFSCREEN)

Comment 16

[Evan Yan]

(In reply to comment #15)
> (From update of attachment 281485 [details] [diff] [review])
> Can we just check STATE_INVISIBLE instead of doing the isCollapsed/isHidden
> check?

We need both of STATE_INVISIBLE check and isCollapsed/isHidden check.
isCollapsed/isHidden fix the problem of this bug. For autocomplete issue in bug 393094, nsAccessible::GetState() returns STATE_INVISIBLE, while isCollapsed/isHidden are all false.

> If you do that the below code can be combined. into 1 statement then ( don't
> need the new part)
> Why is this change needed if nsXULMenuPopupAccesible::GetState() makes sure all
> 3 states are set?

Sorry, it's my fault. I meant to just add checking of STATE_INVISIBLE in nsRootAccessible, and don't combine STATE_COLLAPSED | STATE_OFFSCREEN into aState when isActive in nsXULPopupmenuAccessible. There is situation that having STATE_INVISIBLE while isCollapse/isHidden are false (isActive).

Comment 17

[Evan Yan]

Attachment: updated patch

Comment 18

[Aaron Leventhal]

Please change this:

-        if (State(containerAccessible) & nsIAccessibleStates::STATE_COLLAPSED)
+        PRUint32 state = State(containerAccessible);
+        if (state & nsIAccessibleStates::STATE_COLLAPSED
+            || state & nsIAccessibleStates::STATE_INVISIBLE
+            || state & nsIAccessibleStates::STATE_OFFSCREEN)

to:
+        const kPopupUnavailable = nsIAccessibleStates::STATE_COLLAPSED |
+                                  nsIAccessibleStates::STATE_INVISIBLE;
+        if (State(containerAccessible) & kPopupUnavailable) {

I don't believe STATE_OFFSCREEN needs to be there if STATE_INVISIBLE is.


Also, shouldn't OFFSCREEN+INVISIBLE+COLLAPSED all be set if isActive? If that's true, this change in nsRootAccessible is not necessary.

Comment 19

[Aaron Leventhal]

>  If that's true, this change in nsRootAccessible is not necessary.
What I mean is, I think you should set all those states in nsXULPopupAccessible if !isActive, and remove your changes to nsRootAccessible.

Comment 20

[Aaron Leventhal]

When the popup is collapsed does it already have STATE_INVISIBLE?

I kind of like the solution you had before for popups:
1. If *aState & (STATE_INVISIBLE | STATE_COLLAPSED) it's not active 
2. If not active, it's invisible + collapsed

Then, would that be enough?

Comment 21

[Evan Yan]

the situation in the scenario of bug 393094, it have STATE_INVISIBLE, but isCollapsed and isHidden all got false value.
So it's STATE_INVISIBLE and active.

Comment 22

[Aaron Leventhal]

If it's INVISIBLE how can it be acitve?

Don't use the code I suggested for GetCollapsed etc.
Instead just let INVISIBLE help tell you if its active or not.

Comment 23

[Evan Yan]

(In reply to comment #22)
> If it's INVISIBLE how can it be acitve?
> 
The situation for autocomplete is a little complicated. It's a xul popupmenu, inside which there is a xul tree.
When press right arrow to select an autocomplete listitem, we got STATE_INVISIBLE, but xulElement->GetCollapsed() and GetHidden() still tell it's active.

> Don't use the code I suggested for GetCollapsed etc.
> Instead just let INVISIBLE help tell you if its active or not.
> 
I think we still need GetCollapsed for context menu.

Your first patch fixed this bug, but broke autocomplete issue of bug 393094. So I added STATE_INVISIBLE check based on your first patch.

Comment 24

[Aaron Leventhal]

I think GetCollapsed() and GetHidden() are not telling the truth then, right?

Is the popup open or closed after the right arrow is pressed?

Comment 25

[Evan Yan]

the autocomplete popup list is closed in visual.

Comment 26

[Aaron Leventhal]

That means STATE_INVISIBLE is a better indication than GetCollapsed and GetHidden(). So, use that to set STATE_COLLAPSED.

Comment 27

[Evan Yan]

Attachment: patch v3

Comment 28

[Aaron Leventhal]

Comment on attachment 281646 [details] [diff] [review]
patch v3

Evan, do we need this section anymore?
+    // If a menu popup is not hidden, then it is active
+    // XXX Is this test enough? Can we remove the other tests
+    // in this method?

Or does the INVISIBLE test already cover that?

Comment 29

[Evan Yan]

Can we ONLY depend on INVISIBLE to determine whether the menupoup is collapsed?
If not, we still need GetCollapsed/GetHidden. I think it's safe to keep it there.

Comment 30

[Aaron Leventhal]

Please spend a little more time finding the cleanest way to code it. The "safest" way is often the smallest way -- less code to break later. Easier for future developers to understand. It's always worth spending a little time polishing a fix.

Comment 31

[Evan Yan]

Attachment: just use STATE_INVISIBLE to determine whether xulmenupopup is collapsed or not

Comment 32

[Aaron Leventhal]

Comment on attachment 281820 [details] [diff] [review]
just use STATE_INVISIBLE to determine whether xulmenupopup is collapsed or not

Evan, what's with the big #ifdef DEBUG_A11Y section? Should it be gone?

Comment 33

[Aaron Leventhal]

Comment on attachment 281820 [details] [diff] [review]
just use STATE_INVISIBLE to determine whether xulmenupopup is collapsed or not

Cancelling request until that is explained.

Comment 34

[Evan Yan]

(In reply to comment #32)
> (From update of attachment 281820 [details] [diff] [review])
> Evan, what's with the big #ifdef DEBUG_A11Y section? Should it be gone?
> 
Yes, it should be gone. I just leave it there for debug use, in case we encounter some unexpected behavior (inactive but no INVISIBLE).

Comment 35

[Aaron Leventhal]

Comment on attachment 281820 [details] [diff] [review]
just use STATE_INVISIBLE to determine whether xulmenupopup is collapsed or not

Got it, thanks. Looks good.

Comment 36

[Evan Yan]

Checking in nsXULMenuAccessible.cpp;
/cvsroot/mozilla/accessible/src/xul/nsXULMenuAccessible.cpp,v  <--  nsXULMenuAccessible.cpp
new revision: 1.67; previous revision: 1.66
done

Comment 37

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381