147168 - Implementation of nsIAccessibleHyperText

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Gilbert Fang]

#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);
};

Comment 1

[Gilbert Fang]

see also bugsceri 25.

Comment 2

[Kyle Yuan]

Jessie will be in charge of this bug.

Comment 3

[Gilbert Fang]

Attachment: patch_bz147168_20020630_d

extend the nsRootAccessbile to support nsIAccessibleText

Comment 4

[Kyle Yuan]

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.

Comment 5

[Gilbert Fang]

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. :-/

Comment 6

[Gilbert Fang]

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 :-()

Comment 7

[Gilbert Fang]

Attachment: new patch(patch_bz147168_20020701_b)

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=?

Comment 8

[Kyle Yuan]

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!

Comment 9

[Kyle Yuan]

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?

Comment 10

[Gilbert Fang]

Attachment: Simpel test case

A simple test case in java script  to demo  nsIAccessibleHyperText interface.

Comment 11

[Gilbert Fang]

Attachment: a new patch following the comments #8(patch_bz147168_20020702_a)

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.

Comment 12

[Gilbert Fang]

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

[Kyle Yuan]

Comment on attachment 89905 [details] [diff] [review]
a new patch following the comments #8(patch_bz147168_20020702_a)

r=kyle

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 89905 [details] [diff] [review]
a new patch following the comments #8(patch_bz147168_20020702_a)

sr=jst

Comment 15

[Kyle Yuan]

checked in

Comment 16

[Jessie Li]

Attachment: Test Cases for nsIAccessibleHyperText for HTML element