418368 - Create tests for NSIAccessibleHyperlink interface.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

Create Mochitests for the NSIAccessibleHyperlink interface.

Comment 1

[Marco Zehe (:MarcoZ)]

Attachment: Testing nsIAccessibleHyperlink, and image maps

This patch contains two mochitests: One that tests the nsIAccessibleHyperLink interface, and one that tests a set of image map links. In Image Maps, the accessible link children do not implement the nsIAccessibleHyperLink interface, thus need to be tested differently.

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 304216 [details] [diff] [review]
Testing nsIAccessibleHyperlink, and image maps

Marco, would you mind to document our interfaces as well here?

> _TEST_FILES =\
> 		test_bug368835.xul \
>+		test_NSIAccessibleHyperlink.html \

use lowercase "ns".

>+  <script type="application/javascript">
>+    function doTest()
>+    {
>+      netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');

you don't need this because we are in chrome


>+      var hyperlinkElement = document.getElementById("Hyperlink");
>+      var accService = Components.classes["@mozilla.org/accessibleRetrieval;1"].
>+                       getService(Components.interfaces.nsIAccessibleRetrieval);
>+
>+      var HyperlinkAcc = accService.getAccessibleFor(hyperlinkElement).
>+        QueryInterface(Components.interfaces.nsIAccessibleHyperLink);
>+
>+      is(HyperlinkAcc.anchors, 1, "Number of anchors wrong!");
>+is(HyperlinkAcc.getURI(0).spec, "http://www.mozilla.org/", "URI wrong!");
>+      is(HyperlinkAcc.getObject(0).name, "Mozilla Foundation",
>+         "Wrong object name!");

Possibly it would be better to compare this with nsIAccessible.children or something like this?

>+      is(HyperlinkAcc.isValid(), true, "Not valid!");

It would be nice to test not valid link here as well.

>+      is(HyperlinkAcc.isSelected(), false, "Indicates selection!");

here too.
;
>+      is(accService.getStringRole(mapAcc.finalRole), "image map", "wrong role!");

it's better to comare with nsIAccessibleRole constants I guess.

>+  <img id="Map" width="668" height="53" border="0" usemap="#m" src="imagemap.gif" alt="Yahoo">

what is "imagemap.fig"?

btw, should we test area on nsIAccessibleHyperLink?

Comment 3

[alexander :surkov (:asurkov)]

Should we test ARIA links here?

Comment 4

[Marco Zehe (:MarcoZ)]

(In reply to comment #3)
> Should we test ARIA links here?

I was going with the--no longer operational--test cases here: http://www.mozilla.org/projects/ui/accessibility/unix/testcase/nsIAccExt/TestCases_for_HTML_Elements.html. There, the second Hyperlink test page deals with areas. Back then, the nsIAccessibleHyperLink was still implemented for an area's children. I only realized when i developed the Mochitest that the nsIAccessibleHyperLink interface no longer is implemented for area children. But I'd still like to keep it here to show the difference in link handling.

Comment 5

[alexander :surkov (:asurkov)]

Marco, is it bug about area and nsIAccessibleHyperlink, right?

But I meant ARIA not area.

Comment 6

[Marco Zehe (:MarcoZ)]

Right, makes sense to put an ARIA testcase in as well. Will get on it and submit a new patch soon.

Comment 7

[Marco Zehe (:MarcoZ)]

(In reply to comment #2)
> Marco, would you mind to document our interfaces as well here?

Not at all! :-)

> >+      is(HyperlinkAcc.getObject(0).name, "Mozilla Foundation",
> >+         "Wrong object name!");
> Possibly it would be better to compare this with nsIAccessible.children or
> something like this?

Well the method getObject was specified int hat interface, and getObject(0) is the text leaf that makes up the name for the link, so I thought it was a good idea to use it.

> 
> >+      is(HyperlinkAcc.isValid(), true, "Not valid!");
> It would be nice to test not valid link here as well.

Will do with the ARIA link. isValid only applies to ARIA links currently, and only if an author specifically says the link is known to be invalid. Thinking about it, I should probably remove this test from the normal link because it has no meaning there. I could even put "bla bla" in the href attribute, and it would still be valid. :-)

> >+      is(accService.getStringRole(mapAcc.finalRole), "image map", "wrong role!");
> 
> it's better to comare with nsIAccessibleRole constants I guess.

Do you know of a way to get the nsIAccessibleRole interface from JS? I haven't found a way yet, and DomInspector also uses the above method to display the roles.

> >+  <img id="Map" width="668" height="53" border="0" usemap="#m" src="imagemap.gif" alt="Yahoo">
> 
> what is "imagemap.fig"?

Oh yes, I forgot to download that image from the original example page and put it in here.

Comment 8

[Marco Zehe (:MarcoZ)]

(In reply to comment #7)
> (In reply to comment #2)
> > >+      is(accService.getStringRole(mapAcc.finalRole), "image map", "wrong role!");
> > it's better to comare with nsIAccessibleRole constants I guess.
> Do you know of a way to get the nsIAccessibleRole interface from JS? I haven't
> found a way yet, and DomInspector also uses the above method to display the
> roles.

Nevermind, Gijs helped me with that one. With those constant roles, I can simply use Components.interfaces.nsIAccessibleRole.ROLE_*.

Comment 9

[Marco Zehe (:MarcoZ)]

Attachment: Patch v2

1. Added two ARIA links, one ordinary and one specifically set to be invalid.
2. Added documentation to IDL.
3. Fixed typo in comment for aria-invalid attribute in a comment.
4. Decided to handle image maps in a separate bug after all. Therefore that file is no longer included.

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 304948 [details] [diff] [review]
Patch v2

>+/**
>+ * A cross-platform interface that supports Hyperlink-specific properties and
>+ * methods.
>+   * Note that links embedded within an image map do not implement this interface.

nit: wrong indentation, please file bug for this and mark the comment as XXX.

>+ */
> [scriptable, uuid(a492c7d6-1dd1-11b2-9bc0-80614884799a)]
> interface nsIAccessibleHyperLink : nsISupports
> {
>+  /**
>+   * The numbber of anchors within this Hyperlink. Should normally be 1.

nit: I do not get about 1, it's better to give examples if you wish.


>   readonly attribute long anchors;
>+  /**
>+   * returns the startIndex of the link
>+   */

nit: Uppercase please and dot in the end.

>   readonly attribute long startIndex;
>+  /**

nit: add empty line between attributes.

>+   * Returns the end index of the link.
>+   */

Could you document it more carefully?

>   readonly attribute long endIndex;
>-
>+  /**
>+   * Returns the URI at the given index.
>+   * Not supported with ARIA hyperlinks, since clicks are processed via JavaScript.

What does "not supported" mean?

>+   *
>+   * @param i  The 0-based index of the URI to be returned.
>+   */
>   nsIURI getURI (in long i);

nit: it's not very  good name
nit: add @return

>+  /**
>+   * Returns the object at the given index.

Would be nice to have here something more detailed.

>+  /**
>+   * Determines whether the link is valid.

what does valid means?

>+<head>
>+  <title>NSIHyperLinkAccessible chrome tests</title>
>+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css" />
>+
>+  <script type="application/javascript" src="chrome://mochikit/content/MochiKit/packed.js"></script>
>+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
>+
>+  <script type="application/javascript">
>+    function doTest()
>+    {
>+      var normalHyperlinkElement = document.getElementById("NormalHyperlink");
>+      var accService = Components.classes["@mozilla.org/accessibleRetrieval;1"].
>+                       getService(Components.interfaces.nsIAccessibleRetrieval);
>+
>+      var normalHyperlinkAcc = accService.getAccessibleFor(normalHyperlinkElement).
>+        QueryInterface(Components.interfaces.nsIAccessibleHyperLink);
>+
>+      is(normalHyperlinkAcc.finalRole, Components.interfaces.nsIAccessibleRole.ROLE_LINK, "Not the right role!");

nit: please check 80 linke restriction.

>+      is(normalHyperlinkAcc.anchors, 1, "Number of anchors wrong!");
>+      is(normalHyperlinkAcc.getURI(0).spec, "http://www.mozilla.org/", "URI wrong!");
>+      is(normalHyperlinkAcc.getObject(0).name, "Mozilla Foundation",
>+         "Wrong object name!");
>+      is(normalHyperlinkAcc.isSelected(), false, "Indicates selection!");
>+      normalHyperlinkElement.focus();
>+      is(normalHyperlinkAcc.isSelected(), true, "Indicates no selection!");
>+
>+      var ariaHyperlinkElement = document.getElementById("AriaHyperlink");
>+      var ariaHyperlinkAcc = accService.getAccessibleFor(ariaHyperlinkElement).
>+        QueryInterface(Components.interfaces.nsIAccessibleHyperLink);
>+
>+      is(ariaHyperlinkAcc.finalRole, Components.interfaces.nsIAccessibleRole.ROLE_LINK, "ARIA role not right!");
>+      is(ariaHyperlinkAcc.anchors, 1, "Number of anchors wrong!");
>+      is(ariaHyperlinkAcc.getObject(0).name, "Mozilla Foundation Home",
>+         "Wrong object name!");
>+      is(ariaHyperlinkAcc.isValid(), true, "Should be valid!");
>+      is(ariaHyperlinkAcc.isSelected(), false, "Indicates selection!");
>+      ariaHyperlinkElement.focus();
>+      is(ariaHyperlinkAcc.isSelected(), true, "Indicates no selection!");
>+
>+      var invalidAriaHyperlinkElement =
>+          document.getElementById("InvalidAriaHyperlink");
>+      var invalidAriaHyperlinkAcc =
>+          accService.getAccessibleFor(invalidAriaHyperlinkElement).
>+          QueryInterface(Components.interfaces.nsIAccessibleHyperLink);
>+
>+      is(invalidAriaHyperlinkAcc.isValid(), false, "Should not be valid!");
>+      is(invalidAriaHyperlinkAcc.isSelected(), false, "Indicates selection!");
>+      invalidAriaHyperlinkElement.focus();
>+      is(invalidAriaHyperlinkAcc.isSelected(), false,
>+         "Should not be focusable!");
>+

possibly would be nice to have function helper
function checkLink(aAcc, aAnchors, aName, aIsValid);

Comment 11

[Marco Zehe (:MarcoZ)]

Attachment: non-working WIP

Surkov, is this roughly what you had in mind when you asked for helpeer functions? I get the feeling that this heap of local variables makes the code even worse to read.

Comment 12

[Marco Zehe (:MarcoZ)]

Attachment: Patch 3

1. Converted isValid and isSelected to attributes, updated IDL accordingly with new UUID.
2. Cleaned up HTML testcase.
3. Added testcase for xul:label that is converted into a hyperlink.

Question: in accessible/src/msaa/cAccessibleHyperLink.cpp, I am calling the getter, not the attribute, because I didn't find a way to both get the actual validity state *and* the information whether the call was successful at all. Is this OK, or should it be done differently? If differently, how?

Comment 13

[Marco Zehe (:MarcoZ)]

Attachment: Patch v4

1. Forgot to add test_nsIAccessibleHyperLink.xul to Makefile.in.
2. Also modified the atk/nsMaiHyperLink.cpp file, and same question as for msaa/cAccessibleHyperLink.cpp: is there a less hacky way to achieve this?

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 305199 [details] [diff] [review]
Patch v4


>+ * A cross-platform interface that supports Hyperlink-specific properties and

nit: why hyperllink has upper case first letter?
 /**
>+   * The numbber of anchors within this Hyperlink. Should normally be 1.

could you document how it can be not 1 and how to deal with this? Use @see for that.

>+   * The first anchor is normally the href attribute of the html:a tag.

would be nice to say "for example"

>+  /**
>+   * Returns the startIndex of the link within the parent text run.

iirc we use hypertext offset instead of index, could you check this?

>+   */
>+  readonly attribute long startIndex;
> 
>-  nsIAccessible getObject (in long i);
>+  /**
>+   * Returns the end index of the link. The link itself is represented by one
>+   * embedded character within the text run, so the endIndex should be
>+   * startIndex+1.

nit: add spaces between operands.

>+   * Determines whether the link is valid (e. g. points to a valid URL).
>+   * Note: Currently only used with ARIA links, and the author has to

nit Note: should be @note?

>+  /**
>+   * Returns the object at the given index.

what is the object?  better to say something like "returns an object that represents the link anchor"

 The index is normally 0 for the
>+   * child containing the text leaf with the link's on-screen text.

why? nsAccessible::GetObject() returns itself.

>-// nsIAccessibleHyperLink::IsValid()
>-NS_IMETHODIMP nsAccessible::IsValid(PRBool *aIsValid)
>+// nsIAccessibleHyperLink::valid attribute

nit: use IDL syntax instead

>+NS_IMETHODIMP nsAccessible::GetValid(PRBool *aValid)

write NS_IMETHODIMP on own line

> {
>   PRUint32 state = State(this);
>-  *aIsValid = (0 == (state & nsIAccessibleStates::STATE_INVALID));
>+  *aValid = (0 == (state & nsIAccessibleStates::STATE_INVALID));

while you here add NS_ENSURE_ARG_POINTER for aValid argument.

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 305199 [details] [diff] [review]
Patch v4


>===================================================================
>RCS file: accessible/tests/mochitest/test_nsIAccessibleHyperlink.html
>diff -N accessible/tests/mochitest/test_nsIAccessibleHyperlink.html
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ accessible/tests/mochitest/test_nsIAccessibleHyperlink.html	23 Feb 2008 15:26:57 -0000
>@@ -0,0 +1,92 @@
>+<!DOCTYPE html>
>+<html>
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=418368
>+-->
>+<head>
>+  <title>NSIHyperLinkAccessible chrome tests</title>

use ns

why XUL test is more poor?

Comment 16

[alexander :surkov (:asurkov)]

Marco, please include test for bug 419786.

In my tests I used:

<map name="atoz_map">
    <area href="http://www.bbc.co.uk/radio4/atoz/index.shtml#b"
          coords="17,0,30,14"
          alt="b"
          shape="rect"></aria>
    <area href="http://www.bbc.co.uk/radio4/atoz/index.shtml#a"
          coords="0,0,13,14"
          alt="a"
          shape="rect"></aria>
  </map>
  <img width="447" id="imgmap"
       height="15"
       usemap="#atoz_map"
       src="http://www.bbc.co.uk/radio4/images/letters.gif"></img>

var imageMapHyperlinkElement =
          document.getElementById("imgmap");
      var imageMapHyperlinkAcc = accService.getAccessibleFor(imageMapHyperlinkElement).
        QueryInterface(Components.interfaces.nsIAccessibleHyperLink);
      testThis(imageMapHyperlinkAcc,
               Components.interfaces.nsIAccessibleRole.ROLE_IMAGE_MAP, 2, 
               "b", true, 71, 72);

      is(imageMapHyperlinkAcc.getURI(0).spec, "http://www.bbc.co.uk/radio4/atoz/index.shtml#b", "URI wrong!");
      is(imageMapHyperlinkAcc.getURI(1).spec, "http://www.bbc.co.uk/radio4/atoz/index.shtml#a", "URI wrong!");

      var area1 = imageMapHyperlinkAcc.firstChild;
      ok(area1, "no children");
      try {
        var area1HL = area1.QueryInterface(Components.interfaces.nsIAccessibleHyperLink);
      } catch (e) {
        ok(area1HL, "no interface");
      }

Comment 17

[alexander :surkov (:asurkov)]

Also, I would like to see tests like

<a href=""><img src=""/></a> and probably some another not simple tests as well.

Another thing: QueryInterface may fail. So we should handle this. 

Also please include ID of tested element into error report, it helps to understand where failed.

Please line up the following:

<span id="InvalidAriaHyperlink" role="link" aria-invalid="true"
  onclick="window.open('http:/www.mozilla.org/');">Invalid link</span>

like 

<span id="InvalidAriaHyperlink" role="link" aria-invalid="true"
             onclick="window.open('http:/www.mozilla.org/');">Invalid link</span>

Comment 18

[alexander :surkov (:asurkov)]

Please rename 

readonly attribute long anchors;

to

readonly attribute long anchorsCount;

it's more similar to exisiting names.

Possibly we should rename getObject() to getAnchor() - it's more similar with IA2 and it seems more descriptive, are you agree?

Comment 19

[alexander :surkov (:asurkov)]

Comment on attachment 305199 [details] [diff] [review]
Patch v4

waiting for new patch ;)

Comment 20

[Marco Zehe (:MarcoZ)]

(In reply to comment #14)
> >-// nsIAccessibleHyperLink::IsValid()
> >-NS_IMETHODIMP nsAccessible::IsValid(PRBool *aIsValid)
> >+// nsIAccessibleHyperLink::valid attribute
> 
> nit: use IDL syntax instead

Can you help me out, what does IDL syntax look like in this case?

Comment 21

[Marco Zehe (:MarcoZ)]

(In reply to comment #15)
> (From update of attachment 305199 [details] [diff] [review])
> 
> >===================================================================
> >RCS file: accessible/tests/mochitest/test_nsIAccessibleHyperlink.html
> >diff -N accessible/tests/mochitest/test_nsIAccessibleHyperlink.html
> >--- /dev/null	1 Jan 1970 00:00:00 -0000
> >+++ accessible/tests/mochitest/test_nsIAccessibleHyperlink.html	23 Feb 2008 15:26:57 -0000
> >@@ -0,0 +1,92 @@
> >+<!DOCTYPE html>
> >+<html>
> why XUL test is more poor?

Because much of the nsIAccessibleHyperLink interface isn't implemented for XUL label/links. Should I file a bug on this?

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> (In reply to comment #14)
> > >-// nsIAccessibleHyperLink::IsValid()
> > >-NS_IMETHODIMP nsAccessible::IsValid(PRBool *aIsValid)
> > >+// nsIAccessibleHyperLink::valid attribute
> > 
> > nit: use IDL syntax instead
> 
> Can you help me out, what does IDL syntax look like in this case?
> 

I meant just copy the line from IDL, it should be like
readonly attribute boolean valid or something similar

(In reply to comment #21)
> > why XUL test is more poor?
> 
> Because much of the nsIAccessibleHyperLink interface isn't implemented for XUL
> label/links. Should I file a bug on this?
> 

Of course!

I think it makes sense to add ARIA examples, XUL examples may be handled a bit different than HTML ones.

Comment 23

[Marco Zehe (:MarcoZ)]

(In reply to comment #22)
> (In reply to comment #21)
> > > why XUL test is more poor?
> > 
> > Because much of the nsIAccessibleHyperLink interface isn't implemented for XUL
> > label/links. Should I file a bug on this?
> Of course!

OK, you've just gotten yourself some work. :-) Bug 421066 created.

> I think it makes sense to add ARIA examples, XUL examples may be handled a bit
> different than HTML ones.

We already have an ARIA example in there.

(In reply to comment #18)
> Possibly we should rename getObject() to getAnchor() - it's more similar with
> IA2 and it seems more descriptive, are you agree?

I agree that this name is more what the method actually does. Although ATK calls it getObject, I've decided to rename it.

Comment 24

[Marco Zehe (:MarcoZ)]

Attachment: Patch v5

1. Addressing comments.
2. Adding more tests.
3. Folded your image map tests in.
4. Removed XUL test to deal with in bug 421066 separately.

Comment 25

[alexander :surkov (:asurkov)]

Comment on attachment 308402 [details] [diff] [review]
Patch v5


>+      is(aAcc.finalRole, aRole, "Wrong role for ID "+aID+"!");

nit: use spaces between operand and operator.

>+      var ariaHyperlinkElement = document.getElementById("AriaHyperlink");
>+      try {
>+        var ariaHyperlinkAcc = accService.getAccessibleFor(ariaHyperlinkElement).
>+          QueryInterface(Components.interfaces.nsIAccessibleHyperLink);
>+      } catch(e) {
>+        ok(ariaHyperlinkAcc, "no interface for ARIA Hyperlink!");

is ariaHyperlinkAcc visible if it is defined in ariaHyperlinkAcc? I would suggest to use
try{
  ok(true, "");
} catch(e) {
  ok(false, "");
}

>+      }
>+      testThis(ariaHyperlinkElement, ariaHyperlinkAcc,

please define ariaHyperlinkAcc outside of try even if it is visible here because it's more readable I guess

you do not test getAnchor(1) for imagemap.

please put comments before each test.

>+/**
>+ * A cross-platform interface that supports hyperlink-specific properties and
>+ * methods.

nit: Possibly some examples would be good.

>+ */
>+[scriptable, uuid(fe1dd8c0-d50a-4634-b51d-2b20bfb1e231)]
> interface nsIAccessibleHyperLink : nsISupports
> {


>-  readonly attribute long anchors;
>+  /**
>+   * The numbber of anchors within this Hyperlink. Is always 1.

What does "Is always 1" mean? If it is always 1 why do we need the method ;)? But imagemap returns more than 1 iirc.


>+   * This anchor is, for example, the href attribute of the html:a tag.
>+   */
>+  readonly attribute long anchorsCount;
>+
>+  /**
>+   * Returns the offset of the link within the parent hypertext run.

nit: I'm a bit confused by the word "run" but if it sounds ok for you then it's ok with me. I would say returns offset inside parent hypertext or something like this.

>+   */
>   readonly attribute long startIndex;
>-  readonly attribute long endIndex;
>-
>-  nsIURI getURI (in long i);
> 
>-  nsIAccessible getObject (in long i);
>+  /**
>+   * Returns the end index of the link.

:nit mention please parent hypertext accessible.

 The link itself is represented by one
>+   * embedded character within the text run, so the endIndex should be
>+   * startIndex + 1.

nit: shold be @note?

>+   * Determines whether the link is valid (e. g. points to a valid URL).
>+   * @note  Currently only used with ARIA links, and the author has to
>+   * specify that the link is invalid via the aria-invalid="true" attribute.
>+   * In all other cases, TRUE is returned.

nit: mark this as XXX to get attention to this.

>+   * Determines whether the element currently has the focus, e. g. after
>+   * returning from the destination page.

nit: put empty line here with asterix

>+   * @note  ARIA links can only be focused if they have the tabindex
>+   * attribute set.
>+   */

nit: would be nice to compare with state focused.


Please move anchorsCount here to keep it together with getURI and getAnchor

>+   * Returns the URI at the given index.
>+   * Note that ARIA hyperlinks do not have an URI to point to, since clicks are

Note -> @note

>+   * processed via JavaScript. Therefore this property does not work on ARIA
>+   * links.
>+   *
>+   * @param index  The 0-based index of the URI to be returned.
>+   *
>+   * @return the nsIURI object containing the specifications for the URI.
>+   */
>+  nsIURI getURI (in long index);
>+
>+  /**
>+   * Returns a reference to itself and increases the ref count. The index is always 0.

It's not true for imagemap. You don't need to say about ref count. It is assumed.

> 
>Index: accessible/src/atk/nsMaiHyperlink.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/atk/nsMaiHyperlink.cpp,v
>retrieving revision 1.8
>diff -p -u -5 -r1.8 nsMaiHyperlink.cpp
>--- accessible/src/atk/nsMaiHyperlink.cpp	8 Jul 2007 07:08:04 -0000	1.8
>+++ accessible/src/atk/nsMaiHyperlink.cpp	10 Mar 2008 13:50:44 -0000
>@@ -241,11 +241,11 @@ getObjectCB(AtkHyperlink *aLink, gint aL
> {
>     nsIAccessibleHyperLink *accHyperlink = get_accessible_hyperlink(aLink);
>     NS_ENSURE_TRUE(accHyperlink, nsnull);
> 
>     nsCOMPtr<nsIAccessible> accObj;
>-    accHyperlink->GetObject(aLinkIndex, getter_AddRefs(accObj));
>+    accHyperlink->GetAnchor(aLinkIndex, getter_AddRefs(accObj));
>     NS_ENSURE_TRUE(accObj, nsnull);
> 
>     AtkObject *atkObj = nsAccessibleWrap::GetAtkObject(accObj);
>     //no need to add ref it, because it is "get" not "ref"
>     return atkObj;
>@@ -280,22 +280,22 @@ isValidCB(AtkHyperlink *aLink)
> {
>     nsIAccessibleHyperLink *accHyperlink = get_accessible_hyperlink(aLink);
>     NS_ENSURE_TRUE(accHyperlink, FALSE);
> 
>     PRBool isValid = PR_FALSE;
>-    nsresult rv = accHyperlink->IsValid(&isValid);
>+    nsresult rv = accHyperlink->GetValid(&isValid);
>     return (NS_FAILED(rv)) ? FALSE : static_cast<gboolean>(isValid);
> }
> 
> gint
> getAnchorCountCB(AtkHyperlink *aLink)
> {
>     nsIAccessibleHyperLink *accHyperlink = get_accessible_hyperlink(aLink);
>     NS_ENSURE_TRUE(accHyperlink, -1);
> 
>     PRInt32 count = -1;
>-    nsresult rv = accHyperlink->GetAnchors(&count);
>+    nsresult rv = accHyperlink->GetAnchorsCount(&count);
>     return (NS_FAILED(rv)) ? -1 : static_cast<gint>(count);
> }
> 
> // Check if aHyperlink is a valid MaiHyperlink, and return the
> // nsIAccessibleHyperLink related.
>Index: accessible/src/base/nsAccessible.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/base/nsAccessible.cpp,v
>retrieving revision 1.360
>diff -p -u -5 -r1.360 nsAccessible.cpp
>--- accessible/src/base/nsAccessible.cpp	6 Mar 2008 03:45:43 -0000	1.360
>+++ accessible/src/base/nsAccessible.cpp	10 Mar 2008 13:50:46 -0000
>@@ -3099,63 +3099,81 @@ NS_IMETHODIMP nsAccessible::SelectAllSel
> // nsIAccessibleHyperLink
> // Because of new-atk design, any embedded object in text can implement
> // nsIAccessibleHyperLink, which helps determine where it is located
> // within containing text
> 
>-NS_IMETHODIMP nsAccessible::GetAnchors(PRInt32 *aAnchors)
>+// readonly attribute long nsIAccessibleHyperLink::anchorsCount
>+NS_IMETHODIMP
>+nsAccessible::GetAnchorsCount(PRInt32 *aAnchorsCount)
> {
>-  *aAnchors = 1;
>+  NS_ENSURE_ARG_POINTER(aAnchorsCount);
>+  *aAnchorsCount = 1;
>   return NS_OK;
> }
> 
>-NS_IMETHODIMP nsAccessible::GetStartIndex(PRInt32 *aStartIndex)
>+// readonly attribute long nsIAccessibleHyperLink::startIndex
>+NS_IMETHODIMP
>+nsAccessible::GetStartIndex(PRInt32 *aStartIndex)
> {
>+  NS_ENSURE_ARG_POINTER(aStartIndex);
>   *aStartIndex = 0;
>   PRInt32 endIndex;
>   return GetLinkOffset(aStartIndex, &endIndex);
> }
> 
>-NS_IMETHODIMP nsAccessible::GetEndIndex(PRInt32 *aEndIndex)
>+// readonly attribute long nsIAccessibleHyperLink::endIndex
>+NS_IMETHODIMP
>+nsAccessible::GetEndIndex(PRInt32 *aEndIndex)
> {
>+  NS_ENSURE_ARG_POINTER(aEndIndex);
>   *aEndIndex = 0;
>   PRInt32 startIndex;
>   return GetLinkOffset(&startIndex, aEndIndex);
> }
> 
>-NS_IMETHODIMP nsAccessible::GetURI(PRInt32 i, nsIURI **aURI)
>+NS_IMETHODIMP
>+nsAccessible::GetURI(PRInt32 i, nsIURI **aURI)
> {
>+  NS_ENSURE_ARG_POINTER(aURI);
>   *aURI = nsnull;
>   return NS_ERROR_FAILURE;
> }
> 
>-NS_IMETHODIMP nsAccessible::GetObject(PRInt32 aIndex,
>+NS_IMETHODIMP
>+nsAccessible::GetAnchor(PRInt32 aIndex,
>                                       nsIAccessible **aAccessible)
> {
>+  NS_ENSURE_ARG_POINTER(aAccessible);
>   if (aIndex != 0) {
>     *aAccessible = nsnull;
>     return NS_ERROR_FAILURE;
>   }
>   *aAccessible = this;
>   NS_ADDREF_THIS();
>   return NS_OK;
> }
> 
>-// nsIAccessibleHyperLink::IsValid()
>-NS_IMETHODIMP nsAccessible::IsValid(PRBool *aIsValid)
>+// readonly attribute boolean nsIAccessibleHyperLink::valid
>+NS_IMETHODIMP
>+nsAccessible::GetValid(PRBool *aValid)
> {
>+  NS_ENSURE_ARG_POINTER(aValid);
>   PRUint32 state = State(this);
>-  *aIsValid = (0 == (state & nsIAccessibleStates::STATE_INVALID));
>+  *aValid = (0 == (state & nsIAccessibleStates::STATE_INVALID));
>   // XXX In order to implement this we would need to follow every link
>   // Perhaps we can get information about invalid links from the cache
>-  // In the mean time authors can use role="link" aria_invalid="true"
>+  // In the mean time authors can use role="link" aria-invalid="true"
>   // to force it for links they internally know to be invalid
>   return NS_OK;
> }
> 
>-NS_IMETHODIMP nsAccessible::IsSelected(PRBool *aIsSelected)
>+// readonly attribute boolean nsIAccessibleHyperLink::selected
>+NS_IMETHODIMP
>+nsAccessible::GetSelected(PRBool *aSelected)
> {
>-  *aIsSelected = (gLastFocusedNode == mDOMNode);
>+  NS_ENSURE_ARG_POINTER(aSelected);
>+  *aSelected = (gLastFocusedNode == mDOMNode);
>   return NS_OK;
> }
> 
> nsresult nsAccessible::GetLinkOffset(PRInt32* aStartOffset, PRInt32* aEndOffset)
> {
>Index: accessible/src/html/nsHTMLImageAccessible.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/html/nsHTMLImageAccessible.cpp,v
>retrieving revision 1.47
>diff -p -u -5 -r1.47 nsHTMLImageAccessible.cpp
>--- accessible/src/html/nsHTMLImageAccessible.cpp	6 Mar 2008 03:33:33 -0000	1.47
>+++ accessible/src/html/nsHTMLImageAccessible.cpp	10 Mar 2008 13:50:46 -0000
>@@ -224,16 +224,16 @@ NS_IMETHODIMP nsHTMLImageAccessible::DoA
> }
> 
> ////////////////////////////////////////////////////////////////////////////////
> // nsIAccessibleHyperLink
> NS_IMETHODIMP
>-nsHTMLImageAccessible::GetAnchors(PRInt32 *aAnchors)
>+nsHTMLImageAccessible::GetAnchorsCount(PRInt32 *aAnchors)
> {
>   NS_ENSURE_ARG_POINTER(aAnchors);
> 
>   if (!mMapElement)
>-    return nsLinkableAccessible::GetAnchors(aAnchors);
>+    return nsLinkableAccessible::GetAnchorsCount(aAnchors);
> 
>   return GetChildCount(aAnchors);
> }
> 
> NS_IMETHODIMP
>@@ -260,17 +260,17 @@ nsHTMLImageAccessible::GetURI(PRInt32 aI
> 
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
>-nsHTMLImageAccessible::GetObject(PRInt32 aIndex, nsIAccessible **aAccessible)
>+nsHTMLImageAccessible::GetAnchor(PRInt32 aIndex, nsIAccessible **aAccessible)
> {
>   NS_ENSURE_ARG_POINTER(aAccessible);
>   *aAccessible = nsnull;
> 
>   if (!mMapElement)
>-    return nsLinkableAccessible::GetObject(aIndex, aAccessible);
>+    return nsLinkableAccessible::GetAnchor(aIndex, aAccessible);
> 
>   nsCOMPtr<nsIDOMHTMLCollection> mapAreas = GetAreaCollection();
>   if (mapAreas) {
>     nsCOMPtr<nsIAccessible> accessible;
>     accessible = GetAreaAccessible(mapAreas, aIndex);
>Index: accessible/src/html/nsHTMLImageAccessible.h
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/html/nsHTMLImageAccessible.h,v
>retrieving revision 1.25
>diff -p -u -5 -r1.25 nsHTMLImageAccessible.h
>--- accessible/src/html/nsHTMLImageAccessible.h	6 Mar 2008 03:33:33 -0000	1.25
>+++ accessible/src/html/nsHTMLImageAccessible.h	10 Mar 2008 13:50:46 -0000
>@@ -65,13 +65,13 @@ public:
>   NS_IMETHOD GetState(PRUint32 *aState, PRUint32 *aExtraState);
>   NS_IMETHOD GetRole(PRUint32 *_retval);
>   NS_IMETHOD DoAction(PRUint8 index);
> 
>   // nsIAccessibleHyperLink
>-  NS_IMETHOD GetAnchors(PRInt32 *aAnchors);
>+  NS_IMETHOD GetAnchorsCount(PRInt32 *aAnchors);
>   NS_IMETHOD GetURI(PRInt32 aIndex, nsIURI **aURI);
>-  NS_IMETHOD GetObject(PRInt32 aIndex, nsIAccessible **aAccessible);
>+  NS_IMETHOD GetAnchor(PRInt32 aIndex, nsIAccessible **aAccessible);
> 
>   // nsPIAccessNode
>   NS_IMETHOD Shutdown();
> 
>   // nsIAccessibleImage
>Index: accessible/src/msaa/CAccessibleHyperlink.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/msaa/CAccessibleHyperlink.cpp,v
>retrieving revision 1.6
>diff -p -u -5 -r1.6 CAccessibleHyperlink.cpp
>--- accessible/src/msaa/CAccessibleHyperlink.cpp	25 Feb 2008 20:40:19 -0000	1.6
>+++ accessible/src/msaa/CAccessibleHyperlink.cpp	10 Mar 2008 13:50:46 -0000
>@@ -85,11 +85,11 @@ __try {
>   nsCOMPtr<nsIAccessibleHyperLink> acc(do_QueryInterface(this));
>   if (!acc)
>     return E_FAIL;
> 
>   nsCOMPtr<nsIAccessible> anchor;
>-  acc->GetObject(aIndex, getter_AddRefs(anchor));
>+  acc->GetAnchor(aIndex, getter_AddRefs(anchor));
>   if (!anchor)
>     return E_FAIL;
> 
>   nsCOMPtr<nsIWinAccessNode> winAccessNode(do_QueryInterface(anchor));
>   if (!winAccessNode)
>@@ -194,11 +194,11 @@ __try {
>   nsCOMPtr<nsIAccessibleHyperLink> acc(do_QueryInterface(this));
>   if (!acc)
>     return E_FAIL;
> 
>   PRBool isValid = PR_FALSE;
>-  nsresult rv = acc->IsValid(&isValid);
>+  nsresult rv = acc->GetValid(&isValid);
>   *aValid = isValid;
>   if (NS_SUCCEEDED(rv))
>     return S_OK;
> } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
>

Comment 26

[alexander :surkov (:asurkov)]

Marco, would be nice to add test for 
<a><span></span></a> - accessible inside link, you have this for img but we need for generic element because they are implemented differently.

Also, please add testing for states on link accessibles, they should have STATE_LINKED and STATE_TRAVERSED if link was traversed.

Comment 27

[Marco Zehe (:MarcoZ)]

Attachment: Patch v6

Addressing hopefully all of Alexander's comments.

Comment 28

[alexander :surkov (:asurkov)]

This anchor is, for example, the href attribute of the html:a tag. - anchor is href attribute stumbles me. I thought it's element, something visible on the screen.

Comment 29

[alexander :surkov (:asurkov)]

the patch looks pretty good with me, one concern about states. Actually we need ensure for link related checks that focused/able and linked, traversed are presented. It's safer to check only interested states rather than all. I mean it's worth

function testStates(aElement, aAcc, aState, aExtraState)
{
is(state & aState, aState, "")
}

testStates(..., ..., state_focusable | state_linked, ...)

Comment 30

[alexander :surkov (:asurkov)]

Comment on attachment 310966 [details] [diff] [review]
Patch v6

r=me with comments above fixed

Comment 31

[Marco Zehe (:MarcoZ)]

(In reply to comment #29)
> function testStates(aElement, aAcc, aState, aExtraState)
> {
> is(state & aState, aState, "")
> }
> 
> testStates(..., ..., state_focusable | state_linked, ...)

This way all of them fail. Instead I used:

> function testStates(aElement, aAcc, aState, aExtraState)
> {
> is(state | aState, aState, "")
> }
> 
> testStates(..., ..., state_focusable & state_linked, ...)

This gives us all we are interested in.

I have another patch coming up that addresses this, but also includes one more test to make sure a named anchor is not exposed with the state_linked on it. This is a mochitest for bug 423603.

Comment 32

[Marco Zehe (:MarcoZ)]

Attachment: Patch v7

re-requesting review since I added one more test and had to use a slightly different approach for the states than suggested. Also, I have never seen state_traversed on any of the links anywhere. What does it mean, and what should be done to test for it?

Comment 33

[alexander :surkov (:asurkov)]

(In reply to comment #32)
> Also, I have never seen
> state_traversed on any of the links anywhere. What does it mean, and what
> should be done to test for it?
> 

I think when link has been activated then it should have state traversed.

Comment 34

[alexander :surkov (:asurkov)]

(In reply to comment #31)
>. Instead I used:
> 
> > function testStates(aElement, aAcc, aState, aExtraState)
> > {
> > is(state | aState, aState, "")
> > }
> > 
> > testStates(..., ..., state_focusable & state_linked, ...)

I don't get how it works, state_focusable & state_linked must be 0 always. Isn't it?

Comment 35

[alexander :surkov (:asurkov)]

> (state_selectable & !state_linked)

it should be 0 as well, right? because ! - boolean operator which returns false in our case

Comment 36

[Marco Zehe (:MarcoZ)]

I just found out that all these states are platform specific, meaning they've gone through the wrapper classes when they get to my Mochitest. I'll have to do separate sets of constants for Win32, Linux i686 etc.

Comment 37

[alexander :surkov (:asurkov)]

(In reply to comment #36)
> I just found out that all these states are platform specific, meaning they've
> gone through the wrapper classes when they get to my Mochitest. I'll have to do
> separate sets of constants for Win32, Linux i686 etc.
> 

It's interesting how you will do.

Basically we try not to have gecko states difference between platforms (though we have some difference for focus related states iirc) therefore it would be nice if you will provide some examples.

Comment 38

[Marco Zehe (:MarcoZ)]

(In reply to comment #37)
> Basically we try not to have gecko states difference between platforms (though

Yeah, skip that about platform specific states. I was bitten badly by bug 425125 on this one, which affects all kinds of states, not just those of ARIA pages or elements. The question still is why IA2 tells me that all my links are multi_line and modal/required even though the extra states don't indicate it if I bitwise-and them. Does IA2 have a habit of inheriting states from parent elements?

Comment 39

[Marco Zehe (:MarcoZ)]

Attachment: Patch v8

Fix bitwise comparisons.

Important! Some of these tests will only work if bug 425125 is fixed. This is sort of a proof of concept that these tests really catch regressions. :-)

Comment 40

[Marco Zehe (:MarcoZ)]

I have not managed to find a reliable way to test for whether a link has been visited or not. Everytime I try to use window.open with some sort of features to either open in the background or the like, either the opened page steals focus from the Mochitest page, resulting in all other tests to fail, or it opens, the Mochitests continue running, but the visited/traversed state isn't set in time for the according test to pass.

I am out of ideas on this one and would appreciate some help!

Comment 41

[alexander :surkov (:asurkov)]

Marco, please file bug for the traversed state checking. Let's deal with it later.

Comment 42

[alexander :surkov (:asurkov)]

Comment on attachment 312229 [details] [diff] [review]
Patch v8

r=me

please fix nits:
aElement - the string, would be nice to call aElementId or Id because it add a mesh
testNotStates() - possibly testAbsentStates or something similar

Also you may want to have one method for this testStates(, , aMustBePresented) or similar.

Comment 43

[Marco Zehe (:MarcoZ)]

Attachment: Patch for checkin

Addresses Surkov's comments:
1. The check for absent states has been merged into the testStates function. If anything other than 0 is passed in for the absent state check, a check will be performed to make sure the states requested are NOT within the states. This saves 2 a11y calls to getFinalState and gets rid of a function.

2. The passed in ID is now called aID in all three test functions.

Comment 44

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 312455 [details] [diff] [review]
Patch for checkin

a=beltzner

Comment 45

[alexander :surkov (:asurkov)]

Marco, could you update your patch to trunk?

Comment 46

[Marco Zehe (:MarcoZ)]

Attachment: Updated to trunk

Same patch, just updated to trunk with conflicts resolved after other checkins.

Comment 47

[:Gijs (he/him)]

Checking in mozilla/accessible/public/nsIAccessibleHyperLink.idl;
new revision: 1.6; previous revision: 1.5
Checking in mozilla/accessible/src/atk/nsMaiHyperlink.cpp;
new revision: 1.10; previous revision: 1.9
Checking in mozilla/accessible/src/base/nsAccessible.cpp;
new revision: 1.370; previous revision: 1.369
Checking in mozilla/accessible/src/html/nsHTMLImageAccessible.cpp;
new revision: 1.49; previous revision: 1.48
Checking in mozilla/accessible/src/html/nsHTMLImageAccessible.h;
new revision: 1.26; previous revision: 1.25
Checking in mozilla/accessible/src/msaa/CAccessibleHyperlink.cpp;
new revision: 1.9; previous revision: 1.8
Checking in mozilla/accessible/tests/mochitest/Makefile.in;
new revision: 1.10; previous revision: 1.9

Comment 48

[:Gijs (he/him)]

Oops, Marco just reminded me I forgot one:

Checking in mozilla/accessible/tests/mochitest/test_nsIAccessibleHyperLink.html;
/cvsroot/mozilla/accessible/tests/mochitest/test_nsIAccessibleHyperLink.html,v  <--  test_nsIAccessibleHyperLink.html
initial revision: 1.1
done

Comment 49

[Eric Shepherd [:sheppy]]

Docs updated.