Closed Bug 350186 Opened 18 years ago Closed 18 years ago

accessible objects for xforms input controls

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

Details

Attachments

(2 files, 1 obsolete file)

The bug continues bug 337250.
Depends on: 337250
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
The support of xform:input[type="xsd:date] (calendar widget) and xforms:input[type="xsd:gMonth"] (combobox widget) is skiped.
Attachment #235920 - Flags: review?(aaronleventhal)
Attached file test
Comment on attachment 235920 [details] [diff] [review]
patch

Excellent work! Some comments:

+      *aExtState |= EXT_STATE_EDITABLE;
+      *aExtState |= EXT_STATE_SELECTABLE_TEXT;

I don't know if the compilers are all smart enough to optimize this, so you could do:
aExtState |= (EXT_STATE_EDITABLE | EXT_STATE_SELECTABLE_TEXT);

Also, the |state| variable name is a bit generic and it would be better to use |isReadonly| and |isRelevant| in order to make the code easier to read.

+nsXFormsInputBooleanAccessible::GetState(PRUint32 *aState)
Might as well put the NS_ENSURE_ARG_POINTER here as you do elsewhere.

+// nsXFormsInputBooleanAccessible
Misplaced comment here, it should say
// nsXFormsSecretAccessible


The biggest thing will be the support for nsIAccessibleText/nsIAccessibleEditableText. If we can have the anonymous children of the input's editor created then you could just inherit from nsHyperTextAccessible and get all that for free. We can deal with that in another patch.
(In reply to comment #3)

> The biggest thing will be the support for
> nsIAccessibleText/nsIAccessibleEditableText. If we can have the anonymous
> children of the input's editor created then you could just inherit from
> nsHyperTextAccessible and get all that for free. We can deal with that in
> another patch.
> 

I guess it's better to leave it for follow up patch. It's better to ask xforms controls to implement needed functionality for these interface instead trying to recall corresponding methods from xul/xhtml native widgets. It allows us to keep accessible object independent of xforms host document.
(In reply to comment #3)

> The biggest thing will be the support for
> nsIAccessibleText/nsIAccessibleEditableText. If we can have the anonymous
> children of the input's editor created then you could just inherit from
> nsHyperTextAccessible and get all that for free. We can deal with that in
> another patch.
> 

Disadvantages of nsHyperTextAccessible I can see are:
1) I should have separate accessible object for every document type (xhtml, xul and other supported in future).
2) I should have one more object inherited from nsHyperTextAccessible to keep xforms things and I cannot reuse nsXFormsAccessible object here to avoid multiple inheritance.
3) XForms accessible object will inherit much unneeded code of nsHyperTextAccessible.
4) If xforms textfield value is changed by nsIAccessibleEditableText interface then we should update instance node that textfiled is bound to. It means I cannot use "pure" nsHyperTextAccessible methods.

Disadvantage of implementation of these interfaces for xforms input is much of new code for nsXFormsInputAccessible that partially nsHyperTextAccessible has already.

But in any way I guess xforms input wants additional interface to provide methods like its kins from xhtml and xul have. So I'd vote to implement these interface by starting from nothing.
There are a lot of methods to implement in nsIAccessibleText, nsIAccessibleEditableText and nsIAccessibleHyperText. And some of the methods have many complex parameters (E.g. getTextAtOffset). I think you're in danger of getting a lot of code duplication of the wrong kind. 

> Disadvantages of nsHyperTextAccessible I can see are:
> 1) I should have separate accessible object for every document type (xhtml, 
> xul and other supported in future).
In the future we'll probably have the <body> support the 3 text interfaces, but probably not the document itself. But, I'm not sure if this addresses what you mean.

> 2) I should have one more object inherited from nsHyperTextAccessible to keep
> xforms things and I cannot reuse nsXFormsAccessible object here to avoid
> multiple inheritance.
Could you put the xforms code a mixin/helper class that doesn't have any interface support on its own?
If you use multiple inheritance or a mixin class, or make all XForms elements inherit from Hypertext accessible, then yes you've got unnecessary methods on every object but at least you aren't increasing codesize by as much as reimplementing everything.
Keep in mind that nsHyperTextAccessible::QueryInterface makes sure to only say a given interface is supported when it's appropriate.

> 3) XForms accessible object will inherit much unneeded code of
> nsHyperTextAccessible.
Right, but that code already is loaded anyway, so it doesn't increase code size, correct? I suppose the size of each vtbl would increase, but that's per class and not a big deal.

> 4) If xforms textfield value is changed by nsIAccessibleEditableText interface
> then we should update instance node that textfiled is bound to. It means I
> cannot use "pure" nsHyperTextAccessible methods.
No, but they are virtual so you can upcall before/after doing this in the Xforms versions of those methods. Or no doubt the methods could call a virtual UpdateData() which would only do something in the XForms classes.

> But in any way I guess xforms input wants additional interface to provide
> methods like its kins from xhtml and xul have.
I don't understand this point.

(In reply to comment #6)
> There are a lot of methods to implement in nsIAccessibleText,
> nsIAccessibleEditableText and nsIAccessibleHyperText. And some of the methods
> have many complex parameters (E.g. getTextAtOffset). I think you're in danger
> of getting a lot of code duplication of the wrong kind. 

Accessible objects for xul:textbox/html:input/xforms:input for xhtml document/xforms:input for xul document can have one and the same base since all of these elements are builded on top of html:input. And for these elements we don't need huge code of nsIHypertextAccessible I guess (correct me if I'm wrong). Therefore I'd like to add new class that implements nsIAccessibleText/nsIAccessibleEditableText interfaces and that class will be used as base for those elements.

> > Disadvantages of nsHyperTextAccessible I can see are:
> > 1) I should have separate accessible object for every document type (xhtml, 
> > xul and other supported in future).
> In the future we'll probably have the <body> support the 3 text interfaces, but
> probably not the document itself. But, I'm not sure if this addresses what you
> mean.

I wanted to say we should have at least two accessible objects for xforms:input. The first is for xforms:input that hosted in xhtml document, the second is for xforms:input that hosted in xul document.

> > 2) I should have one more object inherited from nsHyperTextAccessible to keep
> > xforms things and I cannot reuse nsXFormsAccessible object here to avoid
> > multiple inheritance.
> Could you put the xforms code a mixin/helper class that doesn't have any
> interface support on its own?
> If you use multiple inheritance or a mixin class, or make all XForms elements
> inherit from Hypertext accessible, then yes you've got unnecessary methods on
> every object but at least you aren't increasing codesize by as much as
> reimplementing everything.
> Keep in mind that nsHyperTextAccessible::QueryInterface makes sure to only say
> a given interface is supported when it's appropriate.

If all xforms elements will be inherited from nsHyperTextAccessible that I guess it's good decision if new class idea is not suitable.

> > But in any way I guess xforms input wants additional interface to provide
> > methods like its kins from xhtml and xul have.
> I don't understand this point.
> 

I had in view xforms:input hasn't right now methods to work with selection/text/clipboard. I.e it's impossible to implement nsIAccessibleText/nsIAccEditableText interface for xforms:input. But I guess we can add necesseary methods.
So are choices seem to be:
1) Reimplement and maintain 23 methods for html:input, xforms:input and xul:textbox in order to support nsIAccessibleText and nsIAccessibleHyperText.
or
2) Just having an extra class in the inheritance chain, with some code that won't be used by each object.

Do I have this correct?

I see your point about #2, that there will be extra code in each class that isn't needed. However, I still like it better because I don't see the extra code as being a problem. I do see implementing 23 methods as a very large effort. IMO, the extra available code on each object for option #2 does not actually hurt anything when it's not used.

I'm open to #1 but am not sure what we really gain for the amount of effort. Let's find out from some other folks such as Ginn Chen what they think. 
(In reply to comment #8)
> So are choices seem to be:
> 1) Reimplement and maintain 23 methods for html:input, xforms:input and
> xul:textbox in order to support nsIAccessibleText and nsIAccessibleHyperText.
> or
> 2) Just having an extra class in the inheritance chain, with some code that
> won't be used by each object.
> 
> Do I have this correct?

Absolutely.

> I see your point about #2, that there will be extra code in each class that
> isn't needed. However, I still like it better because I don't see the extra
> code as being a problem. I do see implementing 23 methods as a very large
> effort. IMO, the extra available code on each object for option #2 does not
> actually hurt anything when it's not used.

There is other extra code. All methods for nsIAccessibleText/nsIAccessibleEditableText assumes we have couple of nodes inside editor and these methods run through these nodes instead to call method of html:input. I guess these 23 methods implementation will be quite simple for html:input.

> I'm open to #1 but am not sure what we really gain for the amount of effort.
> Let's find out from some other folks such as Ginn Chen what they think. 
> 

Really, I have not big preferences and if you think to keep things like they are then it's fine for me.
There's also a #3, and I think it might be the simplest.

#3 - Have nsXFormsInputAccessible inherit from nsHyperTextAccessible. It would not have nsXFormsAccessible in the inheritance chain. Then either copy the code from nsXFormsAccessible that you need, or create a helper/mixin class. That way you are at worst just copying implementations over to the new class, instead of reimplementing stuff.

What about that one?
BTW, does XForms have a multiline textfield? If so there can be <br>'s in it for the newlines, and the code may end up being more like nsHyperTextAccessible than you think.

And I think you mentioned this earlier -- we'll need nsIAccessibleText support for xforms:output.

Can xforms comboboxes be editable? If so they'll need to support nsIAccessibleEditableText.
(In reply to comment #11)
> BTW, does XForms have a multiline textfield? If so there can be <br>'s in it
> for the newlines, and the code may end up being more like nsHyperTextAccessible
> than you think.

Yes, it has. But html:textarea provides specific interfaces like it does html:input.

> And I think you mentioned this earlier -- we'll need nsIAccessibleText support
> for xforms:output.
> 
> Can xforms comboboxes be editable? If so they'll need to support
> nsIAccessibleEditableText.
> 

Here can be problem because it seems hypertext doesn't work with anonymous nodes.
(In reply to comment #12)
> Here can be problem because it seems hypertext doesn't work with anonymous
> nodes.
It can do that. The nodes in a textarea, for example are anonymous and it works with that.
(In reply to comment #13)
> (In reply to comment #12)
> > Here can be problem because it seems hypertext doesn't work with anonymous
> > nodes.
> It can do that. The nodes in a textarea, for example are anonymous and it works
> with that.
> 

Ok, so is it good way to inherit all xforms accessible object form hypertext accessible since it allows to keep things simpler?
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Here can be problem because it seems hypertext doesn't work with anonymous
> > > nodes.
> > It can do that. The nodes in a textarea, for example are anonymous and it works
> > with that.
> > 
> 
> Ok, so is it good way to inherit all xforms accessible object form hypertext
> accessible since it allows to keep things simpler?
> 

Or, we can create new accessible objects that will expose all methods of nsIAccessibleText/nsIAccessibleEditableText in manner like hypertext makes it. Then we can inherit heypertext accessible and some xforms accessibles from that object. How about it?
Am I still supposed to review this or is it a work in progress?

I've learned that anything which puts text on the screen should support the text interface. That includes buttons and options in a list.
Could you help me understand how we draw misspelled word underlines? Do we use CSS  (do the misspelled words have their own anonymous node?)
(In reply to comment #17)
> Could you help me understand how we draw misspelled word underlines? Do we use
> CSS  (do the misspelled words have their own anonymous node?)
> 

It looks like all magic should be in http://lxr.mozilla.org/mozilla/source/extensions/spellcheck/src/mozInlineSpellChecker.cpp#1232. But I can't find out right now where exactly.
(In reply to comment #16)
> Am I still supposed to review this or is it a work in progress?

Yes, review is still needed since I suppose nothing important will be changed in code of this patch when I'll add implementation of nsIAccEditableText/nsIAccText interfaces.
Attachment #235920 - Flags: review?(aaronleventhal) → review+
Attachment #235920 - Flags: review?(aaronr)
Comment on attachment 235920 [details] [diff] [review]
patch

>Index: accessible/src/xforms/nsXFormsFormControlsAccessible.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/xforms/nsXFormsFormControlsAccessible.cpp,v
>retrieving revision 1.1
>diff -u -8 -p -r1.1 nsXFormsFormControlsAccessible.cpp
>--- accessible/src/xforms/nsXFormsFormControlsAccessible.cpp	24 Aug 2006 21:13:21 -0000	1.1
>+++ accessible/src/xforms/nsXFormsFormControlsAccessible.cpp	29 Aug 2006 16:25:34 -0000
>@@ -138,8 +138,186 @@ NS_IMETHODIMP

>+NS_IMETHODIMP
>+nsXFormsInputAccessible::GetExtState(PRUint32 *aExtState)
>+{
>+  NS_ENSURE_ARG_POINTER(aExtState);
>+
>+  *aExtState = 0;
>+
>+  NS_ENSURE_TRUE(mDOMNode, NS_ERROR_FAILURE);
>+  nsresult rv = nsXFormsAccessible::GetExtState(aExtState);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRBool state = PR_FALSE;
>+  rv = sXFormsService->IsReadonly(mDOMNode, &state);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (!state) {
>+    rv = sXFormsService->IsRelevant(mDOMNode, &state);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    if (state) {
>+      *aExtState |= EXT_STATE_EDITABLE;
>+      *aExtState |= EXT_STATE_SELECTABLE_TEXT;
>+    }
>+  }

Like AaronL said, please combine these into one statement

>+
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>+  if (content->NodeInfo()->Equals(nsAccessibilityAtoms::textarea))
>+    *aExtState |= EXT_STATE_MULTI_LINE;
>+  else
>+    *aExtState |= EXT_STATE_SINGLE_LINE;
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsInputAccessible::GetNumActions(PRUint8* aCount)
>+{
>+  NS_ENSURE_ARG_POINTER(aCount);
>+
>+  *aCount = 1;
>+  return NS_OK;
>+}
>+

Please put a comment next to the enum to make sure that this gets updated as enums are added.  Or an an 'enum_Last' to the end of the enums and return enum_Last-1 here.  Or will there be at some time actions inside this enum that don't apply to input?

>+NS_IMETHODIMP
>+nsXFormsInputAccessible::GetActionName(PRUint8 aIndex, nsAString& aName)
>+{
>+  if (aIndex != eAction_Click)
>+    return NS_ERROR_INVALID_ARG;
>+
>+  nsAccessible::GetTranslatedString(NS_LITERAL_STRING("activate"), aName);
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsInputAccessible::DoAction(PRUint8 aIndex)
>+{
>+  if (aIndex != 0)
>+    return NS_ERROR_INVALID_ARG;
>+
>+  return sXFormsService->Focus(mDOMNode);
>+}
>+

You should probably test against eAction_Click instead of 0, right?

>+// nsXFormsInputBooleanAccessible
>+
>+nsXFormsInputBooleanAccessible::
>+  nsXFormsInputBooleanAccessible(nsIDOMNode *aNode, nsIWeakReference *aShell):
>+  nsXFormsAccessible(aNode, aShell)
>+{
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsInputBooleanAccessible::GetRole(PRUint32 *aRole)
>+{
>+  NS_ENSURE_ARG_POINTER(aRole);
>+
>+  *aRole = ROLE_CHECKBUTTON;
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsInputBooleanAccessible::GetState(PRUint32 *aState)
>+{
>+  nsresult rv = nsXFormsAccessible::GetState(aState);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsAutoString value;
>+  rv = sXFormsService->GetValue(mDOMNode, value);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (value.EqualsLiteral("true"))
>+    *aState |= STATE_CHECKED;
>+
>+  return NS_OK;
>+}

shouldn't you make sure to remove the checked state here if it exists already in aState?

>+
>+NS_IMETHODIMP
>+nsXFormsInputBooleanAccessible::GetNumActions(PRUint8 *aCount)
>+{
>+  NS_ENSURE_ARG_POINTER(aCount);
>+
>+  *aCount = 1;
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsInputBooleanAccessible::GetActionName(PRUint8 aIndex, nsAString& aName)
>+{
>+  if (!aIndex == eAction_Click)
>+    return NS_ERROR_INVALID_ARG;
>+
>+  nsAutoString value;
>+  nsresult rv = sXFormsService->GetValue(mDOMNode, value);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (value.EqualsLiteral("true"))
>+    nsAccessible::GetTranslatedString(NS_LITERAL_STRING("uncheck"), aName);
>+  else
>+    nsAccessible::GetTranslatedString(NS_LITERAL_STRING("check"), aName);
>+
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsInputBooleanAccessible::DoAction(PRUint8 aIndex)
>+{
>+  if (aIndex != 0)
>+    return NS_ERROR_INVALID_ARG;
>+
>+  return DoCommand();
>+}
>+

shouldn't this be a test against eAction_Click instead of 0?

with those considered, r=me
Attachment #235920 - Flags: review?(aaronr) → review+
(In reply to comment #20)

> >+  if (!state) {
> >+    rv = sXFormsService->IsRelevant(mDOMNode, &state);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >+    if (state) {
> >+      *aExtState |= EXT_STATE_EDITABLE;
> >+      *aExtState |= EXT_STATE_SELECTABLE_TEXT;
> >+    }
> >+  }
> 
> Like AaronL said, please combine these into one statement

Sorry, will be fixed.

> >+NS_IMETHODIMP
> >+nsXFormsInputAccessible::GetNumActions(PRUint8* aCount)
> >+{
> >+  NS_ENSURE_ARG_POINTER(aCount);
> >+
> >+  *aCount = 1;
> >+  return NS_OK;
> >+}
> >+
> 
> Please put a comment next to the enum to make sure that this gets updated as
> enums are added.  Or an an 'enum_Last' to the end of the enums and return
> enum_Last-1 here.  Or will there be at some time actions inside this enum that
> don't apply to input?

I guess input will not be support more action than 'click'. enum in base class can be extended theoretically by more elements that are needed for controls different from input. Therefore I think situation is possible when enum_Last-1 will not work correct.

> >+NS_IMETHODIMP
> >+nsXFormsInputAccessible::DoAction(PRUint8 aIndex)
> >+{
> >+  if (aIndex != 0)
> >+    return NS_ERROR_INVALID_ARG;
> >+
> >+  return sXFormsService->Focus(mDOMNode);
> >+}
> >+
> 
> You should probably test against eAction_Click instead of 0, right?

Agree.

> shouldn't you make sure to remove the checked state here if it exists already
> in aState?

I shouldn't. This work will be done by nsXFormsAccessible::GetState() method.

Attached patch patch2Splinter Review
Attachment #235920 - Attachment is obsolete: true
Attachment #239813 - Flags: superreview?(neil)
Comment on attachment 239813 [details] [diff] [review]
patch2

>-[scriptable, uuid(b3114d71-c7b2-4966-b1c9-dc66220430ac)]
>+[scriptable, uuid(1a57d854-12df-4b7b-8552-a3cd1fa90618)]
I didn't think you had to change the uuid as you are only adding constants.
Attachment #239813 - Flags: superreview?(neil) → superreview+
(In reply to comment #23)
> (From update of attachment 239813 [details] [diff] [review] [edit])
> >-[scriptable, uuid(b3114d71-c7b2-4966-b1c9-dc66220430ac)]
> >+[scriptable, uuid(1a57d854-12df-4b7b-8552-a3cd1fa90618)]
> I didn't think you had to change the uuid as you are only adding constants.
> 

I'll take into account in the future.

The patch needs to be checked in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: