Testcase?

(In reply to comment #1) > Testcase? > Sorry, probably you was confused by bug summary. Bug is to implement accessible objects for xforms select/select1 of full appearance that are represented by radiogroup and group of checkboxes.

Comment on attachment 247659 [details] [diff] [review] patch >Index: accessible/src/xforms/nsXFormsAccessible.cpp >=================================================================== >RCS file: /cvsroot/mozilla/accessible/src/xforms/nsXFormsAccessible.cpp,v >retrieving revision 1.3 >diff -u -8 -p -r1.3 nsXFormsAccessible.cpp >--- accessible/src/xforms/nsXFormsAccessible.cpp 6 Nov 2006 02:50:37 -0000 1.3 >+++ accessible/src/xforms/nsXFormsAccessible.cpp 6 Dec 2006 11:02:06 -0000 >@@ -89,16 +90,69 @@ nsXFormsAccessible::GetBoundChildElement > return sXFormsService->GetValue(node, aValue); > } > } > > aValue.Truncate(); > return NS_OK; > } > >+void >+nsXFormsAccessible::CacheSelectChildren() >+{ Can I assume that CacheSelectChildren is called at the right times so that that the cache of items will always be correct even if someone inserts more nodes into the nodeset that is used to build the items when the items live in an itemset? >+ if (!mWeakShell) { >+ // This node has been shut down >+ mAccChildCount = eChildCountUninitialized; >+ return; >+ } >+ >+ if (mAccChildCount != eChildCountUninitialized) >+ return; >+ >+ nsIAccessibilityService *accService = GetAccService(); >+ if (!accService) >+ return; >+ >+ nsCOMPtr<nsIDOMNodeList> children; >+ nsresult rv = >+ sXFormsService->GetSelectChildrenFor(mDOMNode, getter_AddRefs(children)); >+ if (!children) >+ return; >+ >+ PRUint32 length = 0; >+ children->GetLength(&length); >+ >+ nsCOMPtr<nsIAccessible> accessible; >+ nsCOMPtr<nsPIAccessible> currAccessible; >+ nsCOMPtr<nsPIAccessible> prevAccessible; >+ >+ for (PRUint32 i = 0; i < length; i++) { >+ nsCOMPtr<nsIDOMNode> child; >+ children->Item(i, getter_AddRefs(child)); >+ if (!child) >+ continue; >+ >+ accService->GetAttachedAccessibleFor(child, getter_AddRefs(accessible)); >+ currAccessible = do_QueryInterface(accessible); >+ if (!currAccessible) >+ continue; >+ >+ if (i == 0) >+ SetFirstChild(accessible); >+ >+ currAccessible->SetParent(this); >+ if (prevAccessible) { >+ prevAccessible->SetNextSibling(accessible); >+ } >+ prevAccessible = currAccessible; >+ } >+ >+ mAccChildCount = length; >+} >+ There are a couple of places above where you could 'continue' without adding an item into the linked list. If that happens, you still want mAccChildCount to be equal to length, or do you just want it equal to the number of items in the list? >+NS_IMETHODIMP >+nsXFormsSelectableAccessible::GetSelectedChildren(nsIArray **aAccessibles) >+{ >+ NS_ENSURE_ARG_POINTER(aAccessibles); >+ >+ *aAccessibles = nsnull; >+ >+ nsCOMPtr<nsIMutableArray> accessibles = >+ do_CreateInstance(NS_ARRAY_CONTRACTID); >+ NS_ENSURE_TRUE(accessibles, NS_ERROR_FAILURE); >+ >+ nsIAccessibilityService* accService = GetAccService(); >+ NS_ENSURE_TRUE(accService, NS_ERROR_FAILURE); >+ >+ nsresult rv; >+ nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode)); >+ >+ if (content->NodeInfo()->Equals(nsAccessibilityAtoms::select1)) { >+ nsCOMPtr<nsIDOMNode> item; nit: I notice this pattern a lot in this code...getting the content and then figuring out if it is a select or a select1 each time you need to. Can't you just do this once in the constructor to set a member data flag and then just check the flag here to see if it is a select or a select1? >+ rv = sXFormsService->GetSelect1SelectedItem(mDOMNode, getter_AddRefs(item)); >+ NS_ENSURE_SUCCESS(rv, rv); Ugh, don't like the function name 'GetSelect1SelectedItem' much. How about GetSelectedItemForSelect1? >+ >+ if (!item) >+ return NS_OK; >+ >+ nsCOMPtr<nsIAccessible> accessible; >+ accService->GetAccessibleFor(item, getter_AddRefs(accessible)); >+ NS_ENSURE_TRUE(accessible, NS_ERROR_FAILURE); >+ >+ accessibles->AppendElement(accessible, PR_FALSE); >+ NS_ADDREF(*aAccessibles = accessibles); >+ return NS_OK; >+ } >+ >+ nsCOMPtr<nsIDOMNodeList> items; >+ rv = sXFormsService->GetSelectSelectedItems(mDOMNode, getter_AddRefs(items)); >+ NS_ENSURE_SUCCESS(rv, rv); nit: another naming thing. How about GetSelectedItemsForSelect? Or maybe just easier to just make one function in the service (GetSelectedItems) and then always return an array with a length for both select and select1? Not all that much more overhead and you'll get rid of a lot of if(select1) tests here and below. >+NS_IMETHODIMP >+nsXFormsSelectableAccessible::RefSelection(PRInt32 aIndex, >+ nsIAccessible **aAccessible) >+{ >+ NS_ENSURE_ARG_POINTER(aAccessible); >+ *aAccessible = nsnull; >+ >+ nsIAccessibilityService* accService = GetAccService(); >+ NS_ENSURE_TRUE(accService, NS_ERROR_FAILURE); >+ >+ nsresult rv; >+ nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode)); >+ >+ if (content->NodeInfo()->Equals(nsAccessibilityAtoms::select1)) { >+ if (aIndex != 0) >+ return NS_OK; >+ >+ nsCOMPtr<nsIDOMNode> item; >+ rv = sXFormsService->GetSelect1SelectedItem(mDOMNode, getter_AddRefs(item)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (item) >+ return accService->GetAccessibleFor(item, aAccessible); >+ return NS_OK; >+ } >+ >+ nsCOMPtr<nsIDOMNodeList> items; >+ rv = sXFormsService->GetSelectSelectedItems(mDOMNode, getter_AddRefs(items)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (!items) >+ return NS_OK; >+ >+ PRUint32 length = 0; >+ items->GetLength(&length); >+ if (aIndex < 0 || PRUint32(aIndex) <= length) >+ return NS_OK; >+ shouldn't that test be PRUint32(aIndex) >= length? >+ nsCOMPtr<nsIDOMNode> item; >+ items->Item(aIndex, getter_AddRefs(item)); >+ >+ nsCOMPtr<nsIAccessible> accessible; >+ return accService->GetAccessibleFor(item, getter_AddRefs(accessible)); >+} >+ >+NS_IMETHODIMP >+nsXFormsSelectableAccessible::IsChildSelected(PRInt32 aIndex, PRBool *aIsSelected) >+{ >+ NS_ENSURE_ARG_POINTER(aIsSelected); >+ *aIsSelected = PR_FALSE; >+ >+ nsCOMPtr<nsIDOMNode> item = GetItemByIndex(aIndex); >+ if (!item) >+ return NS_OK; >+ >+ nsresult rv; >+ nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode)); >+ >+ if (content->NodeInfo()->Equals(nsAccessibilityAtoms::select1)) { >+ nsCOMPtr<nsIDOMNode> selitem; >+ rv = sXFormsService->GetSelect1SelectedItem(mDOMNode, >+ getter_AddRefs(selitem)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (selitem == item) >+ *aIsSelected = PR_TRUE; >+ return NS_OK; >+ } >+ >+ return sXFormsService->IsSelectItemSelected(mDOMNode, item, aIsSelected); >+} >+ another place where you could just use one service call for both select and select1 instead of treating them differently -> IsItemSelected() >+NS_IMETHODIMP >+nsXFormsSelectableAccessible::ClearSelection() >+{ >+ nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode)); >+ >+ if (content->NodeInfo()->Equals(nsAccessibilityAtoms::select1)) >+ return sXFormsService->SetSelect1SelectedItem(mDOMNode, nsnull); >+ >+ return sXFormsService->ClearSelectSelection(mDOMNode); >+} another place where one service call is good enough for both -> ClearSelection() >+ >+NS_IMETHODIMP >+nsXFormsSelectableAccessible::SelectAllSelection(PRBool *aMultipleSelection) >+{ >+ NS_ENSURE_ARG_POINTER(aMultipleSelection); >+ >+ nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode)); >+ >+ if (content->NodeInfo()->Equals(nsAccessibilityAtoms::select1)) { >+ *aMultipleSelection = PR_FALSE; >+ return NS_OK; >+ } >+ >+ *aMultipleSelection = PR_TRUE; >+ return sXFormsService->SelectAllSelectItems(mDOMNode); >+} >+ one service call for both could be SelectAllItems(mDOMNode, &successful) and just have a select1 return PR_FALSE >+already_AddRefed<nsIDOMNode> >+nsXFormsSelectableAccessible::GetItemByIndex(PRInt32 aIndex) >+{ >+ nsCOMPtr<nsIDOMElement> element(do_QueryInterface(mDOMNode)); >+ if(!element) >+ return nsnull; >+ >+ nsresult rv; >+ nsCOMPtr<nsIDOMNodeList> nodes; >+ rv = element->GetElementsByTagNameNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS), >+ NS_LITERAL_STRING("item"), >+ getter_AddRefs(nodes)); >+ >+ PRUint32 length = 0; >+ nodes->GetLength(&length); >+ >+ if (aIndex < 0 || PRUint32(aIndex) >= length) >+ return nsnull; >+ >+ nsIDOMNode *node; >+ nodes->Item(aIndex, &node); >+ return node; >+} This approach isn't going to work, is it? What if the select/select1 element contains an itemset? Sure, it will contain xforms:items in the anonymous content, but GetElementsByTagNameNS won't find them, right? It'll only search the DOM. >+ >+ >+// nsXFormsSelectableItemAccessible >+ >+nsXFormsSelectableItemAccessible:: >+ nsXFormsSelectableItemAccessible(nsIDOMNode* aNode, nsIWeakReference* aShell) : >+ nsXFormsAccessible(aNode, aShell) >+{ >+} >+ >+NS_IMETHODIMP >+nsXFormsSelectableItemAccessible::GetValue(nsAString& aValue) >+{ >+ return sXFormsService->GetItemValue(mDOMNode, aValue); >+} >+ We already have nsXFormsUtilityService->GetValue(). Why do we need GetItemValue(), too? >+PRBool >+nsXFormsSelectableItemAccessible::IsItemSelected() >+{ >+ nsresult rv; >+ >+ nsCOMPtr<nsINode> parent = do_QueryInterface(mDOMNode); >+ while (parent = parent->GetNodeParent()) { >+ nsCOMPtr<nsIContent> content(do_QueryInterface(parent)); >+ if (!content) >+ return PR_FALSE; >+ >+ nsCOMPtr<nsINodeInfo> nodeinfo = content->NodeInfo(); >+ if (!nodeinfo->NamespaceEquals(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS))) >+ continue; >+ >+ nsCOMPtr<nsIDOMNode> select(do_QueryInterface(parent)); >+ if (!select) >+ continue; >+ >+ if (nodeinfo->Equals(nsAccessibilityAtoms::select)) { >+ PRBool isSelected = PR_FALSE; >+ rv = sXFormsService->IsSelectItemSelected(select, mDOMNode, &isSelected); >+ return NS_SUCCEEDED(rv) && isSelected; >+ } >+ >+ if (nodeinfo->Equals(nsAccessibilityAtoms::select1)) { >+ nsCOMPtr<nsIDOMNode> selitem; >+ rv = sXFormsService->GetSelect1SelectedItem(select, >+ getter_AddRefs(selitem)); >+ return NS_SUCCEEDED(rv) && selitem == mDOMNode; >+ } >+ } >+ >+ return PR_FALSE; >+} >+ finding the parent select...Isn't this something that we could do once in the constructor and store it off? And, of course, create a destructor to null it out again. Or do it here once (so that we don't do it until we know we need it) and then store it off in some member variable? >Index: accessible/src/xforms/nsXFormsAccessible.h >=================================================================== >RCS file: /cvsroot/mozilla/accessible/src/xforms/nsXFormsAccessible.h,v >retrieving revision 1.5 >diff -u -8 -p -r1.5 nsXFormsAccessible.h >--- accessible/src/xforms/nsXFormsAccessible.h 6 Nov 2006 02:50:37 -0000 1.5 >+++ accessible/src/xforms/nsXFormsAccessible.h 6 Dec 2006 11:02:06 -0000 >@@ -77,16 +77,21 @@ protected: > > // Returns value of first child xforms element by tagname that is bound to > // instance node. > nsresult GetBoundChildElementValue(const nsAString& aTagName, > nsAString& aValue); > > // Service allows to get some xforms functionality. > static nsIXFormsUtilityService *sXFormsService; >+ >+ // Cache select accessible children. The method is supposed to be used by >+ // accessible objects for select/select1 elements that are represented by >+ // group of checkboxes and radiogroup and for choices element. >+ void CacheSelectChildren(); Aren't we going to use this for all selects? Why just for checkboxes and radiogroups? It is just a dom node list, afterall. Or does the html:select magically do all of this for us? If so, might want to mention that here. > }; > > > /** > * This class is accessible object for XForms elements that provide accessible > * object for itself as well for anonymous content. You should use this class > * if accessible XForms element is complex, i.e. it is composed from elements > * that should be accessible too. Especially for elements that have multiple >@@ -129,10 +134,42 @@ protected: > virtual void SetEditor(nsIEditor *aEditor); > virtual already_AddRefed<nsIEditor> GetEditor(); > > private: > nsCOMPtr<nsIEditor> mEditor; > }; > > >+/** >+ * The class is base for accessible objects for XForms select and XForms >+ * select1 elements. >+ */ >+class nsXFormsSelectableAccessible : public nsXFormsEditableAccessible >+{ >+public: >+ nsXFormsSelectableAccessible(nsIDOMNode* aNode, nsIWeakReference* aShell); >+ >+ NS_DECL_NSIACCESSIBLESELECTABLE >+ >+protected: >+ already_AddRefed<nsIDOMNode> GetItemByIndex(PRInt32 aIndex); >+}; >+ >+ >+/** >+ * The class is base for accessible objects for XForms item elements. >+ */ >+class nsXFormsSelectableItemAccessible : public nsXFormsAccessible >+{ >+public: >+ nsXFormsSelectableItemAccessible(nsIDOMNode* aNode, nsIWeakReference* aShell); >+ >+ NS_IMETHOD GetValue(nsAString& aValue); >+ NS_IMETHOD GetNumActions(PRUint8 *aCount); >+ NS_IMETHOD DoAction(PRUint8 aIndex); >+ >+protected: >+ PRBool IsItemSelected(); >+}; >+ > #endif > >Index: accessible/src/xforms/nsXFormsFormControlsAccessible.cpp >=================================================================== >RCS file: /cvsroot/mozilla/accessible/src/xforms/nsXFormsFormControlsAccessible.cpp,v >retrieving revision 1.4 >diff -u -8 -p -r1.4 nsXFormsFormControlsAccessible.cpp >--- accessible/src/xforms/nsXFormsFormControlsAccessible.cpp 6 Nov 2006 02:50:37 -0000 1.4 >+++ accessible/src/xforms/nsXFormsFormControlsAccessible.cpp 6 Dec 2006 11:02:06 -0000 >@@ -375,8 +375,146 @@ nsXFormsRangeAccessible::GetCurrentValue > nsresult rv = sXFormsService->GetValue(mDOMNode, value); > NS_ENSURE_SUCCESS(rv, rv); > > PRInt32 error = NS_OK; > *aCurrentValue = value.ToFloat(&error); > return error; > } > >+ >+// nsXFormsChoicesAccessible >+ >+nsXFormsChoicesAccessible:: >+ nsXFormsChoicesAccessible(nsIDOMNode *aNode, nsIWeakReference *aShell): >+ nsXFormsAccessible(aNode, aShell) >+{ >+} >+ >+NS_IMETHODIMP >+nsXFormsChoicesAccessible::GetRole(PRUint32 *aRole) >+{ >+ NS_ENSURE_ARG_POINTER(aRole); >+ >+ *aRole = ROLE_GROUPING; >+ return NS_OK; >+} >+ >+NS_IMETHODIMP >+nsXFormsChoicesAccessible::GetValue(nsAString& aValue) >+{ >+ aValue.Truncate(); >+ return NS_OK; >+} >+ >+void >+nsXFormsChoicesAccessible::CacheChildren() >+{ >+ CacheSelectChildren(); >+} >+ >+ >+// nsXFormsSelectFullAccessible >+ >+nsXFormsSelectFullAccessible:: >+ nsXFormsSelectFullAccessible(nsIDOMNode *aNode, nsIWeakReference *aShell): >+ nsXFormsSelectableAccessible(aNode, aShell) >+{ >+} >+ >+NS_IMETHODIMP >+nsXFormsSelectFullAccessible::GetRole(PRUint32 *aRole) >+{ >+ NS_ENSURE_ARG_POINTER(aRole); >+ >+ *aRole = ROLE_GROUPING; >+ return NS_OK; >+} >+ >+void >+nsXFormsSelectFullAccessible::CacheChildren() >+{ >+ CacheSelectChildren(); >+} >+ >+ >+// nsXFormsItemCheckgroupAccessible >+ >+nsXFormsItemCheckgroupAccessible:: >+ nsXFormsItemCheckgroupAccessible(nsIDOMNode *aNode, nsIWeakReference *aShell): >+ nsXFormsSelectableItemAccessible(aNode, aShell) >+{ >+} >+ >+NS_IMETHODIMP >+nsXFormsItemCheckgroupAccessible::GetRole(PRUint32 *aRole) >+{ >+ NS_ENSURE_ARG_POINTER(aRole); >+ >+ *aRole = ROLE_CHECKBUTTON; >+ return NS_OK; >+} >+ >+NS_IMETHODIMP >+nsXFormsItemCheckgroupAccessible::GetState(PRUint32 *aState) >+{ >+ nsresult rv = nsXFormsSelectableItemAccessible::GetState(aState); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (IsItemSelected()) >+ *aState |= STATE_CHECKED; >+ >+ return NS_OK; >+} >+ >+NS_IMETHODIMP >+nsXFormsItemCheckgroupAccessible::GetActionName(PRUint8 aIndex, nsAString& aName) >+{ >+ if (aIndex != eAction_Click) >+ return NS_ERROR_INVALID_ARG; >+ >+ if (IsItemSelected()) >+ nsAccessible::GetTranslatedString(NS_LITERAL_STRING("uncheck"), aName); >+ else >+ nsAccessible::GetTranslatedString(NS_LITERAL_STRING("check"), aName); >+ >+ return NS_OK; >+} >+ >+// nsXFormsItemRadiogroupAccessible >+ >+nsXFormsItemRadiogroupAccessible:: >+ nsXFormsItemRadiogroupAccessible(nsIDOMNode *aNode, nsIWeakReference *aShell): >+ nsXFormsSelectableItemAccessible(aNode, aShell) >+{ >+} >+ >+NS_IMETHODIMP >+nsXFormsItemRadiogroupAccessible::GetRole(PRUint32 *aRole) >+{ >+ NS_ENSURE_ARG_POINTER(aRole); >+ >+ *aRole = ROLE_RADIOBUTTON; >+ return NS_OK; >+} >+ >+NS_IMETHODIMP >+nsXFormsItemRadiogroupAccessible::GetState(PRUint32 *aState) >+{ >+ nsresult rv = nsXFormsSelectableItemAccessible::GetState(aState); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (IsItemSelected()) >+ *aState |= STATE_CHECKED; >+ >+ return NS_OK; >+} >+ >+NS_IMETHODIMP >+nsXFormsItemRadiogroupAccessible::GetActionName(PRUint8 aIndex, nsAString& aName) >+{ >+ if (aIndex != eAction_Click) >+ return NS_ERROR_INVALID_ARG; >+ >+ nsAccessible::GetTranslatedString(NS_LITERAL_STRING("select"), aName); >+ return NS_OK; >+} >+ >Index: accessible/src/xforms/nsXFormsFormControlsAccessible.h >=================================================================== >RCS file: /cvsroot/mozilla/accessible/src/xforms/nsXFormsFormControlsAccessible.h,v >retrieving revision 1.4 >diff -u -8 -p -r1.4 nsXFormsFormControlsAccessible.h >--- accessible/src/xforms/nsXFormsFormControlsAccessible.h 6 Nov 2006 02:50:37 -0000 1.4 >+++ accessible/src/xforms/nsXFormsFormControlsAccessible.h 6 Dec 2006 11:02:06 -0000 >@@ -144,10 +144,74 @@ public: > > // nsIAccessibleValue > NS_IMETHOD GetMaximumValue(double *aMaximumValue); > NS_IMETHOD GetMinimumValue(double *aMinimumValue); > NS_IMETHOD GetMinimumIncrement(double *aMinimumIncrement); > NS_IMETHOD GetCurrentValue(double *aCurrentValue); > }; > >+ >+/** >+ * Accessible object for xforms:choices. >+ */ >+ >+class nsXFormsChoicesAccessible : public nsXFormsAccessible >+{ >+public: >+ nsXFormsChoicesAccessible(nsIDOMNode* aNode, nsIWeakReference* aShell); >+ >+ NS_IMETHOD GetRole(PRUint32 *aRole); >+ NS_IMETHOD GetValue(nsAString& aValue); >+ >+ void CacheChildren(); >+}; >+ >+ >+/** >+ * Accessible object for xforms:select/xforms:select1 of full appearance that >+ * may be represented by group of checkboxes or radiogroup. >+ */ >+ >+class nsXFormsSelectFullAccessible : public nsXFormsSelectableAccessible >+{ >+public: >+ nsXFormsSelectFullAccessible(nsIDOMNode* aNode, nsIWeakReference* aShell); >+ >+ NS_IMETHOD GetRole(PRUint32 *aRole); >+ >+ void CacheChildren(); >+}; >+ >+ >+/** >+ * Accessible object for xforms:item element of full appearance xforms:select >+ * that is represented by group of checkboxes. >+ */ >+ nit: wording. Maybe try, "Accessible object for a xforms:item when it is represented by a checkbox. This occurs when the item is contained in a xforms:select with full appearance. Such a xforms:select is represented by a checkgroup." >+class nsXFormsItemCheckgroupAccessible : public nsXFormsSelectableItemAccessible >+{ >+public: >+ nsXFormsItemCheckgroupAccessible(nsIDOMNode* aNode, nsIWeakReference* aShell); >+ >+ NS_IMETHOD GetRole(PRUint32 *aRole); >+ NS_IMETHOD GetState(PRUint32 *aState); >+ NS_IMETHOD GetActionName(PRUint8 aIndex, nsAString& aName); >+}; >+ >+ >+/** >+ * Accessible object for xforms:item element of full appearance xforms:select1 >+ * that is represented by radiogroup. >+ */ >+ nit: wording. Maybe try, "Accessible object for a xforms:item when it is represented by a radiobutton. This occurs when the item is contained in a xforms:select1 with full appearance. Such a xforms:select is represented as a radiogroup" >+class nsXFormsItemRadiogroupAccessible : public nsXFormsSelectableItemAccessible >+{ >+public: >+ nsXFormsItemRadiogroupAccessible(nsIDOMNode* aNode, nsIWeakReference* aShell); >+ >+ NS_IMETHOD GetRole(PRUint32 *aRole); >+ NS_IMETHOD GetState(PRUint32 *aState); >+ NS_IMETHOD GetActionName(PRUint8 aIndex, nsAString& aName); >+}; >+ > #endif > >Index: content/base/public/nsIXFormsUtilityService.h this is as far as I've gotten. More to come.

(In reply to comment #4) > >+void > >+nsXFormsAccessible::CacheSelectChildren() > >+{ > > Can I assume that CacheSelectChildren is called at the right times so that that > the cache of items will always be correct even if someone inserts more nodes > into the nodeset that is used to build the items when the items live in an > itemset? CacheSelectChildren is called inside CacheChildren. Accessibility code should guarantee the call of CacheChildren when underlying tree is modifyied I guess. > There are a couple of places above where you could 'continue' without adding an > item into the linked list. If that happens, you still want mAccChildCount to > be equal to length, or do you just want it equal to the number of items in the > list? Fixed. > nit: I notice this pattern a lot in this code...getting the content and then > figuring out if it is a select or a select1 each time you need to. Can't you > just do this once in the constructor to set a member data flag and then just > check the flag here to see if it is a select or a select1? Fixed > nit: another naming thing. How about GetSelectedItemsForSelect? Fixed for most methods. > > Or maybe just easier to just make one function in the service > (GetSelectedItems) and then always return an array with a length for both > select and select1? Not all that much more overhead and you'll get rid of a > lot of if(select1) tests here and below. > another place where you could just use one service call for both select and > select1 instead of treating them differently -> IsItemSelected() > another place where one service call is good enough for both -> > ClearSelection() > one service call for both could be SelectAllItems(mDOMNode, &successful) and > just have a select1 return PR_FALSE Not sure. Currenly XFormsService contains methods that corresponds to methods of some xforms interface. I guess this doesn't make difference where functionality is defined (in xforms or in accessible module). > >+ PRUint32 length = 0; > >+ items->GetLength(&length); > >+ if (aIndex < 0 || PRUint32(aIndex) <= length) > >+ return NS_OK; > >+ > > shouldn't that test be PRUint32(aIndex) >= length? Fixed. > >+already_AddRefed<nsIDOMNode> > >+nsXFormsSelectableAccessible::GetItemByIndex(PRInt32 aIndex) > > This approach isn't going to work, is it? What if the select/select1 element > contains an itemset? Sure, it will contain xforms:items in the anonymous > content, but GetElementsByTagNameNS won't find them, right? It'll only search > the DOM. Looks right. Changed. > We already have nsXFormsUtilityService->GetValue(). Why do we need > GetItemValue(), too? Because getValue() is different a bit. GetValue returns value of current value of xforms element if it is bound to instance node. Value of xforms item is formed by other rules. But if you like I'll include code of GetItemValue into GetValue. > >+PRBool > >+nsXFormsSelectableItemAccessible::IsItemSelected() > >+{ > >+ nsresult rv; > >+ > >+ nsCOMPtr<nsINode> parent = do_QueryInterface(mDOMNode); > >+ while (parent = parent->GetNodeParent()) { > >+ nsCOMPtr<nsIContent> content(do_QueryInterface(parent)); > >+ if (!content) > >+ return PR_FALSE; > finding the parent select...Isn't this something that we could do once in the > constructor and store it off? And, of course, create a destructor to null it > out again. Or do it here once (so that we don't do it until we know we need > it) and then store it off in some member variable? Good point. Though I don't know where to handle the situation when item was moved in tree and should I? AaronLev? > >+ // Cache select accessible children. The method is supposed to be used by > >+ // accessible objects for select/select1 elements that are represented by > >+ // group of checkboxes and radiogroup and for choices element. > >+ void CacheSelectChildren(); > > Aren't we going to use this for all selects? Why just for checkboxes and > radiogroups? It is just a dom node list, afterall. Or does the html:select > magically do all of this for us? If so, might want to mention that here. This method should be used by minimal select1 for xhtml too. Select controls that are implemented using native widget doesn't do this since their items/choices elements are always hidden and therefore they haven't accessible objects.

(In reply to comment #5) > Created an attachment (id=247812) [edit] > patch2 > > (In reply to comment #4) > > > > > > Or maybe just easier to just make one function in the service > > (GetSelectedItems) and then always return an array with a length for both > > select and select1? Not all that much more overhead and you'll get rid of a > > lot of if(select1) tests here and below. > > > another place where you could just use one service call for both select and > > select1 instead of treating them differently -> IsItemSelected() > > > another place where one service call is good enough for both -> > > ClearSelection() > > > one service call for both could be SelectAllItems(mDOMNode, &successful) and > > just have a select1 return PR_FALSE > > Not sure. Currenly XFormsService contains methods that corresponds to methods > of some xforms interface. I guess this doesn't make difference where > functionality > is defined (in xforms or in accessible module). I'm just trying to minimize clutter in the xforms service. Since most of these new functions for select have a corresponding function for select1 that do almost exactly the same thing, I'd like to see them combined if possible. > > > We already have nsXFormsUtilityService->GetValue(). Why do we need > > GetItemValue(), too? > > Because getValue() is different a bit. GetValue returns value of current value > of > xforms element if it is bound to instance node. Value of xforms item is formed > by > other rules. But if you like I'll include code of GetItemValue into GetValue. sXFormsService->GetValue() ends up calling uiWidget->GetCurrentValue(). So can't you just create a method on the xforms side called GetCurrentValue() for item and have it return what you want for accessibility? And then on the accessibility side call sXFormsSevice->GetValue(item). > > > >+PRBool > > >+nsXFormsSelectableItemAccessible::IsItemSelected() > > >+{ > > >+ nsresult rv; > > >+ > > >+ nsCOMPtr<nsINode> parent = do_QueryInterface(mDOMNode); > > >+ while (parent = parent->GetNodeParent()) { > > >+ nsCOMPtr<nsIContent> content(do_QueryInterface(parent)); > > >+ if (!content) > > >+ return PR_FALSE; > > > finding the parent select...Isn't this something that we could do once in the > > constructor and store it off? And, of course, create a destructor to null it > > out again. Or do it here once (so that we don't do it until we know we need > > it) and then store it off in some member variable? > > Good point. Though I don't know where to handle the situation when item was > moved in > tree and should I? AaronLev? XForms will know when the item's parent changes. Can we call a function on the item's accessibility object to tell it to update its stored select control? > > > >+ // Cache select accessible children. The method is supposed to be used by > > >+ // accessible objects for select/select1 elements that are represented by > > >+ // group of checkboxes and radiogroup and for choices element. > > >+ void CacheSelectChildren(); > > > > Aren't we going to use this for all selects? Why just for checkboxes and > > radiogroups? It is just a dom node list, afterall. Or does the html:select > > magically do all of this for us? If so, might want to mention that here. > > This method should be used by minimal select1 for xhtml too. Select controls > that are implemented using native widget doesn't do this since > their items/choices elements are always hidden and therefore they haven't > accessible objects. > Good information. Should be in the comment somewhere.

Comment on attachment 247812 [details] [diff] [review] patch2 >Index: accessible/src/xforms/nsXFormsAccessible.cpp >=================================================================== >RCS file: /cvsroot/mozilla/accessible/src/xforms/nsXFormsAccessible.cpp,v >retrieving revision 1.3 >diff -u -8 -p -r1.3 nsXFormsAccessible.cpp >--- accessible/src/xforms/nsXFormsAccessible.cpp 6 Nov 2006 02:50:37 -0000 1.3 >+++ accessible/src/xforms/nsXFormsAccessible.cpp 7 Dec 2006 11:56:56 -0000 >+already_AddRefed<nsIDOMNode> >+nsXFormsSelectableAccessible::GetItemByIndex(PRInt32 aIndex, >+ nsIAccessible *aAccessible) >+{ >+ if (!aAccessible) >+ aAccessible = this; please use a local variable here, rather than assigning into the parameter. If for no other reason, makes it easier to debug. So that 15 lines from here I can easily look and see that aAccessible was nsnull coming in. Also please use a local variable counter and increment it and compare its value against aIndex to see if we've reached the item we are looking for. Again, it is nice to always see the original value that was passed in. The only time you should modify a parameter is if it is a return buffer. Gotta leave for a couple of hours. More comments to come after I get back.

(In reply to comment #6) > I'm just trying to minimize clutter in the xforms service. Since most of these > new functions for select have a corresponding function for select1 that do > almost exactly the same thing, I'd like to see them combined if possible. XForms select controls has difference from XHTML and XUL select controls. The difference is XForms has one element for single selection and one element for multiselection. If I'll try to combine methods to operate with xforms select/select by common way then I'll get interface for xforms select control (because it is wider). Then I will have for example: selectAllItems() and addItemToSelection() - methods don't make sense for select1 getSelectedItems() - return array containing one item for select1, and it looks redudrant That's a reason why I like to expose methods in XForms service both for select and select1. > > > We already have nsXFormsUtilityService->GetValue(). Why do we need > > > GetItemValue(), too? > > > > Because getValue() is different a bit. GetValue returns value of current value > > of > > xforms element if it is bound to instance node. Value of xforms item is formed > > by > > other rules. But if you like I'll include code of GetItemValue into GetValue. > > sXFormsService->GetValue() ends up calling uiWidget->GetCurrentValue(). So > can't you just create a method on the xforms side called GetCurrentValue() for > item and have it return what you want for accessibility? And then on the > accessibility side call sXFormsSevice->GetValue(item). Looks good if xforms item will implement nsIXFormsUIWidget interface. I'll fix it. > > > >+PRBool > > > >+nsXFormsSelectableItemAccessible::IsItemSelected() > > > >+{ > > > >+ nsresult rv; > > > >+ > > > >+ nsCOMPtr<nsINode> parent = do_QueryInterface(mDOMNode); > > > >+ while (parent = parent->GetNodeParent()) { > > > >+ nsCOMPtr<nsIContent> content(do_QueryInterface(parent)); > > > >+ if (!content) > > > >+ return PR_FALSE; > > > > > finding the parent select...Isn't this something that we could do once in the > > > constructor and store it off? And, of course, create a destructor to null it > > > out again. Or do it here once (so that we don't do it until we know we need > > > it) and then store it off in some member variable? > > > > Good point. Though I don't know where to handle the situation when item was > > moved in > > tree and should I? AaronLev? > > XForms will know when the item's parent changes. Can we call a function on the > item's accessibility object to tell it to update its stored select control? I believe xforms should say nothing accessibility. > > > >+ // group of checkboxes and radiogroup and for choices element. > > > >+ void CacheSelectChildren(); > > > > > > Aren't we going to use this for all selects? Why just for checkboxes and > > > radiogroups? It is just a dom node list, afterall. Or does the html:select > > > magically do all of this for us? If so, might want to mention that here. > > > > This method should be used by minimal select1 for xhtml too. Select controls > > that are implemented using native widget doesn't do this since > > their items/choices elements are always hidden and therefore they haven't > > accessible objects. > > > > Good information. Should be in the comment somewhere. > Ok, I'll do.

Comment on attachment 247812 [details] [diff] [review] patch2 >Index: accessible/src/xforms/nsXFormsFormControlsAccessible.h >=================================================================== >RCS file: /cvsroot/mozilla/accessible/src/xforms/nsXFormsFormControlsAccessible.h,v >retrieving revision 1.5 >diff -u -8 -p -r1.5 nsXFormsFormControlsAccessible.h >--- accessible/src/xforms/nsXFormsFormControlsAccessible.h 6 Dec 2006 13:44:19 -0000 1.5 >+++ accessible/src/xforms/nsXFormsFormControlsAccessible.h 7 Dec 2006 11:56:56 -0000 >+ >+ >+/** >+ * Accessible object for a xforms:item when it is represented by a radiobutton. >+ * This occurs when the item is contained in a xforms:select1 with full >+ * appearance. Such a xforms:select is represented as a radiogroup. >+ */ >+ >+class nsXFormsItemRadiogroupAccessible : public nsXFormsSelectableItemAccessible >+{ nit: should be "Such a xforms:select1 is...." since radiogroups are for select1. >Index: content/base/public/nsIXFormsUtilityService.h >=================================================================== >RCS file: /cvsroot/mozilla/content/base/public/nsIXFormsUtilityService.h,v >retrieving revision 1.6 >diff -u -8 -p -r1.6 nsIXFormsUtilityService.h >--- content/base/public/nsIXFormsUtilityService.h 6 Nov 2006 02:50:37 -0000 1.6 >+++ content/base/public/nsIXFormsUtilityService.h 7 Dec 2006 11:56:56 -0000 >@@ -16,16 +16,17 @@ > * > * The Initial Developer of the Original Code is > * IBM Corporation. > * Portions created by the Initial Developer are Copyright (C) 2006 > * the Initial Developer. All Rights Reserved. > * > * Contributor(s): > * Aaron Reed <aaronr@us.ibm.com> >+ * Alexander Surkov <surkov.alexander@gmail.com> > * > * 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 >@@ -37,28 +38,29 @@ > * ***** END LICENSE BLOCK ***** */ > > #ifndef nsIXFormsUtilityService_h > #define nsIXFormsUtilityService_h > > #include "nsISupports.h" > > class nsIDOMNode; >+class nsIDOMNodeList; > class nsIEditor; > > /* 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 "43ad19a6-5639-4b05-8305-1eb729063912" >+#define NS_IXFORMSUTILITYSERVICE_IID_STR "88d9eaa9-1498-4ffb-85a6-44267595cb20" > #define NS_IXFORMSUTILITYSERVICE_IID \ >-{ 0x43ad19a6, 0x5639, 0x4b05, \ >- { 0x83, 0x5, 0x1e, 0xb7, 0x29, 0x6, 0x39, 0x12 } } >+{ 0x88d9eaa9, 0x1498, 0x4ffb, \ >+ { 0x85, 0xa6, 0x44, 0x26, 0x75, 0x95, 0xcb, 0x20 } } > > /** > * Private interface implemented by the nsXFormsUtilityService in XForms > * extension. > */ > class NS_NO_VTABLE nsIXFormsUtilityService : public nsISupports { > public: > >@@ -126,14 +128,83 @@ public: > */ > NS_IMETHOD GetRangeStep(nsIDOMNode *aElement, nsAString& aValue) = 0; > > /** > * Return nsIEditor for xforms element if element is editable, null if it is > * not editable. Failure if given element doesn't support editing. > */ > NS_IMETHOD GetEditor(nsIDOMNode *aElemenet, nsIEditor **aEditor) = 0; >+ >+ /** >+ * Get selected xforms:item element for xforms:select1. Failure if >+ * given element is not xforms:select1. >+ */ >+ NS_IMETHOD GetSelectedItemForSelect1(nsIDOMNode *aElement, >+ nsIDOMNode ** aItem) = 0; >+ >+ /** >+ * Set selected xforms:item element for xforms:select1. Failure if >+ * given element is not xforms:select1. >+ */ >+ NS_IMETHOD SetSelectedItemForSelect1(nsIDOMNode *aElement, >+ nsIDOMNode *aItem) = 0; nit: you say element in these comments, but the user is not passing in an element, they are passing in a nsIDOMNode. Shouldn't you say 'node' instead? Also, you say, "Failure if given element is not...". Maybe better to say, "Failure if aElement is not..." which spells out explicitly which parameter you are talking about. This will help to avoid confusion with the element mentioned in the first sentence of the comment. This nit applies to many of the comments here if you choose to follow my suggestion. Not a big deal if you don't, it is just a thought. >+ >+ /** >+ * Get selected xforms:item elements list for xforms:select. Failure if >+ * given element is not xforms:select. >+ */ >+ NS_IMETHOD GetSelectedItemsForSelect(nsIDOMNode *aElement, >+ nsIDOMNodeList **aItems) = 0; nit: maybe "Get the list of selected xforms:items from the xforms:select, aElement." >+ >+ /** >+ * Clear selection of xforms:select. Failure if given element is not >+ * xforms:select. >+ */ >+ NS_IMETHOD ClearSelectionForSelect(nsIDOMNode *aElement) = 0; nit: instead of "Clear selection of xforms:select", maybe "Deslect all items contained in the given xforms:select". >+ /** >+ * Return list of xforms elements that children of xforms:select, >+ * xforms:select1, xforms:choices or xforms:itemset elements. Failure if given >+ * node is not among listed above xforms elements. >+ */ >+ NS_IMETHOD GetSelectChildrenFor(nsIDOMNode *aElement, >+ nsIDOMNodeList **aNodeList) = 0; nit: maybe, "Return the list of xforms elements that are children of aElement. aElement may be a xforms:select, xforms:select1, ..." And then give an idea of what elements can be returned. It isn't just any xforms element that is a child of these controls because you check against certain types like choices, itemset, item, etc. and you don't append them to the node list if they aren't of these specific types. >Index: extensions/xforms/nsXFormsAtoms.h >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsAtoms.h,v >retrieving revision 1.22 >diff -u -8 -p -r1.22 nsXFormsAtoms.h >--- extensions/xforms/nsXFormsAtoms.h 6 Jun 2006 13:52:03 -0000 1.22 >+++ extensions/xforms/nsXFormsAtoms.h 7 Dec 2006 11:56:56 -0000 >@@ -71,15 +71,22 @@ class nsXFormsAtoms > static NS_HIDDEN_(nsIAtom *) readyForBindProperty; > static NS_HIDDEN_(nsIAtom *) fatalError; > static NS_HIDDEN_(nsIAtom *) isInstanceDocument; > static NS_HIDDEN_(nsIAtom *) instanceDocumentOwner; > static NS_HIDDEN_(nsIAtom *) externalMessagesProperty; > static NS_HIDDEN_(nsIAtom *) deferredEventListProperty; > static NS_HIDDEN_(nsIAtom *) attrBased; > >+ // xforms elements atoms >+ static NS_HIDDEN_(nsIAtom *) choices; >+ static NS_HIDDEN_(nsIAtom *) item; >+ static NS_HIDDEN_(nsIAtom *) itemset; >+ static NS_HIDDEN_(nsIAtom *) select; >+ static NS_HIDDEN_(nsIAtom *) select1; >+ nit: please add a comment here, like "required for accessibility" to give a hint that we didn't just forget to put in all the elements, but rather that we have a reason for just having atoms for this small list of elements. > #endif >Index: extensions/xforms/nsXFormsUtilityService.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtilityService.cpp,v >retrieving revision 1.14 >diff -u -8 -p -r1.14 nsXFormsUtilityService.cpp >--- extensions/xforms/nsXFormsUtilityService.cpp 6 Nov 2006 02:50:37 -0000 1.14 >+++ extensions/xforms/nsXFormsUtilityService.cpp 7 Dec 2006 11:56:56 -0000 >+NS_IMETHODIMP >+nsXFormsUtilityService::GetSelectChildrenFor(nsIDOMNode *aElement, >+ nsIDOMNodeList **aItemsList) >+{ >+ NS_ENSURE_ARG(aElement); >+ NS_ENSURE_ARG_POINTER(aItemsList); >+ >+ nsNodeList* itemsList = new nsNodeList(); >+ NS_ENSURE_TRUE(itemsList, NS_ERROR_OUT_OF_MEMORY); >+ >+ nsresult rv = GetSelectChildrenForContainer(aElement, itemsList); >+ NS_ENSURE_SUCCESS(rv, rv); >+ you'll have a memory leak here if you return the error without freeing the itemsList you just allocated. >+ NS_ADDREF(*aItemsList = itemsList); >+} >+ you need to return NS_OK here, right? Doesn't look like you are returning anything. >+nsresult >+nsXFormsUtilityService::GetSelectChildrenForNode(nsIDOMNode *aElement, >+ nsNodeList *aNodeList) >+{ >+ nsCOMPtr<nsIDOMNodeList> children; >+ aElement->GetChildNodes(getter_AddRefs(children)); >+ NS_ENSURE_TRUE(children, NS_ERROR_FAILURE); >+ >+ PRUint32 length = 0; >+ children->GetLength(&length); >+ >+ for (PRUint32 i = 0; i < length; i++) { >+ nsCOMPtr<nsIDOMNode> child; >+ children->Item(i, getter_AddRefs(child)); >+ NS_ENSURE_TRUE(child, NS_ERROR_FAILURE); >+ >+ nsCOMPtr<nsIContent> content = do_QueryInterface(child); nit: you should have nsCOMPtr<nsIContent> content(do_QueryInterface(child)); >+ NS_ENSURE_TRUE(content, NS_ERROR_FAILURE); >+ >+ if (content->NodeInfo()->NamespaceEquals(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS))) { >+ if (content->NodeInfo()->Equals(nsXFormsAtoms::item) || >+ content->NodeInfo()->Equals(nsXFormsAtoms::choices)) { >+ aNodeList->AppendElement(content); >+ } else if (content->NodeInfo()->Equals(nsXFormsAtoms::itemset)) { >+ nsresult rv = GetSelectChildrenForContainer(child, aNodeList); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+ } >+ } >+} >+ another place where you don't return anything. Should return NS_OK, if nothing else. >+// nsNodeList >+ >+NS_IMPL_ISUPPORTS1(nsNodeList, nsIDOMNodeList) >+ >+nsNodeList::nsNodeList() >+{ >+} >+ >+nsNodeList::~nsNodeList() >+{ >+} >+ >+NS_IMETHODIMP >+nsNodeList::GetLength(PRUint32* aLength) >+{ >+ *aLength = mElements.Count(); >+ >+ return NS_OK; >+} should test aLength first before assigning into it. >+ >+NS_IMETHODIMP >+nsNodeList::Item(PRUint32 aIndex, nsIDOMNode** aReturn) >+{ >+ nsISupports *tmp = mElements.SafeObjectAt(aIndex); >+ >+ if (!tmp) { >+ *aReturn = nsnull; should test aReturn before this >Index: extensions/xforms/nsXFormsUtilityService.h >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtilityService.h,v >retrieving revision 1.8 >diff -u -8 -p -r1.8 nsXFormsUtilityService.h >--- extensions/xforms/nsXFormsUtilityService.h 6 Nov 2006 02:50:37 -0000 1.8 >+++ extensions/xforms/nsXFormsUtilityService.h 7 Dec 2006 11:56:56 -0000 >+protected: >+ // Append select child elements to given node list that are hosted inside >+ // xforms:select, xforms:select1, xforms:choices or xforms:itemset >+ nsresult GetSelectChildrenForContainer(nsIDOMNode *aElement, >+ nsNodeList *aNodeList); >+ >+ // Append select child elements to given node list that are hosted inside >+ // xforms:select, xforms:select1, xforms:choices. The method is used by >+ // 'GetSelectChildrenForContainer' method. >+ nsresult GetSelectChildrenForNode(nsIDOMNode *aElement, >+ nsNodeList *aNodeList); nit: maybe call this GetSelectChildrenForContainerInternal to emphasize that this function isn't meant to be called from outside GetSelectChildrenForContainer. nit: another good place to put a comment as to what kinds of child elements will be returned in the node list. For example, xforms:label is a child of xforms:select, but it won't be returned. >Index: extensions/xforms/resources/content/selects.xml >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/selects.xml,v >retrieving revision 1.1 >diff -u -8 -p -r1.1 selects.xml >--- extensions/xforms/resources/content/selects.xml 4 Dec 2006 15:21:14 -0000 1.1 >+++ extensions/xforms/resources/content/selects.xml 7 Dec 2006 11:56:57 -0000 >@@ -50,17 +50,28 @@ > element is visible and represented. An example is the controls for XHTML in > select-xhtml.xml. > --> > > <!-- BASE for select/select1 elements. --> > <binding id="selectcontrols-base" > extends="chrome://xforms/content/xforms.xml#xformswidget-base"> > >- <implementation implements="nsIXFormsUIWidget"> >+ <implementation implements="nsIAccessibleProvider"> >+ >+ <!-- nsIAccessibleProvider --> >+ <property name="accessibleType" readonly="true"> >+ <getter> >+ // Right now this binding is used only by full appearance >+ // select/select1 elements. Be care with this accessible type if you >+ // want to use this binding for other types of select/select1 >+ // elements. >+ return Components.interfaces.nsIAccessibleProvider.XFormsSelectFull; >+ </getter> >+ </property> > I would move this comment up to just above the binding declaration. This comment applies more to the binding than to this accessibleType. r-'ing to see the final patch with these comments fixed. Only a couple of significant things in the comments. The rest are nits.

(In reply to comment #8) > > > > > We already have nsXFormsUtilityService->GetValue(). Why do we need > > > > GetItemValue(), too? > > > > > > Because getValue() is different a bit. GetValue returns value of current value > > > of > > > xforms element if it is bound to instance node. Value of xforms item is formed > > > by > > > other rules. But if you like I'll include code of GetItemValue into GetValue. > > > > sXFormsService->GetValue() ends up calling uiWidget->GetCurrentValue(). So > > can't you just create a method on the xforms side called GetCurrentValue() for > > item and have it return what you want for accessibility? And then on the > > accessibility side call sXFormsSevice->GetValue(item). > > Looks good if xforms item will implement nsIXFormsUIWidget interface. I'll fix > it. > Though I'm not sure now that is fine idea since some of xforms:item can't implement focus() method (If I right then listbox or menulist have focus but not their items). Second reason, right now we need getCurrentValue() for accessibility only. The large work and small advantage ;). Therefore I just combined getItemValue with getValue method like you proposed initially.

Comment on attachment 248265 [details] [diff] [review] patch3 >Index: extensions/xforms/nsXFormsUtilityService.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtilityService.cpp,v >retrieving revision 1.14 >diff -u -8 -p -r1.14 nsXFormsUtilityService.cpp >--- extensions/xforms/nsXFormsUtilityService.cpp 6 Nov 2006 02:50:37 -0000 1.14 >+++ extensions/xforms/nsXFormsUtilityService.cpp 11 Dec 2006 17:00:10 -0000 > NS_IMETHODIMP > nsXFormsUtilityService::GetValue(nsIDOMNode *aElement, nsAString& aValue) > { >- GET_XFORMS_UIWIDGET >- return widget->GetCurrentValue(aValue); >+ NS_ENSURE_ARG_POINTER(aElement); nit: NS_ENSURE_ARG_POINTER is usually used for return buffers >+NS_IMETHODIMP >+nsNodeList::GetLength(PRUint32* aLength) >+{ >+ NS_ENSURE_ARG(aLength); nit: NS_ENSURE_ARG_POINTER usually used for return buffers. Great job! Thanks for your patience with my reviews. With those nits, r=me

Comment on attachment 248265 [details] [diff] [review] patch3 (In reply to comment #12) > > Great job! Thanks for your patience with my reviews. With those nits, r=me > I like to fix this nits after comments of next review.

Fixed aaronr's nits and updated to trunk

Comment on attachment 248400 [details] [diff] [review] patch4 >Index: accessible/src/base/nsAccessibilityAtomList.h >=================================================================== >RCS file: /cvsroot/mozilla/accessible/src/base/nsAccessibilityAtomList.h,v >retrieving revision 1.48 >diff -u -8 -p -r1.48 nsAccessibilityAtomList.h >--- accessible/src/base/nsAccessibilityAtomList.h 8 Dec 2006 09:57:25 -0000 1.48 >+++ accessible/src/base/nsAccessibilityAtomList.h 12 Dec 2006 17:23:18 -0000 >@@ -83,43 +83,47 @@ ACCESSIBILITY_ATOM(tableOuterFrame, "Tab > ACCESSIBILITY_ATOM(a, "a") ...... >+ACCESSIBILITY_ATOM(item, "item") // XForms >+ACCESSIBILITY_ATOM(itemset, "itemset") // XForms > ACCESSIBILITY_ATOM(img, "img") > ACCESSIBILITY_ATOM(input, "input") > ACCESSIBILITY_ATOM(label, "label") The 2 new lines should be added below "input", just above "label" >Index: accessible/src/xforms/nsXFormsAccessible.cpp >=================================================================== >RCS file: /cvsroot/mozilla/accessible/src/xforms/nsXFormsAccessible.cpp,v >retrieving revision 1.4 >diff -u -8 -p -r1.4 nsXFormsAccessible.cpp >--- accessible/src/xforms/nsXFormsAccessible.cpp 12 Dec 2006 16:19:16 -0000 1.4 >+++ accessible/src/xforms/nsXFormsAccessible.cpp 12 Dec 2006 17:23:19 -0000 >@@ -95,16 +96,70 @@ nsXFormsAccessible::GetBoundChildElement > return sXFormsService->GetValue(node, aValue); ...... >+void >+nsXFormsAccessible::CacheSelectChildren() >+{ ...... >+ nsCOMPtr<nsIDOMNodeList> children; >+ nsresult rv = >+ sXFormsService->GetSelectChildrenFor(mDOMNode, getter_AddRefs(children)); rv is unused after, should be removed. >+ if (!children) >+ return; >+ >+ PRUint32 length = 0; >+ children->GetLength(&length); >+ >+ nsCOMPtr<nsIAccessible> accessible; >+ nsCOMPtr<nsPIAccessible> currAccessible; >+ nsCOMPtr<nsPIAccessible> prevAccessible; >+ >+ for (PRUint32 i = 0, childLength = 0; i < length; i++) { childLength should be defined and assigned zero outside the "for" Aaron Leventhal suggested to use "index" instead of "i". Code will be easier to be maintained. >+ nsCOMPtr<nsIDOMNode> child; >+ children->Item(i, getter_AddRefs(child)); >+ if (!child) >+ continue; >+ >+ accService->GetAttachedAccessibleFor(child, getter_AddRefs(accessible)); >+ currAccessible = do_QueryInterface(accessible); >+ if (!currAccessible) >+ continue; >+ >+ if (i == 0) should be "if (childLength == 0)", right? >+ SetFirstChild(accessible); >+ >+ currAccessible->SetParent(this); >+ if (prevAccessible) { >+ prevAccessible->SetNextSibling(accessible); >+ } >+ prevAccessible = currAccessible; We can use currAccessible.swap(prevAccessible); >+ childLength++; >+ } >+ >+ mAccChildCount = childLength; >+} >+ >@@ -291,8 +346,360 @@ nsXFormsEditableAccessible::SetEditor(ns ... >+// nsXFormsSelectableAccessible >+ >+nsXFormsSelectableAccessible:: >+ nsXFormsSelectableAccessible(nsIDOMNode* aNode, nsIWeakReference* aShell) : >+ nsXFormsEditableAccessible(aNode, aShell) >+{ >+ nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode)); >+ mIsSelect1Element = >+ content->NodeInfo()->Equals(nsAccessibilityAtoms::select1); should add a null check here. >+} >+ >+NS_IMETHODIMP >+nsXFormsSelectableAccessible::GetSelectedChildren(nsIArray **aAccessibles) >+{ >+ NS_ENSURE_ARG_POINTER(aAccessibles); >+ >+ *aAccessibles = nsnull; >+ >+ nsCOMPtr<nsIMutableArray> accessibles = >+ do_CreateInstance(NS_ARRAY_CONTRACTID); >+ NS_ENSURE_TRUE(accessibles, NS_ERROR_FAILURE); should be NS_ERROR_OUT_OF_MEMORY ...... >+NS_IMETHODIMP >+nsXFormsSelectableAccessible::RemoveChildFromSelection(PRInt32 aIndex) >+{ >+ nsCOMPtr<nsIDOMNode> item = GetItemByIndex(aIndex); >+ if (!item) >+ return NS_OK; >+ >+ nsresult rv; >+ if (mIsSelect1Element) { >+ nsCOMPtr<nsIDOMNode> selitem; >+ rv = sXFormsService->GetSelectedItemForSelect1(mDOMNode, >+ getter_AddRefs(selitem)); >+ NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE); >+ >+ if (selitem != item) >+ return sXFormsService->SetSelectedItemForSelect1(mDOMNode, item); why? I think we should just return NS_ERROR_FAILURE here. ...... >+already_AddRefed<nsIDOMNode> >+nsXFormsSelectableAccessible::GetItemByIndex(PRInt32 aIndex, >+ nsIAccessible *aAccessible) >+{ >+ nsCOMPtr<nsIAccessible> accessible(aAccessible ? aAccessible : this); >+ PRInt32 index = aIndex; >+ >+ nsCOMPtr<nsIAccessible> curAccChild; >+ accessible->GetFirstChild(getter_AddRefs(curAccChild)); >+ >+ while (curAccChild) { >+ nsCOMPtr<nsIAccessNode> curAccNodeChild(do_QueryInterface(curAccChild)); >+ if (curAccNodeChild) { >+ nsCOMPtr<nsIDOMNode> curChildNode; >+ curAccNodeChild->GetDOMNode(getter_AddRefs(curChildNode)); >+ nsCOMPtr<nsIContent> curChildContent(do_QueryInterface(curChildNode)); >+ if (curChildContent) { >+ nsCOMPtr<nsINodeInfo> nodeInfo = curChildContent->NodeInfo(); >+ if (nodeInfo->Equals(nsAccessibilityAtoms::item) && >+ nodeInfo->NamespaceEquals(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS))) { >+ if (!--index) { >+ nsIDOMNode *itemNode = nsnull; >+ curChildNode.swap(itemNode); >+ return itemNode; >+ } >+ >+ curChildNode = GetItemByIndex(index, curAccChild); >+ if (curChildNode) { >+ nsIDOMNode *itemNode = nsnull; >+ curChildNode.swap(itemNode); >+ return itemNode; I think we can replace these 5 lines to: nsCOMPtr<nsIAccessible> temp; curAccChild->GetFirstChild(getter_AddRefs(temp)); curAccChild.swap(temp); continue; We can avoid using recursion. >+ } >+ } >+ } >+ } >+ >+ nsCOMPtr<nsIAccessible> nextAccChild; >+ curAccChild->GetNextSibling(getter_AddRefs(nextAccChild)); >+ curAccChild.swap(nextAccChild); >+ } >+ >+ return nsnull; >+} ...... >+ >+ if (nodeinfo->Equals(nsAccessibilityAtoms::select1)) { >+ nsCOMPtr<nsIDOMNode> selitem; >+ rv = sXFormsService->GetSelectedItemForSelect1(select, >+ getter_AddRefs(selitem)); >+ return NS_SUCCEEDED(rv) && selitem == mDOMNode; Nit: suggest add parenthesis i.e. return NS_SUCCEEDED(rv) && (selitem == mDOMNode); >+ } >+ } >+ >+ return PR_FALSE; >+} >+ >Index: accessible/src/xforms/nsXFormsFormControlsAccessible.h >=================================================================== >RCS file: /cvsroot/mozilla/accessible/src/xforms/nsXFormsFormControlsAccessible.h,v >retrieving revision 1.6 >diff -u -8 -p -r1.6 nsXFormsFormControlsAccessible.h >--- accessible/src/xforms/nsXFormsFormControlsAccessible.h 12 Dec 2006 16:19:16 -0000 1.6 >+++ accessible/src/xforms/nsXFormsFormControlsAccessible.h 12 Dec 2006 17:23:19 -0000 >@@ -170,10 +170,77 @@ public: > class nsXFormsSelectAccessible : public nsXFormsContainerAccessible > { > public: > nsXFormsSelectAccessible(nsIDOMNode *aNode, nsIWeakReference *aShell); > > NS_IMETHOD GetState(PRUint32 *aState); > }; > >+ >+ Nit: adds only 1 blank line instead of adding 2 blank lines here, the style in this file is 2 blank lines between classes. (also in nsXFormsFormControlsAccessible.cpp) >+/** >+ * Accessible object for xforms:choices. >+ */ >+ >+class nsXFormsChoicesAccessible : public nsXFormsAccessible >+{ >Index: content/base/public/nsIXFormsUtilityService.h >=================================================================== >RCS file: /cvsroot/mozilla/content/base/public/nsIXFormsUtilityService.h,v >retrieving revision 1.7 >diff -u -8 -p -r1.7 nsIXFormsUtilityService.h >--- content/base/public/nsIXFormsUtilityService.h 12 Dec 2006 16:19:16 -0000 1.7 >+++ content/base/public/nsIXFormsUtilityService.h 12 Dec 2006 17:23:19 -0000 >@@ -16,16 +16,17 @@ ...... >+ >+ /** >+ * Get selected xforms:item element for xforms:select1. Failure if >+ * given 'aElement' node is not xforms:select1. >+ */ Nit: fix the spacing >Index: extensions/xforms/nsXFormsUtilityService.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtilityService.cpp,v >retrieving revision 1.15 >diff -u -8 -p -r1.15 nsXFormsUtilityService.cpp >--- extensions/xforms/nsXFormsUtilityService.cpp 12 Dec 2006 16:19:17 -0000 1.15 >+++ extensions/xforms/nsXFormsUtilityService.cpp 12 Dec 2006 17:23:19 -0000 >+nsresult >+nsXFormsUtilityService::GetSelectChildrenForNodeInternal(nsIDOMNode *aElement, >+ nsNodeList *aNodeList) >+{ >+ nsCOMPtr<nsIDOMNodeList> children; >+ aElement->GetChildNodes(getter_AddRefs(children)); >+ NS_ENSURE_TRUE(children, NS_ERROR_FAILURE); >+ >+ PRUint32 length = 0; >+ children->GetLength(&length); >+ >+ for (PRUint32 i = 0; i < length; i++) { Nit: use index >+ nsCOMPtr<nsIDOMNode> child; >+ children->Item(i, getter_AddRefs(child)); >+ NS_ENSURE_TRUE(child, NS_ERROR_FAILURE); >+ >+ nsCOMPtr<nsIContent> content(do_QueryInterface(child)); >+ NS_ENSURE_TRUE(content, NS_ERROR_FAILURE); >+ >+ nsCOMPtr<nsINodeInfo> nodeInfo(content->NodeInfo()); >+ if (nodeInfo->NamespaceEquals(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS))) { >+ if (nodeInfo->Equals(nsXFormsAtoms::item) || >+ nodeInfo->Equals(nsXFormsAtoms::choices)) { >+ aNodeList->AppendElement(content); >+ } else if (nodeInfo->Equals(nsXFormsAtoms::itemset)) { >+ nsresult rv = GetSelectChildrenForNode(child, aNodeList); >+ NS_ENSURE_SUCCESS(rv, rv); Nit: above 2 lines can be changed to ...... >+void >+nsNodeList::AppendElement(nsIContent *aContent) >+{ >+ mElements.AppendObject(aContent); >+} >+ Nit: don't add blank line at end of fine >Index: extensions/xforms/nsXFormsUtilityService.h >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtilityService.h,v >retrieving revision 1.9 >diff -u -8 -p -r1.9 nsXFormsUtilityService.h >--- extensions/xforms/nsXFormsUtilityService.h 12 Dec 2006 16:19:17 -0000 1.9 >+++ extensions/xforms/nsXFormsUtilityService.h 12 Dec 2006 17:23:19 -0000 >+ >+ NS_IMETHOD GetSelectedItemForSelect1(nsIDOMNode *aElement, >+ nsIDOMNode ** aItem); Nit: keep the same spacing style with "**aItems" below. >+ NS_IMETHOD SetSelectedItemForSelect1(nsIDOMNode *aElement, nsIDOMNode *aItem); >+ >+ NS_IMETHOD GetSelectedItemsForSelect(nsIDOMNode *aElement, >+ nsIDOMNodeList **aItems);

>>+ nsresult rv = GetSelectChildrenForNode(child, aNodeList); >>+ NS_ENSURE_SUCCESS(rv, rv); >Nit: above 2 lines can be changed to I was trying to say return GetSelectChildrenForNode(child, aNodeList); But, your way is better for debugging. You can ignore this comment.

(In reply to comment #15) > (From update of attachment 248400 [details] [diff] [review] [edit]) > >Index: accessible/src/base/nsAccessibilityAtomList.h > >=================================================================== > >RCS file: /cvsroot/mozilla/accessible/src/base/nsAccessibilityAtomList.h,v > >retrieving revision 1.48 > >diff -u -8 -p -r1.48 nsAccessibilityAtomList.h > >--- accessible/src/base/nsAccessibilityAtomList.h 8 Dec 2006 09:57:25 -0000 1.48 > >+++ accessible/src/base/nsAccessibilityAtomList.h 12 Dec 2006 17:23:18 -0000 > >@@ -83,43 +83,47 @@ ACCESSIBILITY_ATOM(tableOuterFrame, "Tab > > ACCESSIBILITY_ATOM(a, "a") > ...... > >+ACCESSIBILITY_ATOM(item, "item") // XForms > >+ACCESSIBILITY_ATOM(itemset, "itemset") // XForms > > ACCESSIBILITY_ATOM(img, "img") > > ACCESSIBILITY_ATOM(input, "input") > > ACCESSIBILITY_ATOM(label, "label") > > The 2 new lines should be added below "input", just above "label" Why? > >+already_AddRefed<nsIDOMNode> > >+nsXFormsSelectableAccessible::GetItemByIndex(PRInt32 aIndex, > >+ nsIAccessible *aAccessible) > > >+ curChildNode = GetItemByIndex(index, curAccChild); > >+ if (curChildNode) { > >+ nsIDOMNode *itemNode = nsnull; > >+ curChildNode.swap(itemNode); > >+ return itemNode; > > I think we can replace these 5 lines to: > nsCOMPtr<nsIAccessible> temp; > curAccChild->GetFirstChild(getter_AddRefs(temp)); > curAccChild.swap(temp); > continue; > > We can avoid using recursion. No, selects has hierarchical structure. For example, <xf:choices> <xf:item/> <xf:choices> <xf:item/> </xf:choices> </xf:choices> > >+void > >+nsNodeList::AppendElement(nsIContent *aContent) > >+{ > >+ mElements.AppendObject(aContent); > >+} > >+ > > Nit: don't add blank line at end of fine Why? cvs diff always remind if I didn't add new line at the end of the file.

(In reply to comment #17) > Created an attachment (id=248900) [edit] > patch5 > > (In reply to comment #15) > > (From update of attachment 248400 [details] [diff] [review] [edit] [edit]) > > >Index: accessible/src/base/nsAccessibilityAtomList.h > > >=================================================================== > > >RCS file: /cvsroot/mozilla/accessible/src/base/nsAccessibilityAtomList.h,v > > >retrieving revision 1.48 > > >diff -u -8 -p -r1.48 nsAccessibilityAtomList.h > > >--- accessible/src/base/nsAccessibilityAtomList.h 8 Dec 2006 09:57:25 -0000 1.48 > > >+++ accessible/src/base/nsAccessibilityAtomList.h 12 Dec 2006 17:23:18 -0000 > > >@@ -83,43 +83,47 @@ ACCESSIBILITY_ATOM(tableOuterFrame, "Tab > > > ACCESSIBILITY_ATOM(a, "a") > > ...... > > >+ACCESSIBILITY_ATOM(item, "item") // XForms > > >+ACCESSIBILITY_ATOM(itemset, "itemset") // XForms > > > ACCESSIBILITY_ATOM(img, "img") > > > ACCESSIBILITY_ATOM(input, "input") > > > ACCESSIBILITY_ATOM(label, "label") > > > > The 2 new lines should be added below "input", just above "label" > > Why? > alphabet order > > >+already_AddRefed<nsIDOMNode> > > >+nsXFormsSelectableAccessible::GetItemByIndex(PRInt32 aIndex, > > >+ nsIAccessible *aAccessible) > > > > > >+ curChildNode = GetItemByIndex(index, curAccChild); > > >+ if (curChildNode) { > > >+ nsIDOMNode *itemNode = nsnull; > > >+ curChildNode.swap(itemNode); > > >+ return itemNode; > > > > I think we can replace these 5 lines to: > > nsCOMPtr<nsIAccessible> temp; > > curAccChild->GetFirstChild(getter_AddRefs(temp)); > > curAccChild.swap(temp); > > continue; > > > > We can avoid using recursion. > > No, selects has hierarchical structure. For example, > <xf:choices> > <xf:item/> > <xf:choices> > <xf:item/> > </xf:choices> > </xf:choices> > I guess my code works just as same as yours without growing stacks. If it not, my fault. > > >+void > > >+nsNodeList::AppendElement(nsIContent *aContent) > > >+{ > > >+ mElements.AppendObject(aContent); > > >+} > > >+ > > > > Nit: don't add blank line at end of fine > > Why? cvs diff always remind if I didn't add new line at the end of the file. > my cvs doesn't remind me, and most of files don't have a new line at end of them.

(In reply to comment #18) > > > >+ACCESSIBILITY_ATOM(item, "item") // XForms > > > >+ACCESSIBILITY_ATOM(itemset, "itemset") // XForms > > > > ACCESSIBILITY_ATOM(img, "img") > > > > ACCESSIBILITY_ATOM(input, "input") > > > > ACCESSIBILITY_ATOM(label, "label") > > > > > > The 2 new lines should be added below "input", just above "label" > > > > Why? > > > alphabet order Oh, do you mean that should be ACCESSIBILITY_ATOM(img, "img") ACCESSIBILITY_ATOM(input, "input") +ACCESSIBILITY_ATOM(item, "item") // XForms +ACCESSIBILITY_ATOM(itemset, "itemset") // XForms ACCESSIBILITY_ATOM(label, "label") Right? > I guess my code works just as same as yours without growing stacks. > If it not, my fault. Sorry, I can see lowering in tree, but how is raising in tree happen? Imo recursion looks obviously :). > > > >+void > > > >+nsNodeList::AppendElement(nsIContent *aContent) > > > >+{ > > > >+ mElements.AppendObject(aContent); > > > >+} > > > >+ > > > > > > Nit: don't add blank line at end of fine > > > > Why? cvs diff always remind if I didn't add new line at the end of the file. > > > my cvs doesn't remind me, and most of files don't have a new line at end of > them. > Just previously I was asked to add new line into end of file :). I guess additional line is not critical thing.

Comment on attachment 248900 [details] [diff] [review] patch5 great ! everything is fine expect the alphabet order my fault about the recursion. Actually, I love recursion, too.

Comment on attachment 248900 [details] [diff] [review] patch5 (In reply to comment #20) > (From update of attachment 248900 [details] [diff] [review] [edit]) > great ! > > everything is fine expect the alphabet order I'll fix this with other Neil's comment.

Comment on attachment 248900 [details] [diff] [review] patch5 >+already_AddRefed<nsIDOMNode> >+nsXFormsSelectableAccessible::GetItemByIndex(PRInt32 aIndex, >+ nsIAccessible *aAccessible) >+{ >+ nsCOMPtr<nsIAccessible> accessible(aAccessible ? aAccessible : this); >+ PRInt32 index = aIndex; >+ >+ nsCOMPtr<nsIAccessible> curAccChild; >+ accessible->GetFirstChild(getter_AddRefs(curAccChild)); >+ >+ while (curAccChild) { >+ nsCOMPtr<nsIAccessNode> curAccNodeChild(do_QueryInterface(curAccChild)); >+ if (curAccNodeChild) { >+ nsCOMPtr<nsIDOMNode> curChildNode; >+ curAccNodeChild->GetDOMNode(getter_AddRefs(curChildNode)); >+ nsCOMPtr<nsIContent> curChildContent(do_QueryInterface(curChildNode)); >+ if (curChildContent) { >+ nsCOMPtr<nsINodeInfo> nodeInfo = curChildContent->NodeInfo(); >+ if (nodeInfo->Equals(nsAccessibilityAtoms::item) && >+ nodeInfo->NamespaceEquals(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS))) { >+ if (!--index) { >+ nsIDOMNode *itemNode = nsnull; >+ curChildNode.swap(itemNode); >+ return itemNode; >+ } >+ >+ curChildNode = GetItemByIndex(index, curAccChild); >+ if (curChildNode) { >+ nsIDOMNode *itemNode = nsnull; >+ curChildNode.swap(itemNode); >+ return itemNode; >+ } >+ } >+ } >+ } >+ >+ nsCOMPtr<nsIAccessible> nextAccChild; >+ curAccChild->GetNextSibling(getter_AddRefs(nextAccChild)); >+ curAccChild.swap(nextAccChild); >+ } >+ >+ return nsnull; >+} What is the recursive call trying to achieve?

(In reply to comment #22) > What is the recursive call trying to achieve? > Accessible tree of select/select1 controls may be: <select> <item/> <choices> <item/> <choices> <item/> </choices> </choices> </select> Therefore I run recursively through out all accessible child nodes to find index of item element.

cc'ing Neil, since I guess he didn't get my answer :)

Comment on attachment 248900 [details] [diff] [review] patch5 >+ nsCOMPtr<nsINodeInfo> nodeInfo = curChildContent->NodeInfo(); >+ if (nodeInfo->Equals(nsAccessibilityAtoms::item) && >+ nodeInfo->NamespaceEquals(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS))) { It might be worthwhile caching the XForms namespace ID and using that.

Comment on attachment 248900 [details] [diff] [review] patch5 Your recursion doesn't look like it will work in this case: <select> <item/> <choices> <item/> <choices> <item/> </choices> <item/> </choices> <item> </select> It will number the items 1, 2, 3, 3, 2 instead of 1, 2, 3, 4, 5. You might find it easier to use a tree walker or content list.

Comment on attachment 249345 [details] [diff] [review] patch6 I'll file new patch

1) GetItemByIndex changes 2) I forgot to implement nsIAccessibleSelectable for nsXFormsSelectable. Fixed

Comment on attachment 249347 [details] [diff] [review] patch7 >+already_AddRefed<nsIDOMNode> >+nsXFormsSelectableAccessible::GetItemByIndex(PRInt32 *aIndex, >+ nsIAccessible *aAccessible) I don't know if it would be better to use a reference rather than a pointer. >+ (*aIndex)--; --*aIndex; works too. >+ curChildNode = GetItemByIndex(aIndex, curAccChild); >+ if (curChildNode) { >+ nsIDOMNode *itemNode = nsnull; >+ curChildNode.swap(itemNode); >+ return itemNode; >+ } It might be simpler to just use the pointer here i.e. nsIDOMNode *itemNode = GetItemByIndex(aIndex, curAccChild); if (itemNode) return itemNode;

for checkin

(In reply to comment #30) > (From update of attachment 249347 [details] [diff] [review]) > >+already_AddRefed<nsIDOMNode> > >+nsXFormsSelectableAccessible::GetItemByIndex(PRInt32 *aIndex, > >+ nsIAccessible *aAccessible) > I don't know if it would be better to use a reference rather than a pointer. I prefer to have a pointer because reference is not very clean here (reference doesn't say nothing this variable will be changed). > >+ (*aIndex)--; > --*aIndex; works too. > > >+ curChildNode = GetItemByIndex(aIndex, curAccChild); > >+ if (curChildNode) { > >+ nsIDOMNode *itemNode = nsnull; > >+ curChildNode.swap(itemNode); > >+ return itemNode; > >+ } > It might be simpler to just use the pointer here i.e. > nsIDOMNode *itemNode = GetItemByIndex(aIndex, curAccChild); > if (itemNode) > return itemNode; > Fixed.

checked in by smaug

(In reply to comment #32) >(reference doesn't say nothing this variable will be changed). * Call by value - in (may be not useful or impossible for classes) * Call by pointer - optional in/out or out * Call by const pointer - optional in * Call by reference - in/out or out * Call by const reference - in (not useful for simple types)