when gnopernicus read combobox, it will access relation of "labelled_by" and will get the vaule of combobox by AtkTextInterface.

Since I have no access to execute "cvs add", the new files "nsXULSelectAccessibleWrap.h/cpp" can't be included in patch. Attach it seperately.

Are you seeking reviews yet?

Comment on attachment 157692 [details] [diff] [review] patch v1 >Index: src/atk/nsAccessibleText.cpp >=================================================================== >@@ -114,7 +267,7 @@ > PRBool isSelectionCollapsed; > domSel->GetIsCollapsed(&isSelectionCollapsed); > // Don't perform any actions when the selection is not collapsed >- NS_ENSURE_TRUE(isSelectionCollapsed, NS_ERROR_FAILURE); >+ // NS_ENSURE_TRUE(isSelectionCollapsed, NS_ERROR_FAILURE); Can you please explain why this check was removed? >Index: src/base/nsAccessible.cpp >=================================================================== >@@ -1337,6 +1337,54 @@ > return NS_ERROR_NOT_IMPLEMENTED; > } > >+/* nsIAccessible getAccessibleRelated(); */ >+NS_IMETHODIMP nsAccessible::GetAccessibleRelated(PRUint32 aRelationType, nsIAccessible **_retval) >+{ ... >+ nsAutoString label; >+ nsIContent *labelContent = nsnull; >+ nsCOMPtr<nsIDOMXULElement> xulElt(do_QueryInterface(mDOMNode)); >+ nsCOMPtr<nsIDOMHTMLElement> htmlElt(do_QueryInterface(mDOMNode)); Is there a reason why you chose not to use GetXULName and GetHTMLName here? It seems like this contains a lot of duplicated code which is already implemented in those functions. The only difference between GetXULName and your version seems to be that you are checking for the label "undefined". I don't see any significant differences between GetHTMLName and your version. >Index: src/base/nsRootAccessible.cpp >@@ -354,7 +354,7 @@ > > // for focus events on Radio Groups we give the focus to the selected button > nsCOMPtr<nsIDOMXULSelectControlElement> selectControl(do_QueryInterface(targetNode)); >- if (selectControl) { >+ if (selectControl && localName.EqualsIgnoreCase("radiogroup")) { > nsCOMPtr<nsIDOMXULSelectControlItemElement> selectItem; > selectControl->GetSelectedItem(getter_AddRefs(selectItem)); > optionTargetNode = do_QueryInterface(selectItem); This change seems to be separate from all the others in this patch. Was this change meant for another bug? Overall the code looks good.

Comment on attachment 157692 [details] [diff] [review] patch v1 Minusing until Louie addresses pkwarren's comments.

updated patch

Comment on attachment 171738 [details] [diff] [review] patch v2 This patch gets rid of some minor fix and make itself looks simple and clear. It contains 2 main issues: 1. Implementation refRelationSetCB interface in ATK, which is often used by AT-Tools to find a reasonable name for the object through "RELATION_LABELLED_BY". 2. Add AtkText interface to XUL Combobox Accessible because AT-Tool (e.g. gnopernicus) will get the value of combobox through "GetText".

> > // Don't perform any actions when the selection is not collapsed > >- NS_ENSURE_TRUE(isSelectionCollapsed, NS_ERROR_FAILURE); > >+ // NS_ENSURE_TRUE(isSelectionCollapsed, NS_ERROR_FAILURE); > > Can you please explain why this check was removed? > Delete the fix from this patch (has loose connection with this bug. I will file a new bug to deal with it). > Is there a reason why you chose not to use GetXULName and GetHTMLName here? It > seems like this contains a lot of duplicated code which is already implemented > in those functions. The only difference between GetXULName and your version > seems to be that you are checking for the label "undefined". I don't see any > significant differences between GetHTMLName and your version. "RELATION_LABELLED_BY" switch in function "nsAccessible::GetAccessibleRelated" is to find the XUL/HTML element which "label" the current node. So it uses helper function "GetXULLabelContent" and "GetHTMLLabelContent". Although it looks similar with GetXULName/GetHTMLName, they're different. "GetXULLabelContent"/"GetHTMLLabelContent" is only one of the cases in GetXULName/GetHTMLName. Actually, I was thinking to delete "GetXULLabelContent"/"GetHTMLLabelContent" from GetXULName/GetHTMLName because the name got from label will has no much use if AT-Tools can find "RELATION_LABELLED_BY" object. But I can't be sure that AT-Tools always find "RELATION_LABELLED_BY" object before getting accessible name directly from the object. So I suggest to remain "GetXULLabelContent"/"GetHTMLLabelContent" in GetXULName/GetHTMLName first. In this patch, also delete the code to judge "undefined" value because it's not needed any more (as in GetXULName). > >- if (selectControl) { > >+ if (selectControl && localName.EqualsIgnoreCase("radiogroup")) { > > nsCOMPtr<nsIDOMXULSelectControlItemElement> selectItem; > > selectControl->GetSelectedItem(getter_AddRefs(selectItem)); > > optionTargetNode = do_QueryInterface(selectItem); > > This change seems to be separate from all the others in this patch. Was this > change meant for another bug? Delete the fix from this patch (has loose connection with this bug. I will file a new bug to deal with it).

Comment on attachment 171740 [details] [diff] [review] patch v2 Aaron may be able to get to this before me - only thing I see is that the nsIAccessible UUID needs to be updated again when this is checked in.

Comment on attachment 171740 [details] [diff] [review] patch v2 Some questions: Are accessibles monogamous? :) Or, can an accessible have more than 1 other accessible that it has a relationship with? Maybe an accessible can even have more than 1 relationship with the same accessible. For example, it could be labeled by, and also be the child of another accessible. I don't think getAccessibleRelated is enough. What does NODE_CHILD_OF mean, when we already have child nodes? I would like to see the nsIAccessible.idl have some comments that describe the relationships. At the least it should have a link to relevant documentation.

> Are accessibles monogamous? :) Or, can an accessible have more than 1 other > accessible that it has a relationship with? Maybe an accessible can even have > more than 1 relationship with the same accessible. For example, it could be > labeled by, and also be the child of another accessible. I don't think > getAccessibleRelated is enough. Accessibles are not mongamous. One accessible can have relationship with many other accessible. nsAccessible::GetAccessibleRelated is designed to return the accessible with specified relationship (by argument aRelationType). NS_IMETHODIMP nsAccessible::GetAccessibleRelated(PRUint32 aRelationType, nsIAccessible **_retval) All the relation type come from ATK and have been defined in mozilla. I will add explaination for them. Currently, only RELATION_LABELLED_BY is widely used and have major compact on usability (to give accessible a reasonable name). That's the reason only RELATION_LABELLED_BY is implemented at present. If we need other relationship, we can add in "nsAccessible::GetAccessibleRelated" easily in the future. > > What does NODE_CHILD_OF mean, when we already have child nodes? > > I would like to see the nsIAccessible.idl have some comments that describe the > relationships. At the least it should have a link to relevant documentation. > Not investigate this :-) because it's never been used in current Apps on Linux. Anyway, I'll add specific description for these relation types if they're available.

add description for relation types

Comment on attachment 171967 [details] [diff] [review] patch v3 Please use index or count instead a single lettter variable like i. It makes it easier to edit the code later.

Comment on attachment 171967 [details] [diff] [review] patch v3 Louie and I talked about this patch. He is going to integrate nsAccessibleLabel into nsAccessibleText by using the label node for the text node.

Louie also mentioned that he would change the name of nsAccessibleText to nsAccessibleTextMixin or nsAccessibleTextHelper so that things wouldn't be so confusing, for example in nsAccessibleText.h class nsTextAccessibleWrap : public nsTextAccessible, public nsAccessibleText

RELATION_LABELLED_BY is not the only vital relationship. For one thing, we need the reciprocal RELATION_LABEL_FOR, this is essential. Other very important relationships are RELATION_CONTROLLER_FOR/RELATION_CONTROLLED_BY (for things like sliders and scrollbars), RELATION_POPUP_FOR/RELATION_PARENT_WINDOW_OF, RELATION_FLOWS_FROM/RELATION_FLOWS_TO (text only). All these are reciprocal. Louie is right that NODE_CHILD_OF and the EMBEDS/EMBEDDED_BY relations are seldom used at this time, as is MEMBER_OF. But MEMBER_OF can be important since it provides grouping information, for instance in radio button groups. Whatever is done for nsiAccessible, it's vital that the api can provide for many-to-many relationship mapping of more than one type. thanks!

The new patch only implements RELATION_LABELLED_BY and I will fix "nsAccessibleLable" issue in new bug. Bill, thanks very much for your information. This patch has already considered the future adding of new relations. Expanding the code here can add new acc relation for mozilla. I'll implement relations other than "LABELLED_BY" in new bugs.

Comment on attachment 181741 [details] [diff] [review] patch v4 Louie, this is almost ready. Can you change GetAccessibleRelated() so that it only gets the accessibility service and creates the accessible in one place after the switch statement? Just initialize nsCOMPtr<nsIDOMNode> relatedNode before the switch, and check to see if it is non-null afterwards. Also, is it really necessary to get the label content and check to see if it is empty? Why not just expose the label relationship and let the assistive technology check the label to see if it is empty or not. When there is a <label> set but it is empty, a screen reader would know that is intentional and not try to use a heuristic.

Changing bug summary to more accurately reflect what it is for. It is not specific to combo boxes -- it is for implementing relations in general.

Fix the 2 issues Aaron mentioned.

Louie I am working on a modified patch that allows these relations to also be exposed in MSAA. Will post it within a day.

That's great for MSAA module to make use of nsAccessible::GetAccessibleRelated interface. Waiting for your patch.

Neil, this adds support for a control attribute on xul:description, similar to the control attribute in xul:label. All it does is tie the description to a XUL form control for accessibility purposes. When that form control gets focused we can expose a description for it, so that a screen reader can read it for a blind user. It's an optional attribute.

Comment on attachment 184559 [details] [diff] [review] 1) Buld on Louie's patch & fixes for bug 294834 and bug 290344, 2) Add MSAA constants, 3) Add support for more relations, 4) Add support for ATK_STATE_DEFAULT, 5) Fix GetName() for xul:label >+ // Relationships are defined on the same content node >+ // that the role would be defined on >+ nsIContent *content = GetRoleContent(mDOMNode); (Against the current trunk) There is no definition of GetRoleContent() in this patch. I suppose you have missed some part.

Louie, this builds on the reviewed patches in these bugs: bug 294834 and bug 290344 Please take a look at those bugs to see the impl for GetRoleContent() and other missing pieces. Once the tree opens for 1.8b3 we can get them in and it will be easier to review this patch.

Comment on attachment 185021 [details] [diff] [review] This patch applies to today's updated trunk and builds. Ready for review. The patch is good on Linux. Aaron, since "LABEL_FOR" has been implemented, can you help to integrated this relationship into ATK part when you check in. The change is only 1 line. Thanks. Change >+ PRUint32 relationType[1] = {nsIAccessible::RELATION_LABELLED_BY}; to + PRUint32 relationType[2] = {nsIAccessible::RELATION_LABELLED_BY, + nsIAccessible::RELATION_LABEL_FOR};

Comment on attachment 185021 [details] [diff] [review] This patch applies to today's updated trunk and builds. Ready for review. Okay, Louie. I will fix that for ATK, thanks.

Comment on attachment 185021 [details] [diff] [review] This patch applies to today's updated trunk and builds. Ready for review. sr=jst

Checking in public/nsIAccessible.idl; /cvsroot/mozilla/accessible/public/nsIAccessible.idl,v <-- nsIAccessible.idl new revision: 1.39; previous revision: 1.38 done Checking in src/atk/nsAccessibleWrap.cpp; /cvsroot/mozilla/accessible/src/atk/nsAccessibleWrap.cpp,v <-- nsAccessibleWrap.cpp new revision: 1.25; previous revision: 1.24 done Checking in src/base/nsAccessibilityAtomList.h; /cvsroot/mozilla/accessible/src/base/nsAccessibilityAtomList.h,v <-- nsAccessibilityAtomList.h new revision: 1.23; previous revision: 1.22 done Checking in src/base/nsAccessible.cpp; /cvsroot/mozilla/accessible/src/base/nsAccessible.cpp,v <-- nsAccessible.cpp new revision: 1.152; previous revision: 1.151 done Checking in src/base/nsAccessible.h; /cvsroot/mozilla/accessible/src/base/nsAccessible.h,v <-- nsAccessible.h new revision: 1.64; previous revision: 1.63 done Checking in src/msaa/nsAccessibleWrap.cpp; /cvsroot/mozilla/accessible/src/msaa/nsAccessibleWrap.cpp,v <-- nsAccessibleWrap.cpp new revision: 1.29; previous revision: 1.28 done Checking in src/xul/nsXULTextAccessible.cpp; /cvsroot/mozilla/accessible/src/xul/nsXULTextAccessible.cpp,v <-- nsXULTextAccessible.cpp new revision: 1.12; previous revision: 1.11 done