Closed Bug 419786 Opened 12 years ago Closed 12 years ago

Link children associated with imagemaps do not implement the nsIAccessibleHyperLink interface

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: jdiggs, Assigned: surkov)

References

(Blocks 2 open bugs, )

Details

(Keywords: access)

Attachments

(1 file, 2 obsolete files)

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!
Attached patch patch (obsolete) — Splinter Review
Assignee: aaronleventhal → surkov.alexander
Status: NEW → ASSIGNED
Attachment #305963 - Flags: review?(marco.zehe)
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 on attachment 305963 [details] [diff] [review]
patch

I don't think Marco is quite ready to be a reviewer for this one.
Attachment #305963 - Flags: review?(marco.zehe) → review?(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?
(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.
OS: Linux → All
Hardware: PC → All
Summary: Cannot access URIs associated with imagemaps via at-spi → Link children associated with imagemaps do not implement the nsIAccessibleHyperLink interface
Version: unspecified → Trunk
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.
Attached patch patch2 (obsolete) — Splinter Review
with ginn's comment
Attachment #305963 - Attachment is obsolete: true
Attachment #305996 - Flags: review?(ginn.chen)
Attachment #305963 - Flags: review?(ginn.chen)
(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?
Attachment #305996 - Flags: review?(ginn.chen) → review+
(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.
(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.
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.
Attachment #305996 - Flags: approval1.9?
This is important for 1.9 because screen reader users need access to the URL of any link they might activate.
Flags: blocking1.9?
Attached patch patch3Splinter Review
* expose nsIAccessibleHyperLink on area always because nsAccessible exposes it only if it is contained by hypertext accessible
* use GetChildCount in getAnchors() to cache children.
Attachment #305996 - Attachment is obsolete: true
Attachment #306008 - Flags: review?(aaronleventhal)
Attachment #305996 - Flags: approval1.9?
Attachment #306008 - Flags: review?(aaronleventhal) → review+
Attachment #306008 - Flags: approval1.9?
Alexander, if this should go into Beta4, you should request approval 1.9b4 for the patch.
I don't think this has to get into beta 4. As long as it gets into 1.9.
Attachment #306008 - Flags: approval1.9b4?
Comment on attachment 306008 [details] [diff] [review]
patch3

a=beltzner for after beta 4 freeze
Attachment #306008 - Flags: approval1.9b4?
Attachment #306008 - Flags: approval1.9b4-
Attachment #306008 - Flags: approval1.9?
Attachment #306008 - Flags: approval1.9+
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.
Flags: tracking1.9? → blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Alexander, has the tree reopened after B4 so you can check this one in?
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

Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.