Implementation of nsIAccessibleHyperText

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Gilbert Fang, Assigned: Gilbert Fang)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

16 years ago
#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

16 years ago
see also bugsceri 25.
(Assignee)

Updated

16 years ago
Blocks: 136315

Comment 2

16 years ago
Jessie will be in charge of this bug.
QA Contact: dsirnapalli → jessie.li

Updated

16 years ago
Blocks: 151107
(Assignee)

Comment 3

16 years ago
Created attachment 89710 [details] [diff] [review]
patch_bz147168_20020630_d

extend the nsRootAccessbile to support nsIAccessibleText

Comment 4

16 years ago
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

16 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

16 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

16 years ago
Created attachment 89773 [details] [diff] [review]
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=?
Attachment #89710 - Attachment is obsolete: true

Comment 8

16 years ago
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

16 years ago
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

16 years ago
Created attachment 89883 [details]
Simpel test case 

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

Comment 11

16 years ago
Created attachment 89905 [details] [diff] [review]
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.
Attachment #89773 - Attachment is obsolete: true
(Assignee)

Comment 12

16 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

16 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 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

16 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 16

16 years ago
Created attachment 90273 [details]
Test Cases for nsIAccessibleHyperText for HTML element
You need to log in before you can comment on or make changes to this bug.