150603 - Implementation of nsIAccessibleHyperLink

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Gilbert Fang]

see bugsceri 24.

Comment 1

[Kyle Yuan]

Jessie will be in charge of this bug.

Comment 2

[Gilbert Fang]

isValid, why a link may be invalid? because maybe the document is changed , then
 accessible object should be  reconstructed on the document. And before that
re-construction, the accessbile object--hyperlink  of course is not valid. 
In java, every time it creates the hyperlink, it constructs an internal data
structure to record the access information. If the document is changed, these
information is sure out of date. To avoid this problem, there is also a listener
to   listen all the change event to the document and set the isValid a correct
status. 
I think ATK have to have the same mechanism. But in mozilla, every access
information is based on DOM tree, and DOM tree is dynamically constructed and is
always updated to the lastest. So, the hyperlink information will be sure always
VALID.  

Comment 3

[Gilbert Fang]

nsIAccessibleHyperLink should be implemented separately by nsIAccessible. 
nsIAccessibleHyperLink-->nsHyperLinkAcessible.
I will add new files in mozilla/accessible/src/base and maybe add new imethod in
the accessiblity servcie for creating a nsHyperLinkAccessible

Comment 4

[Gilbert Fang]

So unfortunately, in jdk 1.3's document, the only class which implements
AccessibleHyperlink is
JEditorPane.JEditorPaneAccessibleHypertextSupport.HTMLLink
(http://java.sun.com/j2se/1.3/docs/api/javax/swing/JEditorPane.JEditorPaneAccessibleHypertextSupport.HTMLLink.html)
And this class actually does not support any multi-anchor hyperlink. 
See its codes:
	    public int getAccessibleActionCount() {
		return 1;
	    }
and 
	    public Object getAccessibleActionObject(int i) {
		if (i == 0 && isValid() == true) {
		    AttributeSet as = element.getAttributes();
		    AttributeSet anchor = 
			(AttributeSet) as.getAttribute(HTML.Tag.A);
		    String href = (anchor != null) ?
			(String) anchor.getAttribute(HTML.Attribute.HREF) : null;
		    if (href != null) {
			URL u;
			try {
			    u = new URL(JEditorPane.this.getPage(), href);
			} catch (MalformedURLException m) {
			    u = null;
			}
			return u;
		    }
		}
		return null;  // link invalid or i != 0
	    }
It always return 1 for count and only support the index 0.
So, if we have such kind of hypertext, how  do we construct anchors for it?
<a> first part <img src=second.img> third part</a>
Now, in mozilla, it will be exposed as a link accNode(ROLE_LINK) with three
accChildren, accNodeA (ROLE_TEXT) for " first part", accNodeB(ROLE_GRAPHIC) for
the "second.img" , accNodeC(ROLE_TEXT) for "third part".
In my opinion, it should be a hyperlink with *3* anchors with the same
action(url). Although it actually has just one tag <a href=>, it will be showed
as three part in user interface in *3* area. 
Hi, Bill and Aron and John, could u tell what your suggestions are ?

Comment 5

[Gilbert Fang]

 About the attribute startIndex/endIndex:
 From my points of view, they should be the index beginning at  the hypertext
which this hyperlink associated with. Because in mozilla, the document actually
is represented by a DOM tree, the associated hypertext actually is one of the
DOM node which is the a parent or  parent of parent  ... of that hyperlink which
is also a DOM node.
 If I am right, a nsIAccessibleHyperlink object  is depend on two DOM node, one
is for the hyperlink itself , such as in html , "<a href=first link> first
link</a>", another is the hypertext which contains hyperlink, in html, maybe 
the "<div>first line <br> <a href=first link> first link</a> </div>". Suppose
DOMNodeA represents  that <a> html element , DOMNodeB represents the <div> html
element  and the nsHyperL ink can be constructed by nsHyperLink(DOMNode,
HyperTextDOMNode), then accHyperLinkA=new nsHyperLink(DOMNodeA, DOMNodeA) and
accHyperLinkB=new nsHyperLink(DOMNodeA, DOMNodeB) will are the different
objects. Their startIndex and endIndex are surely different, although they in
fact from the same DOM node --"<a href=first link> first link</a>".


 Maybe that is a little confusing. But I think it is because mozilla has
different architecture from java and ATK. Java and ATK is based on some kind of
widget to expose their information. But mozilla is based on CONTENTS, which is
more powerful and more complicated.

Comment 6

[Gilbert Fang]

How can I make a patch for new files ? I have no check-in permissions, so I
could not use " cvs add " first, what should I do then? Do I need to file a bug
for add new files first ?

Comment 7

[Gilbert Fang]

Attachment: a simple testcase

Comment 8

[Gilbert Fang]

Attachment: nsHyperLinkAceessible(patch_bz15063_20020623_b)

Add a new class [xpcom] nsHyperLinkAceessible to implement the
nsIAccseeibleHyperLink interface.

Comment 9

[Gilbert Fang]

need r=?

Comment 10

[Kyle Yuan]

Aaron, I already reviewed this patch in our internal bugzilla. Could you take a 
look at that too?

Comment 11

[Aaron Leventhal]

Can someone provide a summary of how the questions about this interface have
been resolved?

I haven't finished reading all the emails about it, a summary would help me.

Comment 12

[Gilbert Fang]

Summary for this bug. 
1) isValid is always be true at this time. Because now we get information from
DOM node directly which are always Valid.
2) startIndex and endIndex are not implemented in this patch. I am not sure
whether it is useful now , and we can put it later.
3) multi-anchor hyperlink (e.g. client-side map ) are supported .
4) Note: hyperlink is  some different from links now in mozilla,especially with
client-side map . The client-side map in ATK'view will be *ONE* hyperlink with
one or more anchors(links). it is more powerful. 
5) hyperlink is different from other accNode. It depends on the hypertext it is
associated. That is it will own two DOM nodes, one is for itself's information,
another is which it associated. That associated DOM node will affects its
attributes of startIndex/endIndex although I have not implemented it now.  

Comment 13

[Aaron Leventhal]

Gilbert, I do not understand item 5 in your summary.

Also, you need to build your patch with 
cvs diff -uN
after doing a cvs add
This will allow us to see the new files in your patch. It is missing some files.

What are startIndex and endIndex meant/intended to be in any other application?
I don't understand them.

What is the aAssociatedDOMNode, and how does it differ from aDOMNode? Please add
some comments to the code that explain.

Comment 14

[Gilbert Fang]

Attachment: nsHyperLinkAceessible(patch_bz150603_20020626_c)

I am sorry, Aaronl. This patch includes the new files.

Comment 15

[Kyle Yuan]

This patch implemented the hyperlink interface for ATK's point of view. It's 
important but may get confused with current hyperlink interface. After a long 
discussion with Gilbert and Jay, we decide to simplify the work by improving 
current hyperlink rather than introduce a new class. That would be more simple 
and reasonable. Gilbert is going to revise his patch and will re-post it soon.

Comment 16

[Aaron Leventhal]

So how will Mozilla implement ATK's hyperlink interface? Will it do the extra
work in widget/src/gtk2/mai?

Comment 17

[Jay Yan]

Aaronl:

Mozilla's implementation of Hyperlink is similar with all the other interfaces,
that is:

Gilbert's patch is crossplatform code, it will be put on XP part
Bolian's patch will be put into /gtk2/mai to translate mozilla's XP interface
into ATK interface

There is one special thing for hyperlink, please see
http://bugzilla.mozilla.org/show_bug.cgi?id=151114#c1

Comment 18

[Gilbert Fang]

Yes, with kyle's suggestion, I will only implement this interface for the basic
usage.And that will make the code simple, less-risky. As we expect, that will
meet the needs of ATs in the near future.  

Comment 19

[Gilbert Fang]

Attachment: extent the nsHTMLLinkAccessible to support nsIAccessibleHyperLink

startIndex/endIndex is not implemented in this patch.

Comment 20

[Gilbert Fang]

Attachment: final patch(patch_bz150603_20020627_d)

This patch extents the nsHTMLLinkAccessible and creates a new class
nsHTMLImageMapAccessible inherited from nsHTMLImageAccessible.
The two class support the nsIAccessibleHyperLink.
startIndex and endIndex are  not valuable to be implemented now.
need r=?

Comment 21

[Gilbert Fang]

Attachment: Simple test case

Here is a simple test case in java script.
It can show hyperlinks' attributes.

Comment 22

[Aaron Leventhal]

Attachment: This is a really useful description of ATK's hypertext and hyperlink concepts from Gilber Fang of Sun Microsystems

Comment 23

[Aaron Leventhal]

Comment on attachment 89391 [details] [diff] [review]
final patch(patch_bz150603_20020627_d)

Why are you not using nsCOMPtr<nsIDOMHTMLCollection> and nsCOMPtr<nsIDOMNode>?

Also,
+	 if (NS_SUCCEEDED(rv)) {
+	   rv = NS_NewURI(_retval, hrefValue, nsnull, baseURI);
+	   return rv;
+	 }
can be
+	 if (NS_SUCCEEDED(rv))
+	   return NS_NewURI(_retval, hrefValue, nsnull, baseURI);

You can declare and define rv at the same time for the first use, so you do not
need a separate line nsresult rv. You can do nsresult rv =
content->GetDocument(...);
If it were me, I wouldn't bother using rv as a temporary at all since you don't
ever use tje value. I would just use if (NS_SUCCEEDED(|getter|)) {...}
I do not see any support for text hyperlinks. Is this bug supposed to be only
for image maps?

Comment 24

[Gilbert Fang]

Hi, aaron, 
Thanks for your comments.
Here is my reply:
1) For nsIDOMHTMLCollection and  nsIDOMNode,  at the beginning, I really wanted
to use nsCOMPtr<nsIDOMHTMLCollection> and nsCOMPtr<nsIDOMNode>, but I found that
in
http://lxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLImageAccessible.cpp
151   nsIDOMHTMLCollection *mapAreas;
152   mMapElement->GetAreas(&mapAreas);
153   if (!mapAreas) 
154     return nsnull;
155 
156   nsIDOMNode *domNode = nsnull;
157   mapAreas->Item(areaNum,&domNode);
158   if (!domNode)
159     return nsnull;
So I wrote the code in the same way to see whether you will comment on it. :-\)
and the result is these code may be also not in good style. Anyway, I will
modified it according to your suggestion in the new patch.
2) For the temporary rv, the main reason is I do not want to make that line too
long to beyond 80 characters. jst's review tools always complains that my codes
have some lines beyond 80 characters. But for this line, you are quite right. I
change it at once. 
3)For text link, or in my word "simple link"/"single-anchor link", it has been
supported in this patch. I extend the nsHTMLLinkAccessible , and this class now
support the nsIAccessibleHyperLink. I think all text link(<a href></a>) will be
accessible by this class. If you try my simple test case
(http://bugzilla.mozilla.org/attachment.cgi?id=89392&action=view)
you will find the accNode for text link can also be QueryInterfaced by
nsIAccessibleHyperLink and you will get a  nsIAccessibleHyperLink object.

4) For my description, I want to  add some words now:
I will implement the hypertext for the whole content pane, that is to extend
nsRootAccessible to support the nsIAccessibleHyperText. This is the most
rational solution now. If ATs think it is not enough, then we will discuss what
kind of user case we should support and file new bugs for  it.

Thanks again, Aaron.

Comment 25

[Gilbert Fang]

Attachment: modified patch according to aaron comments(patch_bz150603_20020628_c)

need r=?

Comment 26

[Kyle Yuan]

Comment on attachment 89495 [details] [diff] [review]
modified patch according to aaron comments(patch_bz150603_20020628_c)

r=kyle

Comment 27

[Aaron Leventhal]

r=aaronl

The bad code that doesn't use nsCOMPtr<> was probably written by me when I had
no experience yet, or maybe I cut and pasted it from someone else's bad code. We
should use nsCOMPtr<>

Comment 28

[Aaron Leventhal]

Gilbert, can you do me a favor and also fix the bad code in
nsHTMLImageAccessible that you copied from? (The one that doesn't use
nsCOMPtr<>). You don't need a new patch for that, since it's the same code.

Comment 29

[Gilbert Fang]

okay, aaron, it is my pleasure. 

Comment 30

[Gilbert Fang]

Attachment: patch addtionally polishing some old bad style code (patch_bz150603_20020628_d)

this patch is almost the same with the patch 89495, in addition to  modifing
some old bad style code.

Comment 31

[Gilbert Fang]

Comment on attachment 89522 [details] [diff] [review]
patch addtionally polishing  some old bad style code (patch_bz150603_20020628_d)

from the part of aaronl

Comment 32

[Johnny Stenback (:jst)]

Comment on attachment 89522 [details] [diff] [review]
patch addtionally polishing  some old bad style code (patch_bz150603_20020628_d)

- In nsAccessibilityService.cpp:

+  nsCOMPtr<nsIDOMElement> domElement(do_QueryInterface(node));
+  NS_ASSERTION(domElement, "It is impossible that No domElement for img
node!");
+  if (domElement) {
...
+  }
+  else
+    *_retval = new nsHTMLImageAccessible(node, weakShell);
+

You assert that it is impossible for domElement to be null here, yet you code
for the case where it is? Either the assertion is wrong, or you should loose
the else case in that if.

   if (! *_retval) 
     return NS_ERROR_OUT_OF_MEMORY;

- In nsHTMLImageAccessible.cpp:

+#include "nsIURI.h"
...
+#include "nsNetUtil.h"

nsNetUtil.h #includes nsIURI.h for you, so no need to #include nsIURI.h here.

- In nsHTMLImageAccessible.h:

+public:
+  nsHTMLImageMapAccessible(nsIDOMNode* aDomNode, nsIWeakReference* aShell):
+  nsHTMLImageAccessible(aDomNode, aShell)
+  {
+  }//constructor end

The formatting of the above just looks weird, how about this in stead:

+public:
+  nsHTMLImageMapAccessible(nsIDOMNode* aDomNode, nsIWeakReference* aShell)
+    : nsHTMLImageAccessible(aDomNode, aShell)
+  {
+  }//constructor end

- In nsHTMLLinkAccessible::GetURI():

+  if (link) {
+    char *hrefValue;
+    nsresult rv;
+    rv = link->GetHrefCString(hrefValue);
+    if (NS_SUCCEEDED(rv)) {
+      return NS_NewURI(_retval, hrefValue, nsnull, nsnull);
+    }
+  }

The above code will leak the string hrefValue, use an nsXPIDLCString and
*getter_Copies().

sr=jst if you fix the above.

Comment 33

[Gilbert Fang]

Attachment: the new patch following jst's superview comments (patch_bz150603_20020702_a)

could some one help me to check it in?
r=aaronl, sr=jst

Comment 34

[Kyle Yuan]

checked in!

Comment 35

[Jessie Li]

Attachment: Test Cases for nsIAccessibleHyperLink for HTML element