257747 - Implement basic accessible relations

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Louie Zhao]

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

Comment 1

[Louie Zhao]

Attachment: patch v1

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

Comment 2

[Louie Zhao]

Attachment: nsXULSelectAccessibleWrap.cpp

Comment 3

[Louie Zhao]

Attachment: nsXULSelectAccessibleWrap.h

Comment 4

[Aaron Leventhal]

Are you seeking reviews yet?

Comment 5

[Philip K. Warren]

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 6

[Aaron Leventhal]

Comment on attachment 157692 [details] [diff] [review]
patch v1

Minusing until Louie addresses pkwarren's comments.

Comment 7

[Louie Zhao]

Attachment: patch v2

updated patch

Comment 8

[Louie Zhao]

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".

Comment 9

[Louie Zhao]

Attachment: patch v2

Comment 10

[Louie Zhao]

> >   // 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 11

[Philip K. Warren]

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 12

[Aaron Leventhal]

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.

Comment 13

[Louie Zhao]

> 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.

Comment 14

[Louie Zhao]

Attachment: patch v3

add description for relation types

Comment 15

[Aaron Leventhal]

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 16

[Aaron Leventhal]

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.

Comment 17

[Aaron Leventhal]

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

Comment 18

[Bill Haneman]

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!

Comment 19

[Louie Zhao]

Attachment: patch v4

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 20

[Aaron Leventhal]

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.

Comment 21

[Aaron Leventhal]

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.

Comment 22

[Louie Zhao]

Attachment: patch v5

Fix the 2 issues Aaron mentioned.

Comment 23

[Aaron Leventhal]

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

Comment 24

[Louie Zhao]

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

Comment 25

[Aaron Leventhal]

Attachment: 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

Comment 26

[Aaron Leventhal]

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 27

[Louie Zhao]

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.

Comment 28

[Aaron Leventhal]

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 29

[Aaron Leventhal]

Attachment: This patch applies to today's updated trunk and builds

Comment 30

[Aaron Leventhal]

Attachment: This patch applies to today's updated trunk and builds. Ready for review.

Comment 31

[Louie Zhao]

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 32

[Aaron Leventhal]

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 33

[Johnny Stenback (:jst)]

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

sr=jst

Comment 34

[Aaron Leventhal]

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