Implement basic accessible relations

RESOLVED FIXED

Status

()

RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Louie.Zhao, Assigned: Louie.Zhao)

Tracking

({access})

Trunk
x86
Linux
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: sunport17)

Attachments

(1 attachment, 10 obsolete attachments)

(Assignee)

Description

14 years ago
when gnopernicus read combobox, it will access relation of "labelled_by" and
will get the vaule of combobox by AtkTextInterface.
(Assignee)

Comment 1

14 years ago
Created attachment 157692 [details] [diff] [review]
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.
(Assignee)

Comment 2

14 years ago
Created attachment 157693 [details] [diff] [review]
nsXULSelectAccessibleWrap.cpp
(Assignee)

Comment 3

14 years ago
Created attachment 157694 [details] [diff] [review]
nsXULSelectAccessibleWrap.h

Comment 4

14 years ago
Are you seeking reviews yet?
(Assignee)

Updated

14 years ago
Attachment #157692 - Flags: review?(pkwarren)

Comment 5

14 years ago
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

14 years ago
Comment on attachment 157692 [details] [diff] [review]
patch v1

Minusing until Louie addresses pkwarren's comments.
Attachment #157692 - Flags: review?(pkwarren) → review-

Updated

14 years ago
Blocks: 263575

Updated

14 years ago
Whiteboard: sunport17
(Assignee)

Comment 7

14 years ago
Created attachment 171738 [details] [diff] [review]
patch v2

updated patch
Attachment #157692 - Attachment is obsolete: true
Attachment #157693 - Attachment is obsolete: true
Attachment #157694 - Attachment is obsolete: true
(Assignee)

Comment 8

14 years ago
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".
Attachment #171738 - Flags: review?(pkwarren)
(Assignee)

Comment 9

14 years ago
Created attachment 171740 [details] [diff] [review]
patch v2
Attachment #171738 - Attachment is obsolete: true
Attachment #171740 - Flags: review?(pkwarren)
(Assignee)

Updated

14 years ago
Attachment #171738 - Flags: review?(pkwarren)
(Assignee)

Comment 10

14 years ago
> >   // 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

14 years ago
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.
Attachment #171740 - Flags: review?(pkwarren) → review?(aaronleventhal)

Comment 12

14 years ago
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.
Attachment #171740 - Flags: review?(aaronleventhal) → review-
(Assignee)

Comment 13

14 years ago
> 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.



(Assignee)

Comment 14

14 years ago
Created attachment 171967 [details] [diff] [review]
patch v3

add description for relation types
Attachment #171740 - Attachment is obsolete: true

Comment 15

14 years ago
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

14 years ago
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.
Attachment #171967 - Flags: review-

Comment 17

14 years ago
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

14 years ago
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!
(Assignee)

Comment 19

14 years ago
Created attachment 181741 [details] [diff] [review]
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.
Attachment #171967 - Attachment is obsolete: true
Attachment #181741 - Flags: review?(aaronleventhal)

Comment 20

14 years ago
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.
Attachment #181741 - Flags: review?(aaronleventhal) → review-

Comment 21

14 years ago
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.
Summary: [ATK] Implement "Relation" and Text interface for combobox → Implement basic accessible relations
(Assignee)

Comment 22

14 years ago
Created attachment 184395 [details] [diff] [review]
patch v5

Fix the 2 issues Aaron mentioned.
Attachment #181741 - Attachment is obsolete: true
Attachment #184395 - Flags: review?(aaronleventhal)

Comment 23

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

Updated

14 years ago
Attachment #184395 - Flags: review?(aaronleventhal) → review-
(Assignee)

Comment 24

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

Comment 25

14 years ago
Created 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
Attachment #184395 - Attachment is obsolete: true
Attachment #184559 - Flags: review?(Louie.Zhao)

Comment 26

14 years ago
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.
(Assignee)

Comment 27

14 years ago
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

14 years ago
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.

Updated

14 years ago
Attachment #184559 - Attachment is obsolete: true
Attachment #184559 - Flags: review?(Louie.Zhao)

Comment 29

14 years ago
Created attachment 185019 [details] [diff] [review]
This patch applies to today's updated trunk and builds
Attachment #185019 - Flags: review?(Louie.Zhao)

Updated

14 years ago
Attachment #185019 - Attachment is obsolete: true
Attachment #185019 - Flags: review?(Louie.Zhao)

Comment 30

14 years ago
Created attachment 185021 [details] [diff] [review]
This patch applies to today's updated trunk and builds. Ready for review.
Attachment #185021 - Flags: review?(Louie.Zhao)
(Assignee)

Comment 31

14 years ago
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};
Attachment #185021 - Flags: review?(Louie.Zhao) → review+

Comment 32

14 years ago
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.
Attachment #185021 - Flags: superreview?(neil.parkwaycc.co.uk)

Updated

14 years ago
Attachment #185021 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview?(jst)
Comment on attachment 185021 [details] [diff] [review]
This patch applies to today's updated trunk and builds. Ready for review.

sr=jst
Attachment #185021 - Flags: superreview?(jst) → superreview+

Updated

14 years ago
Attachment #185021 - Flags: approval-l10n?

Updated

14 years ago
Attachment #185021 - Flags: approval-l10n? → approval1.8b3?

Updated

14 years ago
Attachment #185021 - Flags: approval1.8b3? → approval1.8b3+

Comment 34

14 years ago
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
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.