961696 - Accessible object:state-changed:focused events for imagemap links are broken

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Attachment: accessible-event listener

Steps to reproduce:
1. Launch the attached accessible-event listener in a terminal
2. Load the attached test case
3. Tab to move amongst the links

Expected results: Each time a link in the imagemap becomes focused, there would be an object:state-changed:focused event for that link.

Actual results: Each time a link in the imagemap becomes focused, there is an object:state-changed:focused event, but it's for the document frame as shown below (even though the links are in the accessible tree).

Impact: Orca cannot report the focused link.

-> [document frame | file:///home/jd/imagemap.html]
    -> [paragraph | ]
    -> [image | Planets]
        -> [link | Sun]
        -> [link | Mercury]
        -> [link | Venus]
object:state-changed:focused(1, 0, 0)
	source: [document frame | file:///home/jd/imagemap.html]
	host_application: [application | Firefox]
object:state-changed:focused(1, 0, 0)
	source: [document frame | file:///home/jd/imagemap.html]
	host_application: [application | Firefox]
object:state-changed:focused(1, 0, 0)
	source: [document frame | file:///home/jd/imagemap.html]
	host_application: [application | Firefox]

Comment 1

[Joanmarie Diggs]

Attachment: testcase: imagemap.html

Comment 2

[alexander :surkov (:asurkov)]

same behavior on crossplatform player

Comment 3

[alexander :surkov (:asurkov)]

So the problem here is HTML area which receive the focus is not accessible (DocAccessible::GetAccessible(HTMLAreaElm) returns null). It happens because AREA accessible has eNotNodeMapEntry flag and thus it is not included into node map cache of DocAccessible. The eNotNodeMapEntry flag is used to ignore HTML area elements for accessible tree updates (HTML image map manages its children itself). So no good ideas how to fix it at the first glance.

Jonathan, just in case, if you want something about code juggling then this is a good option.

Comment 4

[Jonathan Wei [:jwei]]

Attachment: Added area accessibles to node cache

Created a new accessible type for image areas.  We originally kept them out of the node cache so that they wouldn't be updated during UpdateTree, so they've been added to the cache but will be ignored when walking down and updating child accessibles.

Comment 5

[alexander :surkov (:asurkov)]

if eNotNodeMapEntry is used by document accessible only then it makes sense to rely on eDocument (at least until ARIA documents picked it up) and then you can rename eNotNodeMapEntry to eParentDependent or something else and use it for area accessibles.

Comment 6

[Jonathan Wei [:jwei]]

Attachment: Added special case for image area elements in FocusManager

Previous solution might introduce unwanted behaviour in TreeWalker and would also report incorrect accessibles (one map can be used for many images).  Added utility method in FocusManager, encounters similar problem as the first solution for one map -> many images but will be fixed once 135040 is fixed.

Comment 7

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8367537 [details] [diff] [review]
Added special case for image area elements in FocusManager

>+FocusManager::GetFocusableAccessibleFor(nsINode* aNode,
>+                                        DocAccessible* aDoc) const

I tend to think this logic belongs in DocAccessible, but I guess its ok here for now.

>+{
>+  if (!aNode || !aNode->IsContent() ||

I think it would better to null check at the two call sites that don't already and then assume the node is nonnull.

>+      !aNode->AsContent()->IsHTML(nsGkAtoms::area))
>+    return aDoc->GetAccessibleOrContainer(aNode);
>+
>+  // XXX: Hack, fix to determine proper image element when 135040 gets fixed.

standard form would be
// XXX bug xxxxxxx blah blah.

>+
>+  nsIFrame* frame = aNode->AsContent()->GetPrimaryFrame();

drop the blank line

>+  nsImageFrame* imageFrame = do_QueryFrame(frame);
>+
>+  if (imageFrame) {

extra blank line

>+    Accessible* parentAcc = aDoc->GetAccessible(imageFrame->GetContent());
>+
>+    if (parentAcc) {

extra blank line.

>+      HTMLImageMapAccessible* imageMap = parentAcc->AsImageMap();
>+      Accessible* areaAcc = imageMap->GetChildAccessibleFor(aNode);

kill the local var since its only used once?

>+
>+      if (areaAcc)
>+        return areaAcc;

I don't think you want to fall through if the image map accessible didn't have it as a kid, returning null seems better than something that's probably wrong.

>+HTMLImageMapAccessible::GetChildAccessibleFor(nsINode* aNode)

const nsINode*

>+{
>+  for (uint32_t i = 0; i < mChildren.Length(); ++i) {

i++ and manually cse mChildren.Length()

>+    Accessible* area = mChildren.ElementAt(i);

mChildren[i]

>+    if (area->GetNode() == aNode)

GetContent()

you should really be able to write tests that the right accessible gets focused, so please do that.

Comment 8

[Jonathan Wei [:jwei]]

Attachment: Added special case for image area elements in FocusManager v2

Addressed concerns and added test case.

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8370988 [details] [diff] [review]
Added special case for image area elements in FocusManager v2

>+FocusManager::GetFocusableAccessibleFor(nsINode* aNode,
>+                                        DocAccessible* aDoc) const
>+{
>+  if (!aNode->IsContent() || !aNode->AsContent()->IsHTML(nsGkAtoms::area))
>+    return aDoc->GetAccessibleOrContainer(aNode);
>+
>+  // XXX: Bug 961696, fix to determine proper image element when bug 135040 has
>+  // a proper solution.

err, I guess I was unclear more like
// XXX bug 135040 incorrect when multiple images use the same map.


>+  nsIFrame* frame = aNode->AsContent()->GetPrimaryFrame();
>+  nsImageFrame* imageFrame = do_QueryFrame(frame);
>+  if (imageFrame) {
>+    Accessible* parentAcc = aDoc->GetAccessible(imageFrame->GetContent());
>+    if (parentAcc) {
>+      Accessible* areaAcc =

drop the Acc bit from these names just use parent and area

>       {
>         return "Focus element while subdocument is focused " + prettyName(aID);
>       }
>     }
> 
>+    function imageMapChecker()
doesn't the generic focusChecker work? if not please check more than just the role is correct, I'd think you should be able to check you atleast got the right <area>

>+  <map name="planetmap">
>+    <area shape="rect" coords="0,0,82,126" alt="Sun" href="#sun" id="a1">
>+    <area shape="circle" coords="90,58,3" alt="Mercury" href="#mercury" id="a2">
>+    <area shape="circle" coords="124,58,8" alt="Venus" href="#venus" id="a3">

I think you can drop the alt= and the href=

>+  </map>
>+  <img width="146" height="126" alt="Planets" usemap="#planetmap" src="../planets.gif">

if we can it might be good to have two maps so this test breaks when bug 135040 is fixed.
>new file mode 100644
>index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..dcf55855a9a0224d63864511c9e000f323670638
>GIT binary patch

what's wrong with existing images?

Comment 10

[Jonathan Wei [:jwei]]

(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> 
> >       {
> >         return "Focus element while subdocument is focused " + prettyName(aID);
> >       }
> >     }
> > 
> >+    function imageMapChecker()
> doesn't the generic focusChecker work? if not please check more than just
> the role is correct, I'd think you should be able to check you atleast got
> the right <area>

It won't due to its usage of getAccessible (which will still return NULL going through document->GetAccessible()) so I'll make the imageMapChecker more specific.

> >new file mode 100644
> >index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..dcf55855a9a0224d63864511c9e000f323670638
> >GIT binary patch
> 
> what's wrong with existing images?

Just used the same image as the test case attached to this bug, but I'll go find a suitable image that we already have.

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to Jonathan Wei [:jwei] from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> > 
> > >       {
> > >         return "Focus element while subdocument is focused " + prettyName(aID);
> > >       }
> > >     }
> > > 
> > >+    function imageMapChecker()
> > doesn't the generic focusChecker work? if not please check more than just
> > the role is correct, I'd think you should be able to check you atleast got
> > the right <area>
> 
> It won't due to its usage of getAccessible (which will still return NULL
> going through document->GetAccessible()) so I'll make the imageMapChecker
> more specific.

yeah, I guess you could write something like
function match(aPossibleTarget)
{
  return aPossibleTarget.DOMNode == this.mNode;
}
and pass getNode(area2) to imageMapChecker.

> > >new file mode 100644
> > >index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..dcf55855a9a0224d63864511c9e000f323670638
> > >GIT binary patch
> > 
> > what's wrong with existing images?
> 
> Just used the same image as the test case attached to this bug, but I'll go
> find a suitable image that we already have.

I'd expect any of the ones in the manifest next to the one you added would do.

Comment 12

[Jonathan Wei [:jwei]]

Attachment: Added special case for image area elements in FocusManager v3

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0a2aae7b0d4

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d0a2aae7b0d4