[ATK] XUL Textbox has no AccessibleText interface

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Louie Zhao, Assigned: Louie Zhao)

Tracking

({access})

Trunk
x86
Linux
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: sunport17)

Attachments

(1 attachment, 3 obsolete attachments)

12.92 KB, patch
Aaron Leventhal
: review+
Henry Jia
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
After fixing for Bug 244624, XUL Textbox won't inheritage from
nsHTMLTextFieldAccessible anymore. Thus, nsXULTextFieldAccessible has no
AccessibleText interface.
(Assignee)

Comment 1

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

Updated

14 years ago
Attachment #157129 - Flags: review?(aaronleventhal)

Comment 2

14 years ago
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".

Updated

14 years ago
Attachment #157129 - Flags: review?(aaronleventhal) → review-
(Assignee)

Comment 3

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

Updated

14 years ago
Attachment #157129 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Keywords: access

Updated

14 years ago
Blocks: 263575

Updated

14 years ago
Whiteboard: sunport17
(Assignee)

Comment 4

14 years ago
Created attachment 168185 [details] [diff] [review]
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.
Attachment #157595 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #168185 - Flags: review?(aaronleventhal)

Comment 5

14 years ago
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 :)

Updated

14 years ago
Attachment #168185 - Flags: review?(aaronleventhal)
Attachment #168185 - Flags: review-
(Assignee)

Comment 6

14 years ago
Created attachment 168945 [details] [diff] [review]
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.
Attachment #168185 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #168945 - Flags: review?(aaronleventhal)

Updated

14 years ago
Attachment #168945 - Flags: review?(aaronleventhal) → review+
(Assignee)

Updated

14 years ago
Attachment #168945 - Flags: superreview?(Henry.Jia)

Updated

14 years ago
Attachment #168945 - Flags: superreview?(Henry.Jia) → superreview+
(Assignee)

Comment 7

14 years ago
Thanks for r & sr. Patch checked in. 
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.