Closed
Bug 257747
Opened 20 years ago
Closed 19 years ago
Implement basic accessible relations
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: Louie.Zhao, Assigned: Louie.Zhao)
References
Details
(Keywords: access, Whiteboard: sunport17)
Attachments
(1 file, 10 obsolete files)
32.95 KB,
patch
|
Louie.Zhao
:
review+
jst
:
superreview+
mkaply
:
approval1.8b3+
|
Details | Diff | Splinter Review |
when gnopernicus read combobox, it will access relation of "labelled_by" and
will get the vaule of combobox by AtkTextInterface.
Assignee | ||
Comment 1•20 years ago
|
||
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•20 years ago
|
||
Assignee | ||
Comment 3•20 years ago
|
||
Comment 4•20 years ago
|
||
Are you seeking reviews yet?
Assignee | ||
Updated•20 years ago
|
Attachment #157692 -
Flags: review?(pkwarren)
Comment 5•20 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•20 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•20 years ago
|
Whiteboard: sunport17
Assignee | ||
Comment 7•20 years ago
|
||
updated patch
Attachment #157692 -
Attachment is obsolete: true
Attachment #157693 -
Attachment is obsolete: true
Attachment #157694 -
Attachment is obsolete: true
Assignee | ||
Comment 8•20 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•20 years ago
|
||
Attachment #171738 -
Attachment is obsolete: true
Attachment #171740 -
Flags: review?(pkwarren)
Assignee | ||
Updated•20 years ago
|
Attachment #171738 -
Flags: review?(pkwarren)
Assignee | ||
Comment 10•20 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•20 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•20 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•20 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•20 years ago
|
||
add description for relation types
Attachment #171740 -
Attachment is obsolete: true
Comment 15•20 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•20 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•20 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•20 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•20 years ago
|
||
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•20 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•20 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•20 years ago
|
||
Fix the 2 issues Aaron mentioned.
Attachment #181741 -
Attachment is obsolete: true
Attachment #184395 -
Flags: review?(aaronleventhal)
Comment 23•20 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•20 years ago
|
Attachment #184395 -
Flags: review?(aaronleventhal) → review-
Assignee | ||
Comment 24•20 years ago
|
||
That's great for MSAA module to make use of nsAccessible::GetAccessibleRelated
interface. Waiting for your patch.
Comment 25•20 years ago
|
||
Attachment #184395 -
Attachment is obsolete: true
Attachment #184559 -
Flags: review?(Louie.Zhao)
Comment 26•20 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•20 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•20 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•20 years ago
|
Attachment #184559 -
Attachment is obsolete: true
Attachment #184559 -
Flags: review?(Louie.Zhao)
Comment 29•20 years ago
|
||
Attachment #185019 -
Flags: review?(Louie.Zhao)
Updated•20 years ago
|
Attachment #185019 -
Attachment is obsolete: true
Attachment #185019 -
Flags: review?(Louie.Zhao)
Comment 30•20 years ago
|
||
Attachment #185021 -
Flags: review?(Louie.Zhao)
Assignee | ||
Comment 31•20 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•20 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•19 years ago
|
Attachment #185021 -
Flags: superreview?(neil.parkwaycc.co.uk) → superreview?(jst)
Comment 33•19 years ago
|
||
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•19 years ago
|
Attachment #185021 -
Flags: approval-l10n?
Updated•19 years ago
|
Attachment #185021 -
Flags: approval-l10n? → approval1.8b3?
Updated•19 years ago
|
Attachment #185021 -
Flags: approval1.8b3? → approval1.8b3+
Comment 34•19 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
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•