Link children associated with imagemaps do not implement the nsIAccessibleHyperLink interface

RESOLVED FIXED

Status

()

Core
Disability Access APIs
P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Joanmarie Diggs, Assigned: surkov)

Tracking

(Blocks: 2 bugs, {access})

Trunk
access
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

12.69 KB, patch
Aaron Leventhal
: review+
beltzner
: approval1.9b4-
beltzner
: approval1.9+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
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!
(Assignee)

Comment 1

10 years ago
Created attachment 305963 [details] [diff] [review]
patch
Assignee: aaronleventhal → surkov.alexander
Status: NEW → ASSIGNED
Attachment #305963 - Flags: review?(marco.zehe)

Comment 2

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

10 years ago
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)

Comment 4

10 years ago
+  *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?
(Assignee)

Comment 5

10 years ago
(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.

Updated

10 years ago
Blocks: 396346, 417472
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
(Assignee)

Comment 6

10 years ago
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.
(Assignee)

Comment 7

10 years ago
Created attachment 305996 [details] [diff] [review]
patch2

with ginn's comment
Attachment #305963 - Attachment is obsolete: true
Attachment #305996 - Flags: review?(ginn.chen)
Attachment #305963 - Flags: review?(ginn.chen)
(Assignee)

Comment 8

10 years ago
(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?

Updated

10 years ago
Attachment #305996 - Flags: review?(ginn.chen) → review+

Comment 9

10 years ago
(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.
(Assignee)

Comment 10

10 years ago
(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

10 years ago
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.

Updated

10 years ago
Attachment #305996 - Flags: approval1.9?

Comment 12

10 years ago
This is important for 1.9 because screen reader users need access to the URL of any link they might activate.
Flags: blocking1.9?
(Assignee)

Comment 13

10 years ago
Created attachment 306008 [details] [diff] [review]
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.
Attachment #305996 - Attachment is obsolete: true
Attachment #306008 - Flags: review?(aaronleventhal)
Attachment #305996 - Flags: approval1.9?

Updated

10 years ago
Attachment #306008 - Flags: review?(aaronleventhal) → review+
(Assignee)

Updated

10 years ago
Attachment #306008 - Flags: approval1.9?

Comment 14

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

Comment 15

10 years ago
I don't think this has to get into beta 4. As long as it gets into 1.9.
(Assignee)

Updated

10 years ago
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+
(Reporter)

Comment 17

10 years ago
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.

Updated

10 years ago
Flags: tracking1.9? → blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2

Comment 18

10 years ago
Alexander, has the tree reopened after B4 so you can check this one in?
(Assignee)

Comment 19

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 20

10 years ago
there is mochitest in source http://lxr.mozilla.org/mozilla/source/accessible/tests/mochitest/test_nsIAccessibleHyperLink.html
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.