337690 - add initial support of accessible objects for xforms

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

<xf:output ref="xpathexpr">
  <xf:label>It's output</xf:label>
  <html:span>It's output custom look. Mozilla XForms allows to contain arbitrary elements for xforms:output (and not only for)</html:span>
</xf:output>

In XHTML document the "full" content of xf:output (including anonymous content) will be:

<xf:output ref="xpathexpr">
  <xf:label>
  <html:span>That's generated content</html:span>
  <html:span>It's output custom look. ... and so on :)</html:span>
</xf:output>

The example shows that navigation through accessible object for xf:output content will be trickly. I'm interesting can we generalize it and create accessible object with navigation through anonymous content as well as through direct children? Probably binding should provide information about what exactly anonymous content will be havigable.

Comment 1

[alexander :surkov (:asurkov)]

There is other case, it's navigation for xf:label element. Navigation through accessible objects for xf:label children should be either for generated content (if xf:label is bound to instance data) or for direct content. I guess we should have in mind navigation features for xf:label and xf:output while we're trying to generalize behaviour for widgets that have anonymous content.

Comment 2

[aaronr]

What if we set the xf-value class attribute for the span that represents the span that is displayed to the user?  Would help those that want to style the label and output, too.

Comment 3

[Aaron Leventhal]

Attachment: Test case -- works with Accessible Explorer and Window-Eyes

Alexander, the attached test case is spoken by Window-Eyes, because the text node content is exposed as ROLE_TEXT. It's not necessary in this case to expose any extra accessible node.

Can you attach some test cases that don't expose their text to Accessible Explorer or Window-Eyes? BTW, you can download a demo copy of Window-Eyes from http://www.gwmicro.com.

Comment 4

[alexander :surkov (:asurkov)]

nsIAccessible::Name.

XUL: 
1) if there is @xhtml:role attribute then checks @labelledby attribute
2) checks @label attribute
3) searches <xul:label/> element.

HTML:
1) if has @xhtml:role attribute then checks @labelledby attribute
2) searches <label/> element.

XFORMS:
1) Should we provide a support for @xhtml:role?
2) searches <xf:label/> element.

Comment 5

[alexander :surkov (:asurkov)]

nsIAccessible::Description

Common for XUL and XHMTL:
if element has @xhtml2:role then we check @describedby attribute

XUL:
search <xul:description/>
check @tooltiptext

XHTML:
check @title

XForms:
search <xf:hint/>

Also should we check @xhtml:role?

Comment 6

[alexander :surkov (:asurkov)]

I guess the bug 327239 should be fixed to supprot ally name for xforms controls because we should have a way to get label text for ally name.

Comment 7

[Aaron Leventhal]

Check labelledby and describedby whether there is a role or not. We're going to use those regardless of role. It's going to be a universal property which doesn't require a role. I was resistant at first because I wasn't sure if the extra attribute checks would impact performance, but they don't.

See https://bugzilla.mozilla.org/show_bug.cgi?id=337128

Comment 8

[alexander :surkov (:asurkov)]

I want to add intitial support of accessible objects for xforms elements and add accessible objects for xforms:output and xforms:label. Aaron L, does it work for you if I ask of you for initial review? And then I want to get additional (superreview?) from xforms guys, I guess they should ensure that I do the things right in XForms context.

Comment 9

[Aaron Leventhal]

Yes, I want to help with reviews but I'm gone next week.

Can you reply to comment 3?

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> Yes, I want to help with reviews but I'm gone next week.
> 
> Can you reply to comment 3?
> 

(In reply to comment #3)
> Created an attachment (id=222294) [edit]
> Test case -- works with Accessible Explorer and Window-Eyes
> 
> Alexander, the attached test case is spoken by Window-Eyes, because the text
> node content is exposed as ROLE_TEXT. It's not necessary in this case to expose
> any extra accessible node.
>

I belive that Window-Eyes and other software will show correctly text of all xforms elements since they are based on xhtml/xul controls that have accessible objects. But:

1) if xforms:output will have xhtml2:labeledby or xhtml2:describedby attribute then will it have accessible objects?
2) xforms:output can have xforms:label and xforms:hint elements, thease elements serve to define I guess name and description fields of accessible object.

Thease are reasons I can see why we should have accessible object for xforms controls.

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch1

Comment 12

[alexander :surkov (:asurkov)]

Attachment: testcase

Comment 13

[Aaron Leventhal]

Nits: 
- Remove commented-out lines of code from copy/pasted stuff, such as in Makefile.in and some mentions of nsXULAlertAccessible.
- Change long name of XFormsFormControlsAccessible to XFormsControlsAccessible or just XFormsAccessible
- Don't use NS_ENSURE_STATE(content) because it's not unexpected for content to be nsnull. It happens when assistive technology (AT) such as screen reader holds onto an accessible for a DOM node which has been destroyed or hidden via CSS. That accessible object has been "Shutdown" via Shutdown() and mDOMNode will be nsnull. Thus use |if (!content) { return NS_ERROR_FAILURE; }|

Comment 14

[Aaron Leventhal]

More nits:
_retval -> aAccessible (We used to do _retval but an sr= asked us not to do that anymore. We should use well-named method arguments)

+  nsresult rv;
+
+  rv = accessors->IsRelevant(&value);
This can be just 1 line:
+ nsresults rv = accessors->IsRelevant(&value);

Comment 15

[Aaron Leventhal]

+  rv = accessors->IsRelevant(&value);
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (value)
+    *aState |= STATE_UNAVAILABLE;

Shouldn't this be if (!value) it's unavailable? Relevant is the opposite of unavailable.

The method nsXFormsBoundElementsAccessible::GetState(PRUint32 *aState) should probably upcall to nsAccessibleWrap::GetState() rather than initially setting *aState = 0;  It needs to get stuff like STATE_INVISIBLE and STATE_OFFSCREEN from the base accessible class.

Comment 16

[Aaron Leventhal]

Possible optimization:
node -> delegate ->accessors -> value is used in several places. Could have helper method for that.

Comment 17

[Aaron Leventhal]

For label, you might want to take a look at nsLeafAccessible to see how we ensure that there are no child accessibles. Not sure if output should also be a leaf.

Comment 18

[Aaron Leventhal]

We don't want to expose the text nodes of a label or output as children anymore, since that will be redundant, so override nsAccessible::GetChildCount(). Make it do:
CacheChildren(PR_FALSE); // Don't walk anonymous children, normally PR_TRUE
See http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsAccessible.cpp#591


Also, we need to support accessible relations via GetAccessibleRelated(), such as RELATION_LABELLED_BY and RELATION_LABEL_FOR
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsAccessible.cpp#1933

Comment 19

[Aaron Leventhal]

In XUL we have a problem with using nsIAccessibleProvider via XBL. If the XUL is running as content, the XBL has no access to nsIAccessibilityService. See bug 241015. Did you try your test case only as a local file, or does it work if you put it on a web server?

Comment 20

[Aaron Leventhal]

Comment on attachment 224333 [details] [diff] [review]
patch1

Very good start. See comments.

Comment 21

[alexander :surkov (:asurkov)]

Attachment: patch2

(In reply to comment #13)

> - Change long name of XFormsFormControlsAccessible to XFormsControlsAccessible
> or just XFormsAccessible

I just wanted to correspond with xforms specs names. It split all xforms elements on few categories. One of them is Forms Controls, other is UI Elements and so on. I guess it will be good if I name files like sections in specs.

(In reply to comment #16)
> Possible optimization:
> node -> delegate ->accessors -> value is used in several places. Could have
> helper method for that.
> 

I added the method GetXFormsAccessors(), it reduces some code dublications. I prefer it because accessors is used to get states too (not only value).

(In reply to comment #19)
> In XUL we have a problem with using nsIAccessibleProvider via XBL. If the XUL
> is running as content, the XBL has no access to nsIAccessibilityService. See
> bug 241015. Did you try your test case only as a local file, or does it work if
> you put it on a web server?
> 

My tests work only with chrome://. It breaks all idea of xforms accessibility. I should look more precisely at bug 241015.

Comment 22

[alexander :surkov (:asurkov)]

Comment on attachment 224584 [details] [diff] [review]
patch2

GetChildValueByTagName():
I forgot to change NS_ERROR_UNEXPECTED on NS_ERROR_FAILURE. I'll fix it in next patch.

Comment 23

[Aaron Leventhal]

Comment on attachment 224584 [details] [diff] [review]
patch2

+  rv = GetChildValueByTagName(NS_LITERAL_STRING("hint"), aDescription);
+  return rv;

how about 1 line:
return GetChildValueByTagName(NS_LITERAL_STRING("hint"), aDescription);

Comment 24

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 25

[aaronr]

Comment on attachment 224694 [details] [diff] [review]
patch3

If I am reading these build changes correctly (which I may not be), you are going to force XForms to build by default UNLESS DISABLE_XFORMS_HOOKS is set?  Ewww, that would not be good.  Code surrounded by DISABLE_XFORMS_HOOKS should be able to compile and exist fine even if XForms isn't built.  Like in XPath.  XForms should only build because 'xforms' was listed as an extension in the .mozconfig.  Maybe you can make XForms utility service return what you need it to?


>Index: mozilla/accessible/src/xforms/nsXFormsAccessible.cpp
>===================================================================
>RCS file: mozilla/accessible/src/xforms/nsXFormsAccessible.cpp
>diff -N mozilla/accessible/src/xforms/nsXFormsAccessible.cpp
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ mozilla/accessible/src/xforms/nsXFormsAccessible.cpp	7 Jun 2006 15:11:07 -0000
>@@ -0,0 +1,198 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2006
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Alexander Surkov <[e-mail]> (original author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either of the GNU General Public License Version 2 or later (the "GPL"),
>+ * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#include "nscore.h"
>+
>+#include "nsXFormsAccessible.h"
>+#include "nsIDOMElement.h"
>+#include "nsIDOMNodeList.h"
>+#include "nsIXFormsDelegate.h"
>+
>+#define NS_NAMESPACE_XFORMS              "http://www.w3.org/2002/xforms"
>+
>+// nsXFormsAccessible
>+
>+nsXFormsAccessible::nsXFormsAccessible(nsIDOMNode* aNode, nsIWeakReference* aShell):
>+  nsAccessibleWrap(aNode, aShell)
>+{
>+}
>+
>+NS_IMETHODIMP nsXFormsAccessible::GetName(nsAString& aName)
>+{
>+  nsAutoString name;
>+  nsresult rv = GetTextFromRelationID(nsAccessibilityAtoms::labelledby, name);
>+  aName = name;
>+
>+  return rv;
>+}

nit: spelling.  Should be 'labeled', I believe

>+
>+NS_IMETHODIMP nsXFormsAccessible::GetDescription(nsAString& aDescription)
>+{
>+  nsAutoString description;
>+  nsresult rv = GetTextFromRelationID(nsAccessibilityAtoms::describedby, description);
>+  aDescription = description;
>+
>+  return rv;
>+}
>+
>+NS_IMETHODIMP nsXFormsAccessible::GetChildCount(PRInt32 *aAccChildCount) 
>+{
>+  CacheChildren(PR_FALSE); // Don't walk anonymous children, normally PR_TRUE
>+  *aAccChildCount = mAccChildCount;
>+  return NS_OK;
>+}

You should test aAccChildCount before assigning into it.  NS_ENSURE_ARG_POINTER or 'if' test.

>+NS_IMETHODIMP nsXFormsBoundElementsAccessible::GetState(PRUint32 *aState)
>+{
>+  nsresult rv = nsAccessible::GetState(aState);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIXFormsAccessors> accessors;
>+  rv = GetXFormsAccessors(getter_AddRefs(accessors));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRBool value = PR_FALSE;
>+
>+  rv = accessors->IsRelevant(&value);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (value)
>+    *aState |= STATE_UNAVAILABLE;

per comment #15, shouldn't you only set STATE_UNAVAILABLE if !value?

>+
>+  rv = accessors->IsReadonly(&value);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (value)
>+    *aState |= STATE_READONLY;
>+
>+  rv = accessors->IsRequired(&value);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (value)
>+    *aState |= STATE_REQUIRED;
>+
>+  accessors->IsValid(&value);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!value)
>+    *aState |= STATE_INVALID;
>+
>+  return NS_OK;
>+}
>+
>+nsresult nsXFormsBoundElementsAccessible::GetXFormsAccessors(nsIXFormsAccessors **aAccessors)
>+{
>+  nsCOMPtr<nsIXFormsDelegate> delegate(do_QueryInterface(mDOMNode));
>+  if (!delegate)
>+    return NS_ERROR_FAILURE;

nit: I prefer NS_ENSURE_TRUE(delegate, NS_ERROR_FAILURE), but no biggie.


>+nsresult nsXFormsUICommonContentSetAccessible::GetChildValueByTagName(const nsAString& aTagName, nsAString &aValue)
>+{
>+  nsCOMPtr<nsIDOMElement> element(do_QueryInterface(mDOMNode));
>+  if (!element)
>+    return NS_ERROR_FAILURE;

same nit

>+
>+  nsCOMPtr<nsIDOMNodeList> nodes;
>+  nsresult rv = element->
>+    GetElementsByTagNameNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
>+                           aTagName, getter_AddRefs(nodes));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+

I think that the style used most often in Mozilla is to leave element->GetElementsByTagNameNS on the same line and move the parameters down a line if need be.

I also wonder if it wouldn't be better to do this by hand.  I might be wrong, but doesn't GetElementsByTagNameNS search multiple levels deep?  Wouldn't it be better to restrict the search to just the children directly under the element?

>Index: mozilla/accessible/src/xforms/nsXFormsFormControlsAccessible.h
>===================================================================
>RCS file: mozilla/accessible/src/xforms/nsXFormsFormControlsAccessible.h
>diff -N mozilla/accessible/src/xforms/nsXFormsFormControlsAccessible.h
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ mozilla/accessible/src/xforms/nsXFormsFormControlsAccessible.h	7 Jun 2006 15:11:07 -0000
>@@ -0,0 +1,69 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2006
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Alexander Surkov <[e-mail]> (original author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either of the GNU General Public License Version 2 or later (the "GPL"),
>+ * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#ifndef _nsXFormsFormControlsAccessible_H_
>+#define _nsXFormsFormControlsAccessible_H_
>+
>+#include "nsXFormsAccessible.h"
>+
>+/*
>+ * Accessible object for xforms:label
>+ */
>+
>+class nsXFormsLabelAccessible : public nsXFormsBoundElementsAccessible
>+{
>+public:
>+  nsXFormsLabelAccessible(nsIDOMNode *aNode, nsIWeakReference *aShell);
>+
>+  NS_IMETHOD GetRole(PRUint32 *aRole);
>+};
>+
>+/*
>+ * Accessible object for xforms:output
>+ */
>+
>+class nsXFormsOutputAccessible : public nsXFormsUICommonContentSetAccessible
>+{
>+public:
>+  nsXFormsOutputAccessible(nsIDOMNode *aNode, nsIWeakReference *aShell);
>+
>+  NS_IMETHOD GetRole(PRUint32 *aRole);
>+};
>+
>+#endif
>+

Wouldn't you want some kind of role value returned by all elements, even if statictext is only returned for these two controls?

r-'ing until we agree on the build issues, also the possible IsRelevant and GetChildValueByTagName problem.  Rest are just nits.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> (From update of attachment 224694 [details] [diff] [review] [edit])
> If I am reading these build changes correctly (which I may not be), you are
> going to force XForms to build by default UNLESS DISABLE_XFORMS_HOOKS is set? 
> Ewww, that would not be good.  Code surrounded by DISABLE_XFORMS_HOOKS should
> be able to compile and exist fine even if XForms isn't built.  Like in XPath. 
> XForms should only build because 'xforms' was listed as an extension in the
> .mozconfig.  Maybe you can make XForms utility service return what you need it
> to?

Yes, I assume if somebody wants to build mozilla without xforms then he should set DISABLE_XFORMS_HOOKS flag. It's bogus. In generall, I prefer to have other flag that will be setted if mozilla is builded with xforms i.e. I prefer to use #ifdef directive instead #ifndef. Can I have such flag?

I didn't get about XForms utility servcie. Can you say more?

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> (From update of attachment 224694 [details] [diff] [review] [edit])
> 
> nit: spelling.  Should be 'labeled', I believe
>

By "States and Adaptable Properties" docs (http://www.w3.org/WAI/PF/adaptable/StatesAndProperties-20051106.html) you're right.
But accessibility code contains labelledby. Aaronlev?

> I also wonder if it wouldn't be better to do this by hand.  I might be wrong,
> but doesn't GetElementsByTagNameNS search multiple levels deep?  Wouldn't it be
> better to restrict the search to just the children directly under the element?

Probably. Just I guess someone wants to put xf:label etc into some elmenets of host
document. Now implementation of some xforms controls assume xf:label should be direct child but I'm not sure what
it is right. Though I'll get the way you prefer.

> Wouldn't you want some kind of role value returned by all elements, even if
> statictext is only returned for these two controls?

I guess nsAccessible has it and xforms elmenets shouldn't have any special.

Comment 28

[aaronr]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 224694 [details] [diff] [review] [edit] [edit])
> > If I am reading these build changes correctly (which I may not be), you are
> > going to force XForms to build by default UNLESS DISABLE_XFORMS_HOOKS is set? 
> > Ewww, that would not be good.  Code surrounded by DISABLE_XFORMS_HOOKS should
> > be able to compile and exist fine even if XForms isn't built.  Like in XPath. 
> > XForms should only build because 'xforms' was listed as an extension in the
> > .mozconfig.  Maybe you can make XForms utility service return what you need it
> > to?
> 
> Yes, I assume if somebody wants to build mozilla without xforms then he should
> set DISABLE_XFORMS_HOOKS flag. It's bogus. In generall, I prefer to have other
> flag that will be setted if mozilla is builded with xforms i.e. I prefer to use
> #ifdef directive instead #ifndef. Can I have such flag?

Since we are just an extension, we can't put requirements like that on the core browser.  XForms should only build if it is in the extension list in the .mozconfig.  Also, sections of core code can contain code that is only executed by XForms (like XPath has), but it needs to have a VERY small performance impact when XForms is not installed and has to be surrounded by the DISABLE_XFORMS_HOOKS define so that embedders can remove the code to keep their executables as small as possible.

> 
> I didn't get about XForms utility servcie. Can you say more?
> 

XPath uses a service to access XForms specific things.  So that is one way to determine if XForms is available.  Also, that is a way to get XForms to do most, if not all, of the work that you need done.  For example, in your latest patch you get the xforms accessor and then you ask for values, relevance, readonly, etc.  Why not create a service (or use the utility service that already exists) with a method like nsXFormsUtilityService::IsRelevant(nsIDOMNode *aDOMNode) and then on the XForms side it determines whether it is relevant or not and return just a boolean back to the accessibility side.  In that way the accessibility code doesn't really need to know anything about XForms.  It asks the service for any information that it needs.  And if XForms (and thus the service) isn't there, the do_GetService fails and no harm done.

Comment 29

[alexander :surkov (:asurkov)]

(In reply to comment #28)

> XPath uses a service to access XForms specific things.  So that is one way to
> determine if XForms is available.  Also, that is a way to get XForms to do
> most, if not all, of the work that you need done.  For example, in your latest
> patch you get the xforms accessor and then you ask for values, relevance,
> readonly, etc.  Why not create a service (or use the utility service that
> already exists) with a method like
> nsXFormsUtilityService::IsRelevant(nsIDOMNode *aDOMNode) and then on the XForms
> side it determines whether it is relevant or not and return just a boolean back
> to the accessibility side.  In that way the accessibility code doesn't really
> need to know anything about XForms.  It asks the service for any information
> that it needs.  And if XForms (and thus the service) isn't there, the
> do_GetService fails and no harm done.
> 

If I get right then
1) I can extend nsIXFormsUtilityService defined in xpath module and then add dependency from xpath module for accessibility. I don't like that way because it's not obvious for code readers where that stuff is defined. It can confuse at the first sight.

2) I can add special utility service for accessibility module. I belive once we get that utility service for xpath and for accessibility duplicate each other. Again, it's not good.

3) Probably is xforms ready to be hosted in core browser? ;) It's very fine but I guess it's difficult to do it. Probably we can add to core browser restricted number files needed for other core browser modules.

Cartman, what do you think?

Comment 30

[Aaron Leventhal]

We used the English spelling "labelled" in DHTML a11y in order to be consistent with the ATK accessibility API. Didn't want to confuse people with 2 different spellings of it.

Comment 31

[aaronr]

(In reply to comment #29)
> (In reply to comment #28)
> 
> > XPath uses a service to access XForms specific things.  So that is one way to
> > determine if XForms is available.  Also, that is a way to get XForms to do
> > most, if not all, of the work that you need done.  For example, in your latest
> > patch you get the xforms accessor and then you ask for values, relevance,
> > readonly, etc.  Why not create a service (or use the utility service that
> > already exists) with a method like
> > nsXFormsUtilityService::IsRelevant(nsIDOMNode *aDOMNode) and then on the XForms
> > side it determines whether it is relevant or not and return just a boolean back
> > to the accessibility side.  In that way the accessibility code doesn't really
> > need to know anything about XForms.  It asks the service for any information
> > that it needs.  And if XForms (and thus the service) isn't there, the
> > do_GetService fails and no harm done.
> > 
> 
> If I get right then
> 1) I can extend nsIXFormsUtilityService defined in xpath module and then add
> dependency from xpath module for accessibility. I don't like that way because
> it's not obvious for code readers where that stuff is defined. It can confuse
> at the first sight.
> 
> 2) I can add special utility service for accessibility module. I belive once we
> get that utility service for xpath and for accessibility duplicate each other.
> Again, it's not good.
> 
> 3) Probably is xforms ready to be hosted in core browser? ;) It's very fine but
> I guess it's difficult to do it. Probably we can add to core browser restricted
> number files needed for other core browser modules.
> 
> Cartman, what do you think?
> 

I'd say that if we need this utility service to be used by multiple components, then we'll have to find some place to host it in the core.  Having multiple utility services is not ideal because I imagine before long there will be duplication of functionality, especially if we end up with a third component that wants/needs to tie into XForms.  But for right now there isn't an obvious 3rd user that I can think of, so maybe we can get by if we keep a clear seperation of fucntionality.

I honestly have no suggestions where to put it if we only have one service.  Maybe aaronL has an idea?  We can't have it live in XForms because then xpath and accessibility would be dependent on XForms which is a no go.  That is why I put it in XPath to begin with.  And whereever we end up putting it, we'd have to get the owner to agree to put it into the 1.8.1 and 1.8.0.x branches, too, and get XPath and accessibility to agree to require it.  Ugh!

Comment 32

[alexander :surkov (:asurkov)]

(In reply to comment #31)

> I'd say that if we need this utility service to be used by multiple components,
> then we'll have to find some place to host it in the core.  Having multiple
> utility services is not ideal because I imagine before long there will be
> duplication of functionality, especially if we end up with a third component
> that wants/needs to tie into XForms.  But for right now there isn't an obvious
> 3rd user that I can think of, so maybe we can get by if we keep a clear
> seperation of fucntionality.
> 
> I honestly have no suggestions where to put it if we only have one service. 
> Maybe aaronL has an idea?  We can't have it live in XForms because then xpath
> and accessibility would be dependent on XForms which is a no go.  That is why I
> put it in XPath to begin with.  And whereever we end up putting it, we'd have
> to get the owner to agree to put it into the 1.8.1 and 1.8.0.x branches, too,
> and get XPath and accessibility to agree to require it.  Ugh!
> 

If we will find a place in browser core to put utility service then probably we can put there public idl files of xforms. It helps our to forget about utility service (at least for accessibility). Any thoughts?

Comment 33

[aaronr]

(In reply to comment #32)
> (In reply to comment #31)
> 
> > I'd say that if we need this utility service to be used by multiple components,
> > then we'll have to find some place to host it in the core.  Having multiple
> > utility services is not ideal because I imagine before long there will be
> > duplication of functionality, especially if we end up with a third component
> > that wants/needs to tie into XForms.  But for right now there isn't an obvious
> > 3rd user that I can think of, so maybe we can get by if we keep a clear
> > seperation of fucntionality.
> > 
> > I honestly have no suggestions where to put it if we only have one service. 
> > Maybe aaronL has an idea?  We can't have it live in XForms because then xpath
> > and accessibility would be dependent on XForms which is a no go.  That is why I
> > put it in XPath to begin with.  And whereever we end up putting it, we'd have
> > to get the owner to agree to put it into the 1.8.1 and 1.8.0.x branches, too,
> > and get XPath and accessibility to agree to require it.  Ugh!
> > 
> 
> If we will find a place in browser core to put utility service then probably we
> can put there public idl files of xforms. It helps our to forget about utility
> service (at least for accessibility). Any thoughts?
> 

The less we change the core, the better.  We need to keep in mind that all of our current flexibility stems from the fact that we are still an extension.

Comment 34

[Aaron Leventhal]

I think the right approach is to move some of the IDL to the core, on trunk.
Something like content/xforms/public or dom/xforms/public

It's not a requirement for us to make Xforms accessible on the 1.8 branch. Is it really necessary to move the idl to the core on branches too? Or can the xforms code just be built to look for it's headers/idl in the right place for the current branch?

Comment 35

[aaronr]

(In reply to comment #34)
> I think the right approach is to move some of the IDL to the core, on trunk.
> Something like content/xforms/public or dom/xforms/public
> 
> It's not a requirement for us to make Xforms accessible on the 1.8 branch. Is
> it really necessary to move the idl to the core on branches too? Or can the
> xforms code just be built to look for it's headers/idl in the right place for
> the current branch?
> 

Just like anything else, the more differences between trunk and branch, the harder it is to maintain.

What do you want to move to the core and why can't they stay in XForms?

Comment 36

[Aaron Leventhal]

If you keep the idl in xforms then how will the built mozilla/accessible code know whether the version of the idl it's compiled with is the same as the extension currently being used? If that's not a concern then keep it in xforms.

Comment 37

[aaronr]

If it is an idl that needs to be use, like a service idl (something that the xforms implementation doesn't really use internally but rather is a way to interact with the xforms processor) then that's fine.  I meant that I saw no reason to move any of the other idl's to the core.

Comment 38

[Peter Van der Beken [:peterv]]

Note: as part of the fix for bug 278981 I removed all methods of nsIXFormsUtilityService and nsXFormsUtilityService because nothing uses them anymore. The code was mostly moved into nsXFormsUtils, so if/when they become needed outside of the XForms module you can just forward from nsXFormsUtilityService to nsXFormsUtils.

Comment 39

[alexander :surkov (:asurkov)]

(In reply to comment #38)
> Note: as part of the fix for bug 278981 I removed all methods of
> nsIXFormsUtilityService and nsXFormsUtilityService because nothing uses them
> anymore. The code was mostly moved into nsXFormsUtils, so if/when they become
> needed outside of the XForms module you can just forward from
> nsXFormsUtilityService to nsXFormsUtils.
> 

Did you remove nsXFormsUtilityService? But why didn't you remove its methods only?

Comment 40

[alexander :surkov (:asurkov)]

Attachment: class diagram

Forms accessible class diagram. Comments are welcome.

Comment 41

[aaronr]

(In reply to comment #40)
> Created an attachment (id=230720) [edit]
> class diagram
> 
> XForms accessible class diagram. Comments are welcome.
> 

I'm not a great UML person, but after looking at the diagram, this is what I'm seeing:

-- You have a 'value' for nsXFormsItemElement.  But what if the 'value' isn't a simple string but is really complex content.  Or even tougher, a string followed by complex content (since an item can be built from a itemset that can contain a copy element).  Would such an item have no value?

-- Your comment for triggers and submit elements is that they do not use MIP's.  But they do.  That is one of the big values to putting binding attrs on a submit or trigger.  Well, that and providing context for the label, actions, etc. that they might contain.  Relevancy is the only MIP really used with them, though.  Maybe knowing the type will be valuable at some point in the future.  Also, why does trigger or submit need a 'value'?

-- I agree that having a 'value' on secret might not be a good idea.  Maybe just an indicator if the field is empty or not.

-- output doesn't need 'value'?  Or is it just picking it up from nsXFormsBoundElement?

-- select1 and range aren't mentioned.  I can see select1 falling under select, but what about range?  Wouldn't you want to be able to reflect the upper and lower bounds if nothing else?

-- do you need anything for inline alerts?  Nothing popping up for them, remember.

-- special handling for input when it is bound to different types?  Like when it is a checkbox or a date picker?

Comment 42

[alexander :surkov (:asurkov)]

(In reply to comment #41)

Thank you for good questions and pointing me to problems. Sometimes it's needed outer sight to complete the picture. I'm not big UML guy but UML tools allow to show class dependencies easy. Here I use it only.

> -- You have a 'value' for nsXFormsItemElement.  But what if the 'value' isn't a
> simple string but is really complex content.  Or even tougher, a string
> followed by complex content (since an item can be built from a itemset that can
> contain a copy element).  Would such an item have no value?

It's good question and I really don't know what behaviour should be. AaronL probably you can help here? Some context info for AaronL below: xforms:item is analogy of html:option element. html:option has @value attribute to specify item value, xforms:item has xforms:value or xforms:copy elements for that. If xforms:value is used then value of item will be textnode. If xforms:copy is used then value of item will be node. What should we do in case of xforms:copy? Probably should we use nsIDOM3Node::textContent attribute?

> -- Your comment for triggers and submit elements is that they do not use MIP's.
>  But they do.  That is one of the big values to putting binding attrs on a
> submit or trigger.  Well, that and providing context for the label, actions,
> etc. that they might contain.  Relevancy is the only MIP really used with them,
> though.

I missed that. Does trigger use relevant only? At least, I guess it doesn't use readonly property. Or?

> Also, why does trigger or submit need a 'value'?

Only because trigger accessible object is inherited from nsBoundElmAcc class that provides value. Trigger should abolish it.

> 
> -- I agree that having a 'value' on secret might not be a good idea.  Maybe
> just an indicator if the field is empty or not.

Yes, here I need figure out how xhtml:input type="password" is supported.

> -- output doesn't need 'value'?  Or is it just picking it up from
> nsXFormsBoundElement?

Yes, it inherits it from nsXFormsBoundElement. Output I guess don't need in special value like input needs. Btw, label takes value from that object too.

> 
> -- select1 and range aren't mentioned.  I can see select1 falling under select,
> but what about range?  Wouldn't you want to be able to reflect the upper and
> lower bounds if nothing else?

I forgot to mention select1 in nsXFSelectElmAcc and I forgot about range at all. Yes, for sure I think range should have accessible object (bug 109215 is for next of it's kin).

> -- do you need anything for inline alerts?  Nothing popping up for them,
> remember.

Btw, what is it inline alerts and what is it non inline alerts?

> -- special handling for input when it is bound to different types?  Like when
> it is a checkbox or a date picker?
> 

Right, totally agree.

Comment 43

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 44

[alexander :surkov (:asurkov)]

I have a question.

nsXFormsBoundElementAccessible class provides value and state properties for xforms elements that are bound to instance node.

nsXFormsUICommonSetAccessible class provides name and description properties for xforms elements that can have xf:label and xf:hint elements.

xf:label should use nsXFormsBoundElementAccessible.
xf:output, xf:input and others should use both nsXFormsBoundElementAccessible and nsXFormsUICommonSetAccessible.

xf:item and xf:choises should use nsXFormsUICommonSetAccessible only.

Can I use multiple inheritance? For example, let classes above is "almost virual".

class nsXFormsAccessible: nsAccessibleWrap {
public:
  NS_IMETHOD GetName();
  NS_IMETHOD GetDescription();
};

class nsXFormsBoundElementAccessible: nsIAccessible {
public:
  NS_IMETHOD GetValue();
  NS_IMETHOD GetState();
};

class nsXFormsUICommonSetAccessible: nsIAccessible {
public:
  NS_IMETHOD GetName();
  NS_IMETHOD GetDescription();
};

class nsXFormsLabelElementAccessible: public nsXFormsAccessible, nsXFormsBoundElementAccessible
{
}

class nsXFormsInputElementAccessible: public nsXFormsAccessible, nsXFormsBoundElementAccessible, nsXFormsUICommonSetAccessible
{
}

class nsXFormsItemElementAccessible: public nsXFormsAccessible, nsXFormsUICommonSetAccessible
{
}

Will it work?

Comment 45

[alexander :surkov (:asurkov)]

I missed that nsXFormsBoundElementAccessible and nsXFormsUICommonSetAccessible can't be inherited from nsIAccessible interface. Please look at modified example. Will it work?

class nsXFormsAccessible: nsAccessibleWrap {
public:
  NS_IMETHOD GetName();
  NS_IMETHOD GetDescription();
};

class nsXFormsBoundElementAccessible: nsXFormsAccessible {
public:
  NS_IMETHOD GetValue();
  NS_IMETHOD GetState();
};

class nsXFormsUICommonSetAccessible: nsXFormsAccessible {
public:
  NS_IMETHOD GetName();
  NS_IMETHOD GetDescription();
};

class nsXFormsLabelElementAccessible: public nsXFormsBoundElementAccessible
{
}

class nsXFormsInputElementAccessible: public nsXFormsBoundElementAccessible, nsXFormsUICommonSetAccessible
{
}

class nsXFormsItemElementAccessible: public nsXFormsUICommonSetAccessible
{
}

Comment 46

[Aaron Leventhal]

I don't recommend using multiple inheritance. I admit, I don't remember what the problem was with that but last time I tried it there was no luck. Perhaps it was conflicts in knowing which nsIAccessible QI's would succeed to. If you know more than I can you can try it.

However, I think another approach is to have a smart base class with supports state, value, name and description and can determine it's appropriate to supports those from within the various methods.

Comment 47

[alexander :surkov (:asurkov)]

Comment on attachment 232141 [details] [diff] [review]
patch4

(In reply to comment #46)
> I don't recommend using multiple inheritance. I admit, I don't remember what
> the problem was with that but last time I tried it there was no luck. Perhaps
> it was conflicts in knowing which nsIAccessible QI's would succeed to. If you
> know more than I can you can try it.

> However, I think another approach is to have a smart base class with supports
> state, value, name and description and can determine it's appropriate to
> supports those from within the various methods.
> 

Never mind, I will use smart base class approach. I cancel review request to supprot that.

Comment 48

[Aaron Leventhal]

> -- You have a 'value' for nsXFormsItemElement.  But what if the 'value' isn't 
> a simple string but is really complex content.  Or even tougher, a string
> followed by complex content (since an item can be built from a itemset that can contain a copy element).  Would such an item have no value?
Can someone attach a code example of that?

> -- I agree that having a 'value' on secret might not be a good idea.  Maybe
> just an indicator if the field is empty or not.
We expose STATE_PROTECTED for those. The accessible value or string, which can be different from the XForms value is just "*****" depending on the number of characters in there.

> but what about range?  Wouldn't you want to be able to reflect the upper and
> lower bounds if nothing else?
Range should support nsIAccessibleValue interface which allows upper and lower bounds.

> -- do you need anything for inline alerts?  Nothing popping up for them,
> remember.
There is a way to handle those by assigning ROLE_ALERT to the role. As soon as they are made visible we fire an EVENT_ALERT for them. That causes the screen reader to automatically read them.

Comment 49

[alexander :surkov (:asurkov)]

Attachment: xf:copy example

(In reply to comment #48)
> > -- You have a 'value' for nsXFormsItemElement.  But what if the 'value' isn't 
> > a simple string but is really complex content.  Or even tougher, a string
> > followed by complex content (since an item can be built from a itemset that can contain a copy element).  Would such an item have no value?
> Can someone attach a code example of that?

xf:copy example.

Comment 50

[aaronr]

I don't know if these will present a problem or not.  I guess it depends on how you will get the values.  Just keep in mind that xf:label and xf:value can contain xf:output's.  According to the spec, xf:value can contain almost anything and be valid, but xf:output is the only thing I've ever heard of as being used as a child of xf:value.

Comment 51

[Aaron Leventhal]

(In reply to comment #50)
> I don't know if these will present a problem or not.  I guess it depends on how
> you will get the values.  Just keep in mind that xf:label and xf:value can
> contain xf:output's.  According to the spec, xf:value can contain almost
> anything and be valid, but xf:output is the only thing I've ever heard of as
> being used as a child of xf:value.
> 
As long as the label is rendered and has an accessible subtree we can point to it using the labelledby accessible relation. The accessible name can be an attempt to stringize it. The AT sees the item get focus and checks both, then decides whether to use the subtree pointed to by the relation or the accessible name.

As far as value we don't have anything like a valuefor relation.

Comment 52

[alexander :surkov (:asurkov)]

Attachment: patch5

Comment 53

[Aaron Leventhal]

Comment on attachment 232926 [details] [diff] [review]
patch5

I have one main comment, but this patch looks very good:
Instead of putting the common code for GetState() in GetBoundElementState you could just put it in nsXFormsAccessible::GetState() and upcall when appropriate (or don't override).

The same comment applies to the others methods like GetName(). You can put that code in nsXFormsAccessible::GetName() etc.

Nit: if nsIXFormsUtilityService was defined in idl instead, you'd be able to use NS_DECL_NSIXFORMSUTILITYSERVICE rather than repeating the method declarations again in the class header file. That's your choice.

Nit: In nsXFormsAccessible.cpp, this comment seems to come too late:
// nsXFormsAccessible
(In fact I would expect that static definiton to be moved up to the top as well).

I'll plus this and trust that you'll move the GetState(), GetName() etc. code into base impls for those methods and remove the extra method names, if it makes sense to you.

Please ask for sr= on this patch from someone you want to vouch for you -- that will help us get you cvs write access eventually.

Comment 54

[alexander :surkov (:asurkov)]

(In reply to comment #53)
> (From update of attachment 232926 [details] [diff] [review] [edit])
> I have one main comment, but this patch looks very good:
> Instead of putting the common code for GetState() in GetBoundElementState you
> could just put it in nsXFormsAccessible::GetState() and upcall when appropriate
> (or don't override).
> 
> The same comment applies to the others methods like GetName(). You can put that
> code in nsXFormsAccessible::GetName() etc.

That is good question for me. I suppose xforms has 17 accessible elements. So:

1) GetState()/GetValue() methods. These methods returns 'state' and 'value' values based on node state that element is bound to. Only choices and item elements can't be bound to instance node.
2) GetName() method. The method return 'name' for those elements that can contain label element. Only label, hint, help, alert, message elements can't have label element.
3) GetDescription() method. The method return 'name' for those elements that can contain label element. Only label, hint, help, alert, message can't have hint element.

So, I can put GetState() and etc in nsXFormsAccessible. Just I should rewrite them for some controls listed above. AaronLev, is it that thing you talked about?

Comment 55

[Aaron Leventhal]

Yes, if you want to submit a new patch I'll look at it.

Comment 56

[alexander :surkov (:asurkov)]

Attachment: patch6

> Nit: if nsIXFormsUtilityService was defined in idl instead, you'd be able to
> use NS_DECL_NSIXFORMSUTILITYSERVICE rather than repeating the method
> declarations again in the class header file. That's your choice.

Yes I know but it was not my choice :)

I refactor GetValue() and etc methods.

There is one more issue. It looks like GetAllowsAnonChildAccessibles() never called for accessible objects inherited from nsAccessible.

Comment 57

[Aaron Leventhal]

Comment on attachment 233824 [details] [diff] [review]
patch6

Looks good. One other thing is that instead of checking if nsIDOMElement's localName=="submit" you should use nsIContent, and go content->NodeInfo()->IsEqual()
We put our atoms in nsAccessibilityAtomList.h

Comment 58

[alexander :surkov (:asurkov)]

Attachment: patch7

(In reply to comment #57)
> (From update of attachment 233824 [details] [diff] [review] [edit])
> Looks good. One other thing is that instead of checking if nsIDOMElement's
> localName=="submit" you should use nsIContent, and go
> content->NodeInfo()->IsEqual()
> We put our atoms in nsAccessibilityAtomList.h
> 

Fixed.

(In reply to comment #53)
> 
> Please ask for sr= on this patch from someone you want to vouch for you -- that
> will help us get you cvs write access eventually.
> 

Boris, have you enough time to make super review?

Comment 59

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 233836 [details] [diff] [review]
patch7

I won't be able to get to this in the foreseeable future.  Please ask a different sr.

Comment 60

[alexander :surkov (:asurkov)]

Comment on attachment 233836 [details] [diff] [review]
patch7

Neil, do you have a time for sr in nearest future?

Comment 61

[[e-mail]]

Comment on attachment 233836 [details] [diff] [review]
patch7

sr=me with these nits fixed:

>+  XFormsService->IsValid(mDOMNode, &value);
>+  NS_ENSURE_SUCCESS(rv, rv);
You forgot to set rv here.

>+  nsAutoString tagname;
Unused?

>+NS_IMETHODIMP
>+nsXFormsUtilityService::GetXFormsNamespace(nsAString& aNamespace)
>+{
>+  aNamespace.AssignLiteral(NS_NAMESPACE_XFORMS);
>+  return NS_OK;
>+}
What's the deal with this? Can't you just #define the namespace somwhere?

Comment 62

[alexander :surkov (:asurkov)]

(In reply to comment #61)
> >+NS_IMETHODIMP
> >+nsXFormsUtilityService::GetXFormsNamespace(nsAString& aNamespace)
> >+{
> >+  aNamespace.AssignLiteral(NS_NAMESPACE_XFORMS);
> >+  return NS_OK;
> >+}
> What's the deal with this? Can't you just #define the namespace somwhere?
> 

I just like if namespace will be defined in one place. If xforms extension changes namespace for some reasons then accessible code will work.

Comment 63

[alexander :surkov (:asurkov)]

Attachment: patch8

> I just like if namespace will be defined in one place. If xforms extension
> changes namespace for some reasons then accessible code will work.
> 

I didn't fix neil's comment about getting xforms namespace. I talk with Neil and we decide to leave this issue for xforms reviewer.

Comment 64

[aaronr]

Comment on attachment 234016 [details] [diff] [review]
patch8


>Index: accessible/src/xforms/nsXFormsAccessible.cpp
>===================================================================
>RCS file: accessible/src/xforms/nsXFormsAccessible.cpp
>diff -N accessible/src/xforms/nsXFormsAccessible.cpp
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ accessible/src/xforms/nsXFormsAccessible.cpp	16 Aug 2006 16:09:19 -0000
>@@ -0,0 +1,186 @@

>+// nsXFormsAccessible
>+
>+nsIXFormsUtilityService *nsXFormsAccessible::sXFormsService = nsnull;
>+
>+nsIXFormsUtilityService *nsXFormsAccessible::GetXFormsService()
>+{
>+  if (!sXFormsService) {
>+    nsresult rv = CallGetService("@mozilla.org/xforms-utility-service;1",
>+                                 &sXFormsService);
>+    NS_ASSERTION(NS_SUCCEEDED(rv), "No XForms utility service.");
>+  }
>+
>+  return sXFormsService;
>+}

I don't know if you need an assertion here.  It is perfectly ok if the xforms extension isn't on someone's machine.  Or is GetXFormsService only called when the service HAS to be available?  If not, NS_WARNING might be more appropriate.

Also, couldn't you just do this test in the constructor and after that test the static (or a member variable) directly?  That would eliminate the need for a function call everytime.

>+
>+nsXFormsAccessible::
>+nsXFormsAccessible(nsIDOMNode* aNode, nsIWeakReference* aShell):
>+  nsAccessibleWrap(aNode, aShell)
>+{
>+}
>+
>+nsresult
>+nsXFormsAccessible::GetBoundChildElementValue(const nsAString& aTagName,
>+                                              nsAString &aValue)

nit: signatures.  Try to keep them consistent throughout the file...'&' right after the type or right before the variable?  Most of yours in this file seem to be right after the type.

>+{
>+  nsIXFormsUtilityService *XFormsService = GetXFormsService();
>+  NS_ENSURE_TRUE(XFormsService, NS_ERROR_FAILURE);
>+
>+  nsCOMPtr<nsIDOMElement> element(do_QueryInterface(mDOMNode));
>+  NS_ENSURE_TRUE(element, NS_ERROR_FAILURE);
>+
>+  nsAutoString XFormsNS;
>+  XFormsService->GetXFormsNamespace(XFormsNS);
>+
>+  nsCOMPtr<nsIDOMNodeList> nodes;
>+  nsresult rv = element->GetElementsByTagNameNS(XFormsNS, aTagName,
>+                                                getter_AddRefs(nodes));
>+  NS_ENSURE_SUCCESS(rv, rv);

I don't think that you want GetElementsByTagNameNS.  This will search more than just the immediate children.  It will search all of the descendants, too.  Which will screw you up if a form author leaves off a label or hint element but there is one further down.  You might be better off just doing this by going to firstChild and walking the nextSibling list looking for them.

>+
>+  PRUint32 length;
>+  rv = nodes->GetLength(&length);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (length) {
>+    nsCOMPtr<nsIDOMNode> node;
>+    rv = nodes->Item(0, getter_AddRefs(node));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    return XFormsService->GetValue(node, aValue);
>+  }
>+
>+  return NS_OK;
>+}
>+
>+// nsIAccessible
>+
>+NS_IMETHODIMP
>+nsXFormsAccessible::GetValue(nsAString& aValue)
>+{
>+  nsIXFormsUtilityService *XFormsService = GetXFormsService();
>+  NS_ENSURE_TRUE(XFormsService, NS_ERROR_FAILURE);
>+
>+  return XFormsService->GetValue(mDOMNode, aValue);
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsAccessible::GetState(PRUint32 *aState)
>+{
>+  NS_ENSURE_ARG_POINTER(aState);
>+
>+  nsIXFormsUtilityService *XFormsService = GetXFormsService();
>+  NS_ENSURE_TRUE(XFormsService, NS_ERROR_FAILURE);
>+
>+  nsresult rv = nsAccessible::GetState(aState);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRBool value = PR_FALSE;
>+
>+  rv = XFormsService->IsRelevant(mDOMNode, &value);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (value)
>+    *aState |= STATE_UNAVAILABLE;
>+
>+  rv = XFormsService->IsReadonly(mDOMNode, &value);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (value)
>+    *aState |= STATE_READONLY;
>+
>+  rv = XFormsService->IsRequired(mDOMNode, &value);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (value)
>+    *aState |= STATE_REQUIRED;
>+
>+  rv = XFormsService->IsValid(mDOMNode, &value);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!value)
>+    *aState |= STATE_INVALID;
>+
>+  return NS_OK;
>+}

For GetState, GetName and GetDescription, and some other functions that you wrote...if you are going to return an error, don't you want to make sure to empty out the return buffer first?  Like if IsRequired failed above, shouldn't you zero out the aState buffer instead of returning a partially filled out aState?  Or zero it out to begin with and store the states in a local variable until you know that they all succeeded and then fill in aState and return NS_OK all at once?

>+
>+NS_IMETHODIMP
>+nsXFormsAccessible::GetName(nsAString& aName)
>+{
>+  nsAutoString name;
>+  nsresult rv = GetTextFromRelationID(nsAccessibilityAtoms::labelledby, name);
>+  if (NS_SUCCEEDED(rv) && !name.IsEmpty()) {
>+    aName = name;
>+    return NS_OK;
>+  }
>+
>+  // search the xforms:label element
>+  return GetBoundChildElementValue(NS_LITERAL_STRING("label"), aName);
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsAccessible::GetDescription(nsAString& aDescription)
>+{
>+  nsAutoString description;
>+  nsresult rv = GetTextFromRelationID(nsAccessibilityAtoms::describedby,
>+                                      description);
>+
>+  if (NS_SUCCEEDED(rv) && !description.IsEmpty()) {
>+    aDescription = description;
>+    return NS_OK;
>+  }
>+
>+  // search the xforms:hint element
>+  return GetBoundChildElementValue(NS_LITERAL_STRING("hint"), aDescription);
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsAccessible::GetAllowsAnonChildAccessibles(PRBool *aAllowsAnonChildren)
>+{
>+  NS_ENSURE_ARG_POINTER(aAllowsAnonChildren);
>+
>+  *aAllowsAnonChildren = PR_FALSE;
>+  return NS_OK;
>+}
>+
>Index: accessible/src/xforms/nsXFormsAccessible.h
>===================================================================
>RCS file: accessible/src/xforms/nsXFormsAccessible.h
>diff -N accessible/src/xforms/nsXFormsAccessible.h
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ accessible/src/xforms/nsXFormsAccessible.h	16 Aug 2006 16:09:19 -0000
>@@ -0,0 +1,84 @@

>+class nsXFormsAccessible : public nsAccessibleWrap
>+{
>+public:
>+  nsXFormsAccessible(nsIDOMNode* aNode, nsIWeakReference* aShell);
>+
>+  // Returns value of instance node that xforms element is bound to.
>+  NS_IMETHOD GetValue(nsAString& aValue);
>+
>+  // Returns state of xforms element taking into account state of instance node
>+  // that it is bound to.
>+  NS_IMETHOD GetState(PRUint32 *aState);
>+
>+  // Returns value of child xforms 'label' element.
>+  NS_IMETHOD GetName(nsAString& aName);
>+
>+  // Returns value of child xforms 'hint' element.
>+  NS_IMETHOD GetDescription(nsAString& aDescription);
>+
>+  // Denides accessible nodes inside xforms element.
>+  NS_IMETHOD GetAllowsAnonChildAccessibles(PRBool *aAllowsAnonChildren);
>+

nit: spelling on 'Denides'.  Also, could you explain this function a little better?  From the description I don't really understand what it does or why we are always returning PR_FALSE

>Index: accessible/src/xforms/nsXFormsFormControlsAccessible.cpp
>===================================================================
>RCS file: accessible/src/xforms/nsXFormsFormControlsAccessible.cpp
>diff -N accessible/src/xforms/nsXFormsFormControlsAccessible.cpp
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ accessible/src/xforms/nsXFormsFormControlsAccessible.cpp	16 Aug 2006 16:09:19 -0000
>@@ -0,0 +1,163 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2006
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Alexander Surkov <[e-mail]> (original author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either of the GNU General Public License Version 2 or later (the "GPL"),
>+ * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#include "nsXFormsFormControlsAccessible.h"
>+
>+// nsXFormsLabelAccessible
>+
>+nsXFormsLabelAccessible::
>+  nsXFormsLabelAccessible(nsIDOMNode *aNode, nsIWeakReference *aShell):
>+  nsXFormsAccessible(aNode, aShell)
>+{
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsLabelAccessible::GetRole(PRUint32 *aRole)
>+{
>+  NS_ENSURE_ARG_POINTER(aRole);
>+
>+  *aRole = ROLE_STATICTEXT;
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsLabelAccessible::GetName(nsAString& aName)
>+{
>+  nsAutoString name;
>+  nsresult rv = GetTextFromRelationID(nsAccessibilityAtoms::labelledby, name);
>+  aName = name;
>+  return rv;
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsLabelAccessible::GetDescription(nsAString& aDescription)
>+{
>+  nsAutoString description;
>+  nsresult rv = GetTextFromRelationID(nsAccessibilityAtoms::describedby,
>+                                      description);
>+  aDescription = description;
>+  return rv;
>+}
>+
>+// nsXFormsOutputAccessible
>+
>+nsXFormsOutputAccessible::
>+  nsXFormsOutputAccessible(nsIDOMNode *aNode, nsIWeakReference *aShell):
>+  nsXFormsAccessible(aNode, aShell)
>+{
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsOutputAccessible::GetRole(PRUint32 *aRole)
>+{
>+  NS_ENSURE_ARG_POINTER(aRole);
>+
>+  *aRole = ROLE_STATICTEXT;
>+  return NS_OK;
>+}
>+
>+// nsXFormsTriggerAccessible
>+
>+nsXFormsTriggerAccessible::
>+  nsXFormsTriggerAccessible(nsIDOMNode *aNode, nsIWeakReference *aShell):
>+  nsXFormsAccessible(aNode, aShell)
>+{
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsTriggerAccessible::GetRole(PRUint32 *aRole)
>+{
>+  NS_ENSURE_ARG_POINTER(aRole);
>+
>+  *aRole = ROLE_PUSHBUTTON;
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsTriggerAccessible::GetState(PRUint32 *aState)
>+{
>+  NS_ENSURE_ARG_POINTER(aState);
>+
>+  nsresult rv = nsXFormsAccessible::GetState(aState);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>+  NS_ENSURE_STATE(content);
>+
>+  if (content->NodeInfo()->Equals(nsAccessibilityAtoms::submit)) {
>+    *aState |= STATE_DEFAULT;
>+  }

Could you put a comment in trigger GetState to explain why a submit trigger always returns STATE_DEFAULT but a regular trigger doesn't?

>Index: content/base/public/nsIXFormsUtilityService.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/public/nsIXFormsUtilityService.h,v
>retrieving revision 1.2
>diff -u -8 -p -r1.2 nsIXFormsUtilityService.h
>--- content/base/public/nsIXFormsUtilityService.h	13 Jul 2006 14:21:52 -0000	1.2
>+++ content/base/public/nsIXFormsUtilityService.h	16 Aug 2006 16:09:19 -0000
>@@ -34,18 +34,18 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #ifndef nsIXFormsUtilityService_h
> #define nsIXFormsUtilityService_h
> 
>+#include "nsIDOMNode.h"
> 
>-#include "nsISupports.h"
> 
> /* For IDL files that don't want to include root IDL files. */
> #ifndef NS_NO_VTABLE
> #define NS_NO_VTABLE
> #endif
> 
> /* nsIXFormsUtilityService */
> #define NS_IXFORMSUTILITYSERVICE_IID_STR "f7276415-bb3e-4170-b746-aa57f68d7006"
>@@ -56,14 +56,45 @@
> /**
>  * Private interface implemented by the nsXFormsUtilityService in XForms
>  * extension.
>  */
> class NS_NO_VTABLE nsIXFormsUtilityService : public nsISupports {
> public:
> 
>   NS_DECLARE_STATIC_IID_ACCESSOR(NS_IXFORMSUTILITYSERVICE_IID)
>+
>+  /**
>+   * Return XForms namespace.
>+   */
>+  NS_IMETHOD GetXFormsNamespace(nsAString& aNamespace) = 0;
>+

I personally don't see why you would need this.  The XForms namespace for version 1.0 can't change, it's already been documented by the W3C in two different editions.  If they change it now, it would bust a lot of forms.  I think it would be safe to #define it somewhere that you can get to it.  Last I heard XForms 1.1 wasn't going to have a different namespace either.  Unless you are anticipating XForms 2.0 (probably 3 years away), I'd say you can do without this function.

I could potentially see a function for getting the XForms version of a form (1.0 or 1.1 in the near future), but until the W3C formally publishes how that will be designated, you can probably just ignore it for now.

>+  /**
>+   * Return true if instance node that element is bound to is readonly.
>+   */
>+  NS_IMETHOD IsReadonly(nsIDOMNode *aElement, PRBool *aState) = 0;
>+
>+  /**
>+   * Return true if instance node that element is bound to is relevant.
>+   */
>+  NS_IMETHOD IsRelevant(nsIDOMNode *aElement, PRBool *aState) = 0;
>+
>+  /**
>+   * Return true if instance node that element is bound to is required.
>+   */
>+  NS_IMETHOD IsRequired(nsIDOMNode *aElement, PRBool *aState) = 0;
>+
>+  /**
>+   * Return true if instance node that element is bound to is valid.
>+   */
>+  NS_IMETHOD IsValid(nsIDOMNode *aElement, PRBool *aState) = 0;
>+
>+  /**
>+   * Return value of instance node that element is bound to.
>+   */
>+  NS_IMETHOD GetValue(nsIDOMNode *aElement, nsAString& aValue) = 0;
>+
> };
> 
> NS_DEFINE_STATIC_IID_ACCESSOR(nsIXFormsUtilityService,
>                               NS_IXFORMSUTILITYSERVICE_IID)
> 
> #endif /* nsIXFormsUtilityService_h */
>Index: extensions/xforms/nsXFormsUtilityService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtilityService.cpp,v
>retrieving revision 1.10
>diff -u -8 -p -r1.10 nsXFormsUtilityService.cpp
>--- extensions/xforms/nsXFormsUtilityService.cpp	13 Jul 2006 14:21:52 -0000	1.10
>+++ extensions/xforms/nsXFormsUtilityService.cpp	16 Aug 2006 16:09:19 -0000
>@@ -15,17 +15,18 @@
>  * The Original Code is Mozilla XForms support.
>  *
>  * The Initial Developer of the Original Code is
>  * IBM Corporation.
>  * Portions created by the Initial Developer are Copyright (C) 2004
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>- *  Aaron Reed <[e-mail]>
>+ *  Aaron Reed <[e-mail]> (original author)
>+ *  Alexander Surkov <[e-mail]>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either the GNU General Public License Version 2 or later (the "GPL"), or
>  * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>  * in which case the provisions of the GPL or the LGPL are applicable instead
>  * of those above. If you wish to allow use of your version of this file only
>  * under the terms of either the GPL or the LGPL, and not to allow others to
>  * use your version of this file under the terms of the MPL, indicate your
>@@ -33,9 +34,72 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsXFormsUtilityService.h"
> 
>+#include "nsIXFormsDelegate.h"
>+#include "nsIXFormsAccessors.h"
>+#include "nsXFormsUtils.h"
>+
> NS_IMPL_ISUPPORTS1(nsXFormsUtilityService, nsIXFormsUtilityService)
>+
>+#define GET_XFORMS_ACCESSORS \
>+NS_ENSURE_ARG_POINTER(aElement);\
>+nsCOMPtr<nsIXFormsDelegate> delegate(do_QueryInterface(aElement));\
>+NS_ENSURE_TRUE(delegate, NS_ERROR_FAILURE);\
>+nsCOMPtr<nsIXFormsAccessors> accessors;\
>+delegate->GetXFormsAccessors(getter_AddRefs(accessors));\
>+NS_ENSURE_TRUE(accessors, NS_ERROR_FAILURE);
>+
>+NS_IMETHODIMP
>+nsXFormsUtilityService::GetXFormsNamespace(nsAString& aNamespace)
>+{
>+  aNamespace.AssignLiteral(NS_NAMESPACE_XFORMS);
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsUtilityService::IsReadonly(nsIDOMNode *aElement, PRBool *aState)
>+{
>+  NS_ENSURE_ARG_POINTER(aState);
>+
>+  GET_XFORMS_ACCESSORS
>+  return accessors->IsReadonly(aState);
>+}
>+

nit: just a personal preference, but I think the initializer macro (GET_XFORMS_ACCESSORS) should be up next to the NS_ENSURE_ARG, with the newline between the macro and the body of the function rather than the newline between the NS_ENSURE_ARG and the macro.  But that is just me.  If you like it where it is, it isn't that big of a deal to me.

none of the changes I mentioned above were very radical and probably don't need further review.  with those changes, r=me

Comment 65

[aaronr]

Just thought of something...should nsXFormsAccessible::GetState have a 'out of range' state?  Or is that something that you are going to add later when you add support for select, select1 and range?

Comment 66

[alexander :surkov (:asurkov)]

(In reply to comment #65)
> Just thought of something...should nsXFormsAccessible::GetState have a 'out of
> range' state?  Or is that something that you are going to add later when you
> add support for select, select1 and range?
> 

Yes, let leave it for select/select1/range.

Comment 67

[alexander :surkov (:asurkov)]

Attachment: patch9 [for checkin]

Comment 68

[aaronr]

checked into trunk for surkov.  Not marking xf-to-branch (xforms keywords) since I assume these changes won't make ff 2.0 or 1.5.x.