548291 - Support ARIA on area (map) elements

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

We currently don't support this; reported to me by Steve (TPG).

Fixing this will allow the usemap technique for canvas in FF.

Test case:
http://www.paciellogroup.com/blog/misc/map.html

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: simplest fix

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Note: I thought about using our typical way to construct accessibles for area elements. The reason we don't already do this is that one area element can be used for multiple maps (image maps), and our cache would simply return the first one it created. So I left things as they are but just set the role map entry so that ARIA will get picked up.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Marco, want to give it a whirl?
https://build.mozilla.org/tryserver-builds/[e-mail]-try-cbe09305cc46/

Test case:
http://www.paciellogroup.com/blog/misc/map.html

Comment 4

[Marco Zehe (:MarcoZ)]

A few problems:
1. Each item has the "linked" state set. On a button, text box etc., this is not appropriate.
2. The menuitem role isn't being picked up. Others are OK.
3. Several names aren't being picked up. Basically everything after "textbox disabled" does not get its proper name set. Rather, the link URL is set as both the name and description.

Comment 5

[alexander :surkov (:asurkov)]

We don't need to set up role map entry every time when the area accessible is requested, it should be done when accessible is created and initialized (see InitAccessible for reference). Ideally I would like to deal with area accessibles in more general way than we do now. Please add mochitests as well.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks guys, this confirms a small change like this isn't enough.

(In reply to comment #5)
> We don't need to set up role map entry every time when the area accessible is
> requested, it should be done when accessible is created and initialized (see
> InitAccessible for reference).

Yes InitAccessible does essentially the same thing and I agree we should call that when we create the object, thanks.

> Ideally I would like to deal with area
> accessibles in more general way than we do now. Please add mochitests as well.

I think I am thinking the same thing. What do you have in mind? It seems to me the only special thing about nsHTMLAreaAccessible is the GetBounds...

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: part 1 (add role map when created)

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #4)
> 2. The menuitem role isn't being picked up. Others are OK.

TPG bug: role="menutiem"

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #4)
> A few problems:
> 1. Each item has the "linked" state set. On a button, text box etc., this is
> not appropriate.

I'll fix on a follow up when we land this patch.

> 3. Several names aren't being picked up. Basically everything after "textbox
> disabled" does not get its proper name set. Rather, the link URL is set as both
> the name and description.

I'll file a follow up when we land.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: part 2 (add link states only when appropriate)

(In reply to comment #9)
> (In reply to comment #4)
> > A few problems:
> > 1. Each item has the "linked" state set. On a button, text box etc., this is
> > not appropriate.
> 
> I'll fix on a follow up when we land this patch.

On second thought might as well fix it here :) I can land along with the first patch if you guys agree. Tested to work in domi.

Comment 11

[Marco Zehe (:MarcoZ)]

Comment on attachment 430063 [details] [diff] [review]
part 2 (add link states only when appropriate)

What about other states? If I see this correctly, even setting something like the "disabled" state like shown in the one textbox, or the "checked/unchecked" states on the checkboxes will not get set if we just return.

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #11)
> (From update of attachment 430063 [details] [diff] [review])
> What about other states? If I see this correctly, even setting something like
> the "disabled" state like shown in the one textbox, or the "checked/unchecked"
> states on the checkboxes will not get set if we just return.

It is hard to catch via patch inspection. The important call is nsAccessible::GetState, which happens to call GetStateInternal before calling GetARIAState (since we want ARIA to override). Adding an nsHTMLAreaAccessible GetStateInternal (note nsHTMLAreaAccessible is derived indirectly from nsAccessible) is safe here and we just bail if it isn't a link, and call the nsHTMLLinkAccessible::GetStateInternal otherwise, to add the link states.

Try build coming.

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Try build with patch 1 and 2: https://build.mozilla.org/tryserver-builds/[e-mail]-try-933539cf70c0/

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 430022 [details] [diff] [review]
part 1 (add role map when created)


>     accessNode = new nsHTMLAreaAccessible(domNode, this, mWeakShell);
>     if (!accessNode)
>       return nsnull;
> 
>     nsresult rv = accessNode->Init();
>     if (NS_FAILED(rv))
>       return nsnull;
> 
>+    // We should respect ARIA on area elements (for the a8e canvas map technique)
>+    nsAccessible* acc = reinterpret_cast<nsAccessible *>(accessNode.get());
>+    acc->SetRoleMapEntry(nsAccUtils::GetRoleMapEntry(domNode));
>+

Change GetAreaAccessible() to return already_AddRefed<nsAccessible> to avoid casting.

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 430063 [details] [diff] [review]
part 2 (add link states only when appropriate)

>diff --git a/accessible/src/html/nsHTMLAreaAccessible.cpp b/accessible/src/html/nsHTMLAreaAccessible.cpp
>--- a/accessible/src/html/nsHTMLAreaAccessible.cpp
>+++ b/accessible/src/html/nsHTMLAreaAccessible.cpp
>@@ -85,16 +85,28 @@ nsHTMLAreaAccessible::GetDescription(nsA
>   // Still to do - follow IE's standard here
>   nsCOMPtr<nsIDOMHTMLAreaElement> area(do_QueryInterface(mDOMNode));
>   if (area) 
>     area->GetShape(aDescription);
> 
>   return NS_OK;
> }
> 
>+nsresult
>+nsHTMLAreaAccessible::GetStateInternal(PRUint32 *aState, PRUint32 *aExtraState)
>+{
>+  // Bypass the link states specialization for non links.
>+  if (mRoleMapEntry && mRoleMapEntry->role != nsIAccessibleRole::ROLE_NOTHING
>+                    && mRoleMapEntry->role != nsIAccessibleRole::ROLE_LINK) {
>+    return NS_OK;

obviously we don't want to lost all calculated states in this case.

>+  }
>+
>+  return nsHTMLLinkAccessible::GetStateInternal(aState,aExtraState);

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 430063 [details] [diff] [review]
part 2 (add link states only when appropriate)


>+nsresult
>+nsHTMLAreaAccessible::GetStateInternal(PRUint32 *aState, PRUint32 *aExtraState)
>+{


please put the method into proper section, it should be "nsAccessible public implementation".

ah, canceled Marco's review previously, I hope it's ok :) canceling my review as well.

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #11 and comment #15)

Doh, you guys are right! My bad. Will fix. I was thinking your comment was regarding aria states only.

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: part 1

Better part 1.

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 430218 [details] [diff] [review]
part 1

Cancelling review request until I add mochitests.

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: combined patch 1 (with tests)

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

Oops sorry, that contains the image data.

Comment 22

[Marco Zehe (:MarcoZ)]

Comment on attachment 430323 [details] [diff] [review]
combined patch 1 (with tests)

>+    // XXX this fails: testStates("cb3", STATE_UNAVAILABLE);

Why does this fail, have you investigated?

>\ No newline at end of file

Nit: Please add this. :)

Comment 23

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #22)
> (From update of attachment 430323 [details] [diff] [review])
> >+    // XXX this fails: testStates("cb3", STATE_UNAVAILABLE);
> 
> Why does this fail, have you investigated?

I don't know. I think I want to change states.js. What disabled controls do we expect to be focusable?

> 
> >\ No newline at end of file
> 
> Nit: Please add this. :)

Done - thanks.

BTW try build: https://build.mozilla.org/tryserver-builds/[e-mail]-try-73b62d7382c9/

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

Related code from states.js is currently:

  // unavailable
  if (state & STATE_UNAVAILABLE) {
    var role = getRole(aAccOrElmOrID);
    if (role != ROLE_GROUPING && role != ROLE_EMBEDDED_OBJECT)
      isState(state & STATE_FOCUSABLE, STATE_FOCUSABLE, false,
              "Disabled " + id + " must be focusable!");
  }

Comment 25

[Marco Zehe (:MarcoZ)]

This is consistent with the way at least Windows handles it. Except for menu items. every control that is grayed out also cannot be tabbed to. Don't current yremember how a disabled form control on a web page is handled. I believe tabbing skips over those, too.

Comment 26

[Marco Zehe (:MarcoZ)]

This try-server build doesn't see anything on the Paciello Group ssample page linked to from comment #0. Did they break it or does the patch not work as expected?

Comment 27

[Marco Zehe (:MarcoZ)]

Sorry for the spam, the TPG page is currently at fault, also doesn't work with a "regular" build right now.

Comment 28

[David Bolter [:davidb] (NeedInfo me for attention)]

Right TPG changed it to a canvas based example.

Comment 29

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #22)
> (From update of attachment 430323 [details] [diff] [review])
> >+    // XXX this fails: testStates("cb3", STATE_UNAVAILABLE);
> 
> Why does this fail, have you investigated?

I should mention that the real issue here is that the disabled attribute has no affect on <area> elements. If we need that I think it should be a follow up.

Comment 30

[alexander :surkov (:asurkov)]

Comment on attachment 430323 [details] [diff] [review]
combined patch 1 (with tests)


>+nsresult
>+nsHTMLAreaAccessible::GetStateInternal(PRUint32 *aState, PRUint32 *aExtraState)
>+{
>+  // Bypass the link states specialization for non links.
>+  if (mRoleMapEntry && mRoleMapEntry->role != nsIAccessibleRole::ROLE_NOTHING

what is the reason of nothing role?

>-    nsresult rv = accessNode->Init();
>+    nsresult rv = accessible->Init();
>     if (NS_FAILED(rv))

call Shutdown()


>+  already_AddRefed<nsAccessible>
>     GetAreaAccessible(nsIDOMHTMLCollection* aAreaNodes, PRInt32 aAreaNum);

please add small comment here

>+		test_aria_usemap.html \

I would suggest to split this test on two: tree/test_aria_(img)map.html and states/test_aria.html (or states/test_aria_(img)map.html)

>+++ b/accessible/tests/mochitest/test_aria_usemap.html
>@@ -0,0 +1,107 @@
>+<!DOCTYPE html>
>+<html>
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=548291
>+-->

you don't need this, it's dupe of bug list below

>+  function doTest()
>+  {
>+    var accTree = {
>+      role: ROLE_IMAGE_MAP,
>+      children: [
>+                 {

right indent please

>+    // XXX this fails: testStates("cb3", STATE_UNAVAILABLE);

please file a bug and point it here if you don't want to deal with it now

>\ No newline at end of file

as Marco said please fix it

Comment 31

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #30)
> (From update of attachment 430323 [details] [diff] [review])
> 
> >+nsresult
> >+nsHTMLAreaAccessible::GetStateInternal(PRUint32 *aState, PRUint32 *aExtraState)
> >+{
> >+  // Bypass the link states specialization for non links.
> >+  if (mRoleMapEntry && mRoleMapEntry->role != nsIAccessibleRole::ROLE_NOTHING
> 
> what is the reason of nothing role?

For cases where the role maps to ROLE_NOTHING, we want to use the native element role, and keep the link states.
(Similar to http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessibilityService.cpp#526)

> 
> >-    nsresult rv = accessNode->Init();
> >+    nsresult rv = accessible->Init();
> >     if (NS_FAILED(rv))
> 
> call Shutdown()
> 

Done.

> 
> >+  already_AddRefed<nsAccessible>
> >     GetAreaAccessible(nsIDOMHTMLCollection* aAreaNodes, PRInt32 aAreaNum);
> 
> please add small comment here
> 
> >+		test_aria_usemap.html \
> 
> I would suggest to split this test on two: tree/test_aria_(img)map.html and
> states/test_aria.html (or states/test_aria_(img)map.html)

That makes sense because of the directory organization, but at the same time I like to see these tests in the same file. It is sort of like having a document.

> 
> >+++ b/accessible/tests/mochitest/test_aria_usemap.html
> >@@ -0,0 +1,107 @@
> >+<!DOCTYPE html>
> >+<html>
> >+<!--
> >+https://bugzilla.mozilla.org/show_bug.cgi?id=548291
> >+-->
> 
> you don't need this, it's dupe of bug list below

deleted locally.

> 
> >+  function doTest()
> >+  {
> >+    var accTree = {
> >+      role: ROLE_IMAGE_MAP,
> >+      children: [
> >+                 {
> 
> right indent please

Done.

> 
> >+    // XXX this fails: testStates("cb3", STATE_UNAVAILABLE);
> 
> please file a bug and point it here if you don't want to deal with it now

I removed it as per out chat.

> 
> >\ No newline at end of file

Already done :)

Comment 32

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: combined patch 2

Comments addressed.

Comment 33

[Marco Zehe (:MarcoZ)]

Comment on attachment 430620 [details] [diff] [review]
combined patch 2

Thanks! r=me

Comment 34

[alexander :surkov (:asurkov)]

Comment on attachment 430620 [details] [diff] [review]
combined patch 2


>@@ -143,16 +143,28 @@ nsHTMLAreaAccessible::GetChildAtPoint(PR

>+nsresult
>+nsHTMLAreaAccessible::GetStateInternal(PRUint32 *aState, PRUint32 *aExtraState)

please move it before GetChildAtPoint


>   // nsAccessible
>   virtual nsresult GetNameInternal(nsAString& aName);
>   virtual nsresult GetChildAtPoint(PRInt32 aX, PRInt32 aY,
>                                    PRBool aDeepestChild,
>                                    nsIAccessible **aChild);
>+  virtual nsresult GetStateInternal(PRUint32 *aState, PRUint32 *aExtraState);

should be between GetNameInternal and GetChildAtPoint

>+    nsRefPtr<nsAccessible> accessible;
>     accessible = GetAreaAccessible(mapAreas, aIndex);

nsRefPtr<nsAccessible> accessible = GetAreaAccessible(mapAreas, aIndex);

>+    mAccessNodeCache->Put(key, accessible);

if (!mAccessNodeCache->Put(key, accessible))
  return nsnull;

>+
>+  // Return the cached (or created here) accessible object representing an area.

use /** */

it might be just return accessible for area element.

> _TEST_FILES =\
> 		test_aria.html \
>+		test_aria_map.html \

test_aria_imgmap ?

>+    // Test states.
>+    testStates("t1", 0, EXT_STATE_EDITABLE);
>+    testStates("t2", 0, EXT_STATE_EDITABLE);
>+    testStates("rb1", (STATE_CHECKABLE | STATE_CHECKED));
>+    testStates("rb2", STATE_CHECKABLE, 0, STATE_CHECKED);
>+    testStates("cb1", (STATE_CHECKABLE | STATE_CHECKED));
>+    testStates("cbox",(STATE_HASPOPUP | STATE_COLLAPSED), EXT_STATE_EXPANDABLE);
>+    testStates("cb2", STATE_CHECKABLE);

please add tests they do not have link specific states

>+<a target="_blank"
>+  href="https://bugzilla.mozilla.org/show_bug.cgi?id=548291">

@title please with the bug summary

>+ Mozilla Bug 548291

> _TEST_FILES =\
>+		test_aria_map.html \

the same ?

>+++ b/accessible/tests/mochitest/tree/test_aria_map.html

>+    // Test image map tree structure, roles, and names.
>+    testAccessibleTree("imagemap", accTree);
>+
>+    // Test states.
>+    testStates("t1", 0, EXT_STATE_EDITABLE);
>+    testStates("t2", 0, EXT_STATE_EDITABLE);
>+    testStates("rb1", (STATE_CHECKABLE | STATE_CHECKED));
>+    testStates("rb2", STATE_CHECKABLE, 0, STATE_CHECKED);
>+    testStates("cb1", (STATE_CHECKABLE | STATE_CHECKED));
>+    testStates("cbox",(STATE_HASPOPUP | STATE_COLLAPSED), EXT_STATE_EXPANDABLE);
>+    testStates("cb2", STATE_CHECKABLE);

no state check is needed

>+<a target="_blank"
>+  href="https://bugzilla.mozilla.org/show_bug.cgi?id=548291">

title please

Comment 35

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks guys. Let's hope this helps.

Landed on central:
http://hg.mozilla.org/mozilla-central/rev/34117a5795c4 and
http://hg.mozilla.org/mozilla-central/rev/ee9dd725d8ea (missing tests)

Comment 36

[David Bolter [:davidb] (NeedInfo me for attention)]

Backed out: http://hg.mozilla.org/mozilla-central/rev/5de3bdcca2cf

Comment 37

[David Bolter [:davidb] (NeedInfo me for attention)]

I can recreate this locally, which is good.

Comment 38

[David Bolter [:davidb] (NeedInfo me for attention)]

Alex, please ping me if you add cycle collector smarts.

Comment 39

[alexander :surkov (:asurkov)]

(In reply to comment #38)
> Alex, please ping me if you add cycle collector smarts.

done, in bug 545467

Comment 40

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: updated patch

Well this got a lot simpler! Tests pass, no crash :)

Comment 41

[alexander :surkov (:asurkov)]

Comment on attachment 437899 [details] [diff] [review]
updated patch


> nsresult
>+nsHTMLAreaAccessible::GetStateInternal(PRUint32 *aState, PRUint32 *aExtraState)
>+{
>+  // Bypass the link states specialization for non links.
>+  if (mRoleMapEntry && mRoleMapEntry->role != nsIAccessibleRole::ROLE_NOTHING
>+                    && mRoleMapEntry->role != nsIAccessibleRole::ROLE_LINK) {

nit: no && operator on the line start, line up mRoleMapEntry->role != nsIAccessibleRole::ROLE_LINK to mRoleMapEntry above

>+    return nsAccessible::GetStateInternal(aState,aExtraState);

nit: space between aState,aExtraState

> _TEST_FILES =\
> 		test_aria.html \
>+		test_aria_imagemap.html \

imgmap to be shorter?

>+    testStates("t1", 0, EXT_STATE_EDITABLE, STATE_LINKED);
>+    testStates("t2", 0, EXT_STATE_EDITABLE, STATE_LINKED);
>+    testStates("rb1", (STATE_CHECKABLE | STATE_CHECKED), 0, STATE_LINKED);
>+    testStates("rb2", STATE_CHECKABLE, 0, STATE_CHECKED, STATE_LINKED);

it sounds like STATE_LINKED here is used as extra state

>+    testStates("cb1", (STATE_CHECKABLE | STATE_CHECKED), 0, STATE_LINKED);
>+    testStates("cbox",(STATE_HASPOPUP | STATE_COLLAPSED), EXT_STATE_EXPANDABLE
>+                     , STATE_LINKED);

nit: space between "cbox" and second argument
nit: no "," on the line start

>+
>+<img id="imagemap" src="../formimage.png" width="219" height="229" border="0" usemap="#aMap">
>+<map id="aMap" name="aMap">

nit: what's aMap means? areaMap? please use move descriptive name

>+  <area id="cb2" role="checkbox" shape="rect" alt="" title="have car"  coords="90,145,114,164" href="#" />
>+  <area id="cb3" role="checkbox" shape="rect" alt="" title="have airplane"  coords="130,163,152,184" href="#" />
>+  <area id="b1" role="button" shape="rect" alt="" title="submit"  coords="4,198,67,224" href="#" />

nit: these aren't used in test, is it  artefact or do you like to keep them?

Comment 42

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch to land

Thanks Alexander. I'll land when the tree opens.

(In reply to comment #41)
> nit: no && operator on the line start, line up mRoleMapEntry->role !=

Done.

> nit: space between aState,aExtraState

Done.

> imgmap to be shorter?

Done.

> nit: space between "cbox" and second argument
> nit: no "," on the line start

Done.

> 
> nit: what's aMap means? areaMap? please use move descriptive name

Done.

> nit: these aren't used in test, is it  artefact or do you like to keep them?

I like them for consistency.

Comment 43

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 438079 [details] [diff] [review]
patch to land

Landed http://hg.mozilla.org/mozilla-central/rev/38450301dc01