257093 - [ATK] XUL Textbox has no AccessibleText interface

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Louie Zhao]

After fixing for Bug 244624, XUL Textbox won't inheritage from
nsHTMLTextFieldAccessible anymore. Thus, nsXULTextFieldAccessible has no
AccessibleText interface.

Comment 1

[Louie Zhao]

Attachment: patch v1

In this patch, also fixed another bug "GetXULName". When getting the value of
attribute "label", it will return string "undefined" if there is no "label"
attribute for the object.

Comment 2

[Aaron Leventhal]

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

>Index: src/atk/nsAccessibleText.cpp
>===================================================================
> nsAccessibleText::nsAccessibleText(nsIDOMNode *aNode)
> {
>+  nsCOMPtr<nsIDOMXULTextBoxElement> textBox(do_QueryInterface(aNode));
>+  if (textBox) {
...

Can this somehow be moved to nsXULTextAccessibleWrap?


>+nsXULTextFieldAccessibleWrap::nsXULTextFieldAccessibleWrap(nsIDOMNode* aNode, nsIWeakReference* aShell):
>+nsXULTextFieldAccessible(aNode, aShell), nsAccessibleEditableText(aNode)
>+{ 
>+  nsCOMPtr<nsIDOMXULTextBoxElement> textBox(do_QueryInterface(aNode));
>+  if (textBox) {
>+    nsCOMPtr<nsIDOMHTMLInputElement> inputField;
>+    textBox->GetInputField(getter_AddRefs(inputField));
>+    if (inputField) {
>+      nsCOMPtr<nsIPresShell> shell(do_QueryReferent(mWeakShell));

Will it always be an nsIDOMXULTextBoxElement?
If so, you can get rid of the extra |if (textBox)| and just use an
|NS_ASSERTION(textBox, "Not a XUL textbox!");|

>+#ifdef MOZ_ACCESSIBILITY_ATK
>+  *_retval = new nsXULTextFieldAccessibleWrap(aNode, weakShell);
>+#else
>   *_retval = new nsXULTextFieldAccessible(aNode, weakShell);
>+#endif

Let's avoid #ifdef's in the future. They're going to make things difficult when
people want to write accessibility extensions in Javascript like MozBraille
(mozbraille.mozdev.org).
Anyway, it's cleaner to always create a nsXULTextFieldAccessibleWrap and just
use a typedef in the non-atk versions of nsAccessibleWrap.h

>+  if (NS_FAILED(rv) || label.IsEmpty() || label.EqualsIgnoreCase("undefined") ) {
>+    label.Truncate();

What if the label actually is called "undefined". That's a bug if it's coming
through that way. It's not a clean API. We should be able to find out if there
is no label without comparing it to "undefined".

Comment 3

[Louie Zhao]

Attachment: patch v2

> 
> Can this somehow be moved to nsXULTextAccessibleWrap?
> 

I suppose you mean to move to nsXULTextFieldAccessibleWrap.

> 
> Will it always be an nsIDOMXULTextBoxElement?
> If so, you can get rid of the extra |if (textBox)| and just use an
> |NS_ASSERTION(textBox, "Not a XUL textbox!");|
> 

The answer is yes. (refer to
xpfe/global/resources/content/bindings/textbox.xml)

> >+#ifdef MOZ_ACCESSIBILITY_ATK
> >+  *_retval = new nsXULTextFieldAccessibleWrap(aNode, weakShell);
> >+#else
> >   *_retval = new nsXULTextFieldAccessible(aNode, weakShell);
> >+#endif
> 
> Let's avoid #ifdef's in the future. They're going to make things difficult
when
> people want to write accessibility extensions in Javascript like MozBraille
> (mozbraille.mozdev.org).
> Anyway, it's cleaner to always create a nsXULTextFieldAccessibleWrap and just

> use a typedef in the non-atk versions of nsAccessibleWrap.h
> 

I agree with you. There is a lot of #ifdef MOZ_ACCESSIBILITY_ATK. We should put
atk related code under src/atk, not under crossplatform part.

> >+  if (NS_FAILED(rv) || label.IsEmpty() ||
label.EqualsIgnoreCase("undefined") ) {
> >+	label.Truncate();
> 
> What if the label actually is called "undefined". That's a bug if it's coming

> through that way. It's not a clean API. We should be able to find out if
there
> is no label without comparing it to "undefined".
> 

Yes, this is really a bug. Here is just a workaround because many XUL elements
will export "undefined" if we don't do this. Do you have any suggestion for
this? I have add comments in the code. Maybe We can fix it later if we can't
find any better solution now.

Comment 4

[Louie Zhao]

Attachment: patch updated

Below is the changes in this patch:
1. Implement nsXULTextFieldAccessibleWrap, which has nsIAccessibleEditableText
interface and can export text in XUL textbox.
2. Export "*" instead of real text when textbox (both HTML and XUL) is password
type.

Comment 5

[Aaron Leventhal]

1. What XBL binding is returning "undefined" for GetLabel() ?  It shouldn't be
hard to make it return "" instead. I want to fix that before we check this in.
2. Can you move the ConvertTextToAsterisks into cross platform code and base it
on STATE_PROTECTED? I think it's a good idea to do it for MSAA, as well as any
other new API we end up supporting.

Other than that, it looks good. I expect to r= the next patch :)

Comment 6

[Louie Zhao]

Attachment: patch v4

>1. What XBL binding is returning "undefined" for GetLabel() ?	It shouldn't be

>hard to make it return "" instead. I want to fix that before we check this in.


In mozilla 1.7 branch, GetLabel of nsIDOMXULLabeledControlElement and 
nsIDOMXULSelectControlItemElement may return "undefined" if there is no label.
In the latest trunk, it doesn't happen with my testcases. I suppose it has
already been fixed. In this patch, get rid of comparing with "undefined" since
it's not needed.

> 2. Can you move the ConvertTextToAsterisks into cross platform code and base
> it on STATE_PROTECTED? I think it's a good idea to do it for MSAA, as well as

> any other new API we end up supporting.

"ConvertTextToAsterisks" is used in implementation of AtkText interface. No
corresponding part is included in MSAA and Mac, which uses
nsIAccessible::GetValue. So no need to move ConvertTextToAsterisks into
crossplatform.

Another fix is included: nsXULTextFieldAccessible::GetValue will return text
even its type is password. That's a secruity issue. Need fix ASAP.

Comment 7

[Louie Zhao]

Thanks for r & sr. Patch checked in.