419786 - Link children associated with imagemaps do not implement the nsIAccessibleHyperLink interface

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Joanmarie Diggs]

Steps to reproduce:

1. Launch the test case in the URL above.
2. Launch Accerciser
3. Find a way to obtain the URI for each of the links in the imagemap. :-)

I can't figure it out.  I asked Surkov in #accessibility and he thinks it might be a bug, so I'm filing it.

Given a regular ol' table cell with some text and a couple of links:

* The cell implements accessible Hypertext (ht)
* ht.getNLinks() returns 2
* ht.getLink(0) returns an accessible hyperlink associated with
  the first of the two links (hl1)
* ht.getLink(1) returns an accessible hyperlink associated with
  the second of the two links (hl2)
* hl1.getURI(0) gives you the URI associated with the first link
* hl2.getURI(0) gives you the URI associated with the second link

What could be simpler? :-)

Given the imagemap in the test case (and the one found at the bottom of safeway.com) you have a table cell which contains an image which contains children of ROLE_LINK (one child per region in the map):

* The cell implements accessible Hypertext (ht)
* ht.getNLinks() returns 1
* ht.getLink(0) returns an accessible hyperlink (hl1)
* hl1.getURI(0) gives you an empty string.

The image implements accessible Hyperlink but not accessible Hypertext. In the
spirit of being thorough, I queried the image's accessible Hyperlink interface
and tried getURI(0).  Another empty string.

If this isn't a bug but mere user ignorance, please accept my apologies and point me at the magic needed to get the URIs. :-)

Thank you very much in advance for looking at this!

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Aaron Leventhal]

Comment on attachment 305963 [details] [diff] [review]
patch

Is the patch large because some methods were moved to a different part of the file?

Comment 3

[Aaron Leventhal]

Comment on attachment 305963 [details] [diff] [review]
patch

I don't think Marco is quite ready to be a reviewer for this one.

Comment 4

[Ginn Chen]

+  *aAnchors = mAccChildCount;

Is it right, if the image is just an anchor with no image maps ?

+  nsIAccessible *accessible = nsnull;
+  CallQueryInterface(accessNode, &accessible);
+
+  return accessible;

We need ADDREF, right?

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> +  *aAnchors = mAccChildCount;
> 
> Is it right, if the image is just an anchor with no image maps ?

Do you keep in mind an example like <a href=""><img src=""/></a>?

> +  nsIAccessible *accessible = nsnull;
> +  CallQueryInterface(accessNode, &accessible);
> +
> +  return accessible;
> 
> We need ADDREF, right?
> 

QueryInterface always AddRefs I guess.

(In reply to comment #2)
> (From update of attachment 305963 [details] [diff] [review])
> Is the patch large because some methods were moved to a different part of the
> file?
> 

yes, GetAreaAccessible method because it breaks nsIAccessible method on two parts.

Comment 6

[alexander :surkov (:asurkov)]

Marco, I thought this bug is for a bit different thing, it's about imagemap doesn't implement that interface but you say about area.

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch2

with ginn's comment

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> Marco, I thought this bug is for a bit different thing, it's about imagemap
> doesn't implement that interface but you say about area.
> 

looking at code area should implement nsIAccessibleHyperlink interface, don't they do?

Comment 9

[Marco Zehe (:MarcoZ)]

(In reply to comment #8)
> (In reply to comment #6)
> > Marco, I thought this bug is for a bit different thing, it's about imagemap
> > doesn't implement that interface but you say about area.

html:area tags make up the image map. :-) Remember the first patch to bug 418368, and that the imagge map example couldn't be tested because the links contained within that image map don't implement the nsIAccessibleHyperLink interface. That's exactly what this bug is about.

> looking at code area should implement nsIAccessibleHyperlink interface, don't
> they do?

No.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > Marco, I thought this bug is for a bit different thing, it's about imagemap
> > > doesn't implement that interface but you say about area.
> 
> html:area tags make up the image map. :-) Remember the first patch to bug
> 418368, and that the imagge map example couldn't be tested because the links
> contained within that image map don't implement the nsIAccessibleHyperLink
> interface. That's exactly what this bug is about.

Yes, it's exactly this bug is about after you changed the bug summary :) In the patch I implement nsIAccessibleHyperlink interface on imagemap accessible (parent of area accessibles).

> > looking at code area should implement nsIAccessibleHyperlink interface, don't
> > they do?
> 
> No.
> 

I'll look what's wrong with area accessibles.

Comment 11

[Marco Zehe (:MarcoZ)]

My original trouble was that, whenever I asked for one of the link child objects of the html:area object, that these children did not implement nsIAccessibleHyperLink, only nsIAccessible. If the patch that Ginn just granted review for fixes that I think we'll be fine.

Comment 12

[Aaron Leventhal]

This is important for 1.9 because screen reader users need access to the URL of any link they might activate.

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch3

* expose nsIAccessibleHyperLink on area always because nsAccessible exposes it only if it is contained by hypertext accessible
* use GetChildCount in getAnchors() to cache children.

Comment 14

[Marco Zehe (:MarcoZ)]

Alexander, if this should go into Beta4, you should request approval 1.9b4 for the patch.

Comment 15

[Aaron Leventhal]

I don't think this has to get into beta 4. As long as it gets into 1.9.

Comment 16

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

Comment on attachment 306008 [details] [diff] [review]
patch3

a=beltzner for after beta 4 freeze

Comment 17

[Joanmarie Diggs]

Alexander (and all), I just did a quick try of this patch (will test more thoroughly later) and it works like a charm.  Awesome!!  Thanks for fixing it so quickly.

Comment 18

[Marco Zehe (:MarcoZ)]

Alexander, has the tree reopened after B4 so you can check this one in?

Comment 19

[alexander :surkov (:asurkov)]

checked in

/cvsroot/mozilla/accessible/src/html/nsHTMLAreaAccessible.cpp,v  <--  nsHTMLAreaAccessible.cpp
new revision: 1.32; previous revision: 1.31
done
Checking in accessible/src/html/nsHTMLAreaAccessible.h;
/cvsroot/mozilla/accessible/src/html/nsHTMLAreaAccessible.h,v  <--  nsHTMLAreaAccessible.h
new revision: 1.17; previous revision: 1.16
done
Checking in accessible/src/html/nsHTMLImageAccessible.cpp;
/cvsroot/mozilla/accessible/src/html/nsHTMLImageAccessible.cpp,v  <--  nsHTMLImageAccessible.cpp
new revision: 1.47; previous revision: 1.46
done
Checking in accessible/src/html/nsHTMLImageAccessible.h;
/cvsroot/mozilla/accessible/src/html/nsHTMLImageAccessible.h,v  <--  nsHTMLImageAccessible.h
new revision: 1.25; previous revision: 1.24
done

Comment 20

[alexander :surkov (:asurkov)]

there is mochitest in source http://lxr.mozilla.org/mozilla/source/accessible/tests/mochitest/test_nsIAccessibleHyperLink.html