Closed Bug 147168 Opened 22 years ago Closed 22 years ago

Implementation of nsIAccessibleHyperText

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: gilbert.fang, Assigned: gilbert.fang)

References

Details

Attachments

(3 files, 2 obsolete files)

#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);
};
see also bugsceri 25.
Blocks: 136315
Jessie will be in charge of this bug.
QA Contact: dsirnapalli → jessie.li
Blocks: 151107
Attached patch patch_bz147168_20020630_d (obsolete) — Splinter Review
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
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. :-/
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 :-()
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?
Attached file Simpel test case
A simple test case in java script  to demo  nsIAccessibleHyperText interface.
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
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 on attachment 89905 [details] [diff] [review]
a new patch following the comments #8(patch_bz147168_20020702_a)

r=kyle
Attachment #89905 - Flags: review+
Comment on attachment 89905 [details] [diff] [review]
a new patch following the comments #8(patch_bz147168_20020702_a)

sr=jst
Attachment #89905 - Flags: superreview+
checked in
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: