257638 - [ATK] Menu Item doesn't work properly

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Louie Zhao]

Menu Item doesn't work properly with GOK and Gnopernicus:

1. The checkboxes in the Menu arent represented in GOK 'Menus'.
2. The radio buttons in the Menu arent represented in GOK Menus.

   Mozilla doesn't set proper role to menu item when it's a checkbox or radiobutton.

3. Mozilla starts up. In GOK window, click "Menus"->"File"->"Open Web Location".
Open Web Location dialog pops up, only keyboard can work now. GOK can't operate
on the dialog now.
 
   When popping up "Open Web Location" dialog using "Click" method of menuitem,
"Click" method only returns after closing the dialog, which will prevent GOK to
proceed properly.

4. Menu->View->Text Zoom has no accessible name.
   The content of some menuitems is updated dynamically. We need fire a "popup
showing" event to trigger such updating.

Comment 1

[Louie Zhao]

Attachment: patch v1

Comment 2

[Philip K. Warren]

Comment on attachment 157689 [details] [diff] [review]
patch v1

>Index: src/xul/nsXULMenuAccessible.cpp
>===================================================================
>@@ -75,16 +86,18 @@
> 
>   if (!menuItemType.IsEmpty()) {
>     // Selectable?
>-    if (menuItemType.EqualsLiteral("radio"))
>+    if (menuItemType.EqualsIgnoreCase("radio") ||   
>+        menuItemType.EqualsIgnoreCase("checkbox"))
>       *_retval |= STATE_SELECTABLE;
> 
>     // Checked?
>     PRBool isChecked = PR_FALSE;
>     element->HasAttribute(NS_LITERAL_STRING("checked"), &isChecked); 
>     if (isChecked) {
>-      if (*_retval & STATE_SELECTABLE)
>-        *_retval |= STATE_SELECTED;  // Use STATE_SELECTED for radio buttons
>-      else *_retval |= STATE_CHECKED;
>+      nsAutoString checkValue;
>+      element->GetAttribute(NS_LITERAL_STRING("checked"), checkValue);
>+      if (!checkValue.EqualsIgnoreCase("false"))
>+        *_retval |= STATE_CHECKED;
>     }
>   }
> 

Aaron - how do these changes affect Win32? We are making all radio and checkbox
menu items have STATE_SELECTABLE, and then setting them to have STATE_CHECKED
if they are selected (no longer using STATE_SELECTED).

>@@ -173,6 +186,18 @@
> NS_IMETHODIMP nsXULMenuitemAccessible::GetRole(PRUint32 *_retval)
> {
>   *_retval = ROLE_MENUITEM;
>+
>+#ifdef MOZ_ACCESSIBILITY_ATK
>+  nsCOMPtr<nsIDOMElement> element(do_QueryInterface(mDOMNode));
>+  NS_ASSERTION(element, "No DOM element for menu node!");
>+  nsAutoString menuItemType;
>+  element->GetAttribute(NS_LITERAL_STRING("type"), menuItemType);
>+  if (menuItemType.EqualsIgnoreCase("radio"))
>+    *_retval = ROLE_RADIO_MENU_ITEM;
>+  else if (menuItemType.EqualsIgnoreCase("checkbox"))
>+    *_retval = ROLE_CHECK_MENU_ITEM;
>+#endif
>+
>   return NS_OK;
> }
> 

As Aaron has mentioned previously, we would like to have these platform
specific changes implemented in wrapper classes in the src/atk/* directory.

The timer changes look good to me.

Comment 3

[Aaron Leventhal]

I just tested this with Window-Eyes and it actually works better than before the
patch. Without this patch, the menu item is read as "checked" even when unchecked.

I agree that the platform-specific code should be moved into the Wrap files. In
this case, I think we should add the new roles to xp code and map them to
ROLE_MENUITEM in msaa/nsAccessibleWrap.cpp. That way when we fix role and state
to work for scripts we can use the rich set of roles in javscript. Generally I
think it's better to start with more information in the xp code, and map into to
the subset in the Wrap classes.

Comment 4

[Louie Zhao]

Attachment: patch v2

1. The timer issue will be fixed in bug 277888 since many other places beside
nsXULMenuitemAccessible also need timer.

2. Define ROLE_RADIO_MENUITEM and ROLE_CHECK_MENUITEM in MSAA Roles, so
nsXULMenuitemAccessible::GetRole contains no platform-specific code.

Comment 5

[Louie Zhao]

patch checked in