AT-SPI getLinkIndex doesn't work

RESOLVED FIXED

Status

()

Firefox
Disability Access
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Willie Walker, Assigned: Ginn Chen)

Tracking

({access, fixed1.8.1})

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051214 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051214 Firefox/1.6a1

The accessible hypertext implementation doesn't appear to support getLinkIndex.


Reproducible: Always

Steps to Reproduce:
1) Save the following html to foo.html:

<html>
<p>This is a <a href="foo1">test</a> and here is <a href="foo2">another test</a>.</p>
</html>

2) Run this tool in an xterm
3) Run Firefox and point it to foo.html
4) Tab between the two links in foo.html and observe the output in the xterm

Actual Results:  
You will see the following outpu:

object:link-selected -1 0 <CORBA.any of type 'IDL:omg.org/CORBA/Null:1.0'>
 linkIndex at 11: -1
 linkIndex at 30: -1


Expected Results:  
Character offset 11 in foo.html is in the 'test' link
and getLinkIndex(11) should thus return a link index
of 0.  Likewise, character offset 30 in foo.html is in
the 'another test' link and getLinkIndex(3) should thus
return a link index of 1.
(Reporter)

Comment 1

13 years ago
Created attachment 205961 [details]
Standalone test case
(Assignee)

Comment 2

13 years ago
480 /* long getLinkIndex (in long charIndex); */
481 NS_IMETHODIMP nsAccessibleHyperText::GetLinkIndex(PRInt32 aCharIndex, PRInt32 *aLinkIndex)
482 {
483   return NS_ERROR_NOT_IMPLEMENTED;
484 }
Assignee: nobody → ginn.chen
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 3

13 years ago
Created attachment 206581 [details] [diff] [review]
patch

Implement the interface.
Also remove the evil hacking for gnopernicus.
We do not care about caret position when getting link numbers of hypertext.
Attachment #206581 - Flags: review?(aaronleventhal)
(Assignee)

Comment 4

13 years ago
Created attachment 206682 [details] [diff] [review]
patch v2 (also fixed bug 320387 and 320395)
Attachment #206581 - Attachment is obsolete: true
Attachment #206682 - Flags: review?(aaronleventhal)
Attachment #206581 - Flags: review?(aaronleventhal)

Comment 5

13 years ago
Comment on attachment 206682 [details] [diff] [review]
patch v2 (also fixed bug 320387 and 320395)

I'm giving r+ although the proposal is to give links their own objects -- see http://www.mozilla.com/access/unix/new-atk.html

In GetLinkIndex()
Please put in a couple more comments, something like:
// Get the text node for the char index
// If it's a link node, find out what # child the text node is
// Check each text node in the list up until that one, counting the links

+  nsIDOMNode* domNode = FindTextNodeByOffset(aCharIndex, beforeLength);
what's beforeLength?
One technique is to comment that it's not used or call it beforeLength_unused

GetSelectedLinkIndex
+    if (linkNode) {
+      if (linkNode == nsAccessNode::gLastFocusedNode) {
No need for nested if. Just use &&

+  nsCOMPtr<nsIDOMNode> parentNode = nsnull;
+  nsCOMPtr<nsILink> link = nsnull;
No need to declare as null. nsCOMPtr<> does that automatically

+  if (domNode && GetLinkNode(domNode)) {
+    if (NS_SUCCEEDED(mTextChildren->IndexOf(0, domNode, &nodeIndex))) {
use && instead of nested if's
Also, extra null check unnecessary
Attachment #206682 - Flags: review?(aaronleventhal) → review+
(Assignee)

Comment 6

13 years ago
Created attachment 213552 [details] [diff] [review]
patch v3 (addressing Aaron's comment)

+    if (linkNode) {
+      if (linkNode == nsAccessNode::gLastFocusedNode) {
didn't change, because we have to use nested if here, we have linkcount++; between   two right braces.
Attachment #213552 - Flags: superreview?(roc)
I don't really understand what's going on here, but have you tested

<a href="..."><b>Hello</b> <b>Kitty</b></a>
<a href="..."><a href="...">Hello</a> <a href="...">Kitty</a></a>
?
(Assignee)

Comment 8

13 years ago
Interesting test case, but I don't think it relates to this bug.

Currently
<a href="..."><b>Hello</b> <b>Kitty</b></a>
are treated as 3 links, "Hello", space, "Kitty" are independent dom nodes.
They're all reflected to index 0-5 ("Hello") 

We can solve this kind of problem in our new proposal.
(http://www.mozilla.org/access/unix/new-atk.html)
Attachment #213552 - Flags: superreview?(roc) → superreview+
(Assignee)

Updated

12 years ago
Attachment #213552 - Flags: approval-branch-1.8.1?(aaronleventhal)
(Assignee)

Comment 9

12 years ago
Checking in src/atk/nsAccessibleHyperText.cpp;
/cvsroot/mozilla/accessible/src/atk/nsAccessibleHyperText.cpp,v  <--  nsAccessibleHyperText.cpp
new revision: 1.31; previous revision: 1.30
done
Checking in src/atk/nsAccessibleHyperText.h;
/cvsroot/mozilla/accessible/src/atk/nsAccessibleHyperText.h,v  <--  nsAccessibleHyperText.h
new revision: 1.6; previous revision: 1.5
done
Checking in src/base/nsRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v  <--  nsRootAccessible.cpp
new revision: 1.141; previous revision: 1.140
done
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Attachment #213552 - Flags: approval-branch-1.8.1?(aaronleventhal) → approval-branch-1.8.1+
(Assignee)

Comment 10

12 years ago
Checking in src/atk/nsAccessibleHyperText.h;
/cvsroot/mozilla/accessible/src/atk/nsAccessibleHyperText.h,v  <--  nsAccessibleHyperText.h
new revision: 1.4.8.1; previous revision: 1.4
done
Checking in src/atk/nsAccessibleHyperText.cpp;
/cvsroot/mozilla/accessible/src/atk/nsAccessibleHyperText.cpp,v  <--  nsAccessibleHyperText.cpp
new revision: 1.25.8.3; previous revision: 1.25.8.2
done
Checking in src/base/nsRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v  <--  nsRootAccessible.cpp
new revision: 1.130.2.8; previous revision: 1.130.2.7
done
Keywords: access, fixed1.8.1
You need to log in before you can comment on or make changes to this bug.