Closed
Bug 147168
Opened 22 years ago
Closed 22 years ago
Implementation of nsIAccessibleHyperText
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
People
(Reporter: gilbert.fang, Assigned: gilbert.fang)
References
Details
Attachments
(3 files, 2 obsolete files)
19.67 KB,
application/x-compressed
|
Details | |
8.67 KB,
patch
|
yuanyi21
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
10.10 KB,
application/octet-stream
|
Details |
#include "nsISupports.idl" #include "nsIAccessibleHyperLink.idl" [scriptable, uuid(8f18d922-1dd2-11b2-82ea-829b78a44413)] interface nsIAccessibleHyperText : nsISupports { readonly attribute long links; nsIAccessibleHyperLink getLink (in long index); long getLinkIndex (in long charIndex); };
Assignee | ||
Comment 1•22 years ago
|
||
see also bugsceri 25.
Jessie will be in charge of this bug.
QA Contact: dsirnapalli → jessie.li
Assignee | ||
Comment 3•22 years ago
|
||
extend the nsRootAccessbile to support nsIAccessibleText
1) this code is not compact enough: + if (hyperlink) { + return PR_TRUE; + } + return PR_FALSE; 2) +nsresult nsRootAccessible::GetLinksFromAccNode(nsIAccessible *aAccNode, I think you can return a PRInt32 for link number instead of returning NS_OK always. Same thing for GetAccNodeCharLength(). 3) + if (IsHyperLink(aAccNode)) { + links = 1; + } + if (0 == aIndex && 1 == links) { + return CallQueryInterface(aAccNode, _retval); + } You can put those two conditions together. Because IsHyperLink(aAccNode) and (1 == links) are equivalent in your case.
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•22 years ago
|
||
oh, kyle. Thank u for your pre-review. :-) unfortunately, I think I should extend the nsHTMLIFrameAccessible instead of nsRootAccessible. The latter is the really acc node for the whole content pane. :-/
Assignee | ||
Comment 6•22 years ago
|
||
Hi, kyle, as for your comments, #1, yes . It will be "return hyperlink? PR_TRUE, PR_FALSE;" #2, yes. That will make the source a little more neatly and efficiently #3, yes. It will be like if (IsHyperLink(aAccNode)) { links = 1; /* links is also used later if (0 == aIndex) return CallQueryInterface(aAccNode, _retval); } Kyle, you really have a eagle eye :-()
Assignee | ||
Comment 7•22 years ago
|
||
1 this patch is to extend the nsHTMLIFrameAccessible instead of nsRootAccessible to support the nsIAccessibleHyperText 2 this patch depends on the patch for bugzilla 150603(implementation of nsIAccessibleHyperLink) 3 this patch has been modified by kyle's comments (comments #4) Need r=?
Attachment #89710 -
Attachment is obsolete: true
Yeah, nsHTMLIFrameAccessible is the right place to implemet nsIAccessibleHyperText now. some nits: in nsHTMLIFrameAccessible::GetLinksFromAccNode() + PRInt32 rv = 0; + + //see whether beginning node is a hyperlink + if (IsHyperLink(aAccNode)) { + rv = 1; + } could be: PRInt32 rv = IsHyperLink(aAccNode) ? 1 : 0; + childLinks = GetLinksFromAccNode(child); + rv += childLinks; could be rv += GetLinksFromAccNode(child); |childLinks| can be saved. else is okay for me!
I think your fix by using +NS_IMPL_ISUPPORTS_INHERITED2(nsHTMLIFrameAccessible, nsBlockAccessible, nsIAccessibleDocument, nsIAccessibleHyperText) is more professional in XPCOM's point of view. Can it apply to nsHTMLIFrameRootAccessible too? Aaron, whether the original code (NS_INTERFACE_MAP_BEGIN, NS_IMPL_ADDREF_INHERITED, etc.) is not fully XPCOM compliance?
Assignee | ||
Comment 10•22 years ago
|
||
A simple test case in java script to demo nsIAccessibleHyperText interface.
Assignee | ||
Comment 11•22 years ago
|
||
Oh, Kyle's suggestions is good. But I am really a little bewildered whether the code is too concise to make it easy to read in the future. Anyway, I agree with kyle and make this new patch.
Attachment #89773 -
Attachment is obsolete: true
Assignee | ||
Comment 12•22 years ago
|
||
For comments #9 Of course, NS_IMPL_ISUPPORTS_INHERITEDX can also be applied to nsHTMLIFrameRootAccessible. I have read the xpcom/glue/nsISupportsImpl.h and found that NS_IMPL_ISUPPORTS_INHERITEDX actually the more convenient way to substitute the others macros 617 #define NS_IMPL_ISUPPORTS_INHERITED2(Class, Super, i1, i2) \ 618 NS_IMPL_QUERY_INTERFACE_INHERITED2(Class, Super, i1, i2) \ 619 NS_IMPL_ADDREF_INHERITED(Class, Super) \ 620 NS_IMPL_RELEASE_INHERITED(Class, Super) \ But what I want to know is whether we purify the old code in this bug or file a new bug. If this bug's patch in fact changes something unrelated with the summary, it may be diffcult for others to track the bugs. How is the mozilla's way to purify old code which is correct at all for only neatness?
Comment 13•22 years ago
|
||
Comment on attachment 89905 [details] [diff] [review] a new patch following the comments #8(patch_bz147168_20020702_a) r=kyle
Attachment #89905 -
Flags: review+
Comment 14•22 years ago
|
||
Comment on attachment 89905 [details] [diff] [review] a new patch following the comments #8(patch_bz147168_20020702_a) sr=jst
Attachment #89905 -
Flags: superreview+
Comment 15•22 years ago
|
||
checked in
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 16•22 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•