Closed
Bug 150603
Opened 22 years ago
Closed 22 years ago
Implementation of nsIAccessibleHyperLink
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
People
(Reporter: gilbert.fang, Assigned: gilbert.fang)
References
Details
Attachments
(5 files, 6 obsolete files)
18.68 KB,
application/x-compressed
|
Details | |
6.56 KB,
text/plain
|
Details | |
9.38 KB,
patch
|
gilbert.fang
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
9.19 KB,
patch
|
Details | Diff | Splinter Review | |
10.30 KB,
application/octet-stream
|
Details |
see bugsceri 24.
Jessie will be in charge of this bug.
QA Contact: dsirnapalli → jessie.li
Comment 2•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 6•22 years ago
|
||
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 ?
Assignee | ||
Comment 7•22 years ago
|
||
Assignee | ||
Comment 8•22 years ago
|
||
Add a new class [xpcom] nsHyperLinkAceessible to implement the nsIAccseeibleHyperLink interface.
Assignee | ||
Comment 9•22 years ago
|
||
need r=?
Comment 10•22 years ago
|
||
Aaron, I already reviewed this patch in our internal bugzilla. Could you take a look at that too?
Status: NEW → ASSIGNED
Comment 11•22 years ago
|
||
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.
Assignee | ||
Comment 12•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 14•22 years ago
|
||
I am sorry, Aaronl. This patch includes the new files.
Attachment #89021 -
Attachment is obsolete: true
Comment 15•22 years ago
|
||
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•22 years ago
|
||
So how will Mozilla implement ATK's hyperlink interface? Will it do the extra work in widget/src/gtk2/mai?
Comment 17•22 years ago
|
||
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
Assignee | ||
Comment 18•22 years ago
|
||
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.
Assignee | ||
Comment 19•22 years ago
|
||
startIndex/endIndex is not implemented in this patch.
Attachment #89196 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #88852 -
Attachment is obsolete: true
Assignee | ||
Comment 20•22 years ago
|
||
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=?
Attachment #89254 -
Attachment is obsolete: true
Assignee | ||
Comment 21•22 years ago
|
||
Here is a simple test case in java script. It can show hyperlinks' attributes.
Comment 22•22 years ago
|
||
Comment 23•22 years ago
|
||
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?
Attachment #89391 -
Flags: needs-work+
Assignee | ||
Comment 24•22 years ago
|
||
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 26•22 years ago
|
||
Comment on attachment 89495 [details] [diff] [review] modified patch according to aaron comments(patch_bz150603_20020628_c) r=kyle
Attachment #89495 -
Flags: review+
Comment 27•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 29•22 years ago
|
||
okay, aaron, it is my pleasure.
Assignee | ||
Comment 30•22 years ago
|
||
this patch is almost the same with the patch 89495, in addition to modifing some old bad style code.
Attachment #89495 -
Attachment is obsolete: true
Assignee | ||
Comment 31•22 years ago
|
||
Comment on attachment 89522 [details] [diff] [review] patch addtionally polishing some old bad style code (patch_bz150603_20020628_d) from the part of aaronl
Attachment #89522 -
Flags: review+
Comment 32•22 years ago
|
||
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.
Attachment #89522 -
Flags: superreview+
Assignee | ||
Comment 33•22 years ago
|
||
could some one help me to check it in? r=aaronl, sr=jst
Comment 34•22 years ago
|
||
checked in!
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 35•22 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•