152370 - menulist and listbox not reporting correct accessible value

Status: VERIFIED FIXED

Description

[dsirnapalli]

-- Testcase attached.
Summary describes the bug.

Comment 1

[dsirnapalli]

Created attachment 87976 [details]
Test Case to reproduce the bug.

Comment 2

[Aaron Leventhal]

Dharma, the value of a combobox or list should be the name of the item that is
currently selected. It should not be reporting an error at all. If it does
report an error, I think NS_ERROR_FAILURE would be the correct failure code,
because this is in fact implemented.

More about the problem:

When I look at the menulists in font appearance prefs, I see that the Serif,
Sans-serif and monospace font menulist's report their value correctly.

It turns out that the menu items in those menulists are not supporting
nsIDOMXULSelectControlItemElement, as they should be. It appears they're not
picking up the correct bindings.

Comment 3

[Aaron Leventhal]

Created attachment 88066 [details] [diff] [review]
Sometimes the list of items for the dropdown wasn't fully loaded, but the text field in the combobox contained the appropriate value. This patch uses the value in the text  field now.

Seeking r= from Kyle or John Gaunt

Comment 4

[Kyle Yuan]

Aaron, seems this fix is only for menulist. 

I have a different idea about fixing this bug. Since you fixed bug 147756, now 
we can do nsCOMPtr<nsIDOMXULSelectControlElement> select(do_QueryInterface
(mDOMNode)) for menulist/listbox successfully. I think current implemetations 
of getAccValue are logical and consistent with HTML impl.

There is one thing should be considered, we should use selectedItem->GetLabel 
or selectedItem->GetValue as the return value. (currently, listitem supports 
GetLabel, but menuitem doesn't)?

Comment 5

[Kyle Yuan]

this is my fix :)

Index: nsXULSelectAccessible.cpp
===================================================================
RCS file: /cvsroot/mozilla/accessible/src/xul/nsXULSelectAccessible.cpp,v
retrieving revision 1.6
diff -u -r1.6 nsXULSelectAccessible.cpp
--- nsXULSelectAccessible.cpp	12 Jun 2002 05:16:30 -0000	1.6
+++ nsXULSelectAccessible.cpp	18 Jun 2002 04:47:25 -0000
@@ -534,7 +534,7 @@
   if (select) {
     nsCOMPtr<nsIDOMXULSelectControlItemElement> selectedItem;
     select->GetSelectedItem(getter_AddRefs(selectedItem));
-    return selectedItem->GetValue(_retval);
+    return selectedItem->GetLabel(_retval);
   }
   return NS_ERROR_FAILURE;
 }
Index: menu.xml
===================================================================
RCS file: /cvsroot/mozilla/xpfe/global/resources/content/bindings/menu.xml,v
retrieving revision 1.14
diff -u -r1.14 menu.xml
--- menu.xml	13 Apr 2002 14:25:31 -0000	1.14
+++ menu.xml	18 Jun 2002 04:49:30 -0000
@@ -51,6 +51,8 @@
     <implementation>
       <property name="value" onset="this.setAttribute('value',val); return 
val;"
                             onget="return this.getAttribute('value');"/>
+      <property name="label" onget="return this.getAttribute('label');"
+                             onset="this.setAttribute('label', val); return 
val;"/>
     </implementation>  
   </binding>
 
If we decide to use GetValue, then there is nothing to be changed.

Comment 6

[Aaron Leventhal]

Created attachment 88175 [details] [diff] [review]
Fixes GetAccValue() for <listbox> as well. Uses label of selected element for value.

Kyle, I didn't use your change to nsComboBoxAccessible::GetAccValue() because
it doesn't work for the case where the entire list isn't loaded yet.

For example, run the MSAA Inspect tool and open Prefs -> Appearance -> Fonts.
If you inspect the combo box, your patch doesn't produce the correct value.
This is because the entire list isn't actually loaded yet, but the textfield
shows the currently selected value.

However, adding label support to menu item was definitely useful. Thank you.

Seeking r=

Comment 7

[Kyle Yuan]

Comment on attachment 88175 [details] [diff] [review]
Fixes GetAccValue() for <listbox> as well. Uses label of selected element for value.

r=kyle

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 88175 [details] [diff] [review]
Fixes GetAccValue() for <listbox> as well. Uses label of selected element for value.

sr=jst

Comment 9

[Aaron Leventhal]

fixed

Comment 10

[dsirnapalli]

-- Verified in todays trunk build. Works fine. Marking bug as verified.