Closed Bug 548291 Opened 10 years ago Closed 10 years ago

Support ARIA on area (map) elements

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: davidb, Assigned: davidb)

References

Details

Attachments

(1 file, 7 obsolete files)

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
Assignee: nobody → bolterbugz
Attached patch simplest fix (obsolete) — Splinter Review
Attachment #429735 - Flags: review?(surkov.alexander)
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.
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.
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.
Attachment #429735 - Flags: review?(surkov.alexander)
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...
Attachment #429735 - Attachment is obsolete: true
Attachment #430022 - Flags: review?(surkov.alexander)
(In reply to comment #4)
> 2. The menuitem role isn't being picked up. Others are OK.

TPG bug: role="menutiem"
(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.
(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.
Attachment #430063 - Flags: review?(surkov.alexander)
Attachment #430063 - Flags: review?(marco.zehe)
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.
(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.
Attachment #430022 - Attachment description: protect against setting the role map twice → part 1 (add role map when created)
Attachment #430063 - Attachment description: fix link states → part 2 (add link states only when appropriate)
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.
Attachment #430022 - Flags: review?(surkov.alexander)
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);
Attachment #430063 - Flags: review?(marco.zehe)
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.
Attachment #430063 - Flags: review?(surkov.alexander)
(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.
Attached patch part 1 (obsolete) — Splinter Review
Better part 1.
Attachment #430022 - Attachment is obsolete: true
Attachment #430218 - Flags: review?(surkov.alexander)
Comment on attachment 430218 [details] [diff] [review]
part 1

Cancelling review request until I add mochitests.
Attachment #430218 - Flags: review?(surkov.alexander)
Attached patch combined patch 1 (with tests) (obsolete) — Splinter Review
Attachment #430063 - Attachment is obsolete: true
Attachment #430218 - Attachment is obsolete: true
Attachment #430323 - Flags: review?(surkov.alexander)
Attachment #430323 - Flags: review?(marco.zehe)
Oops sorry, that contains the image data.
Attachment #430323 - Flags: review?(marco.zehe)
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. :)
(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/dbolter@mozilla.com-try-73b62d7382c9/
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!");
  }
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.
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?
Sorry for the spam, the TPG page is currently at fault, also doesn't work with a "regular" build right now.
Right TPG changed it to a canvas based example.
(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 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
Attachment #430323 - Flags: review?(surkov.alexander)
(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 :)
Attached patch combined patch 2 (obsolete) — Splinter Review
Comments addressed.
Attachment #430323 - Attachment is obsolete: true
Attachment #430620 - Flags: review?(surkov.alexander)
Attachment #430620 - Flags: review?(marco.zehe)
Attachment #430620 - Flags: review?(marco.zehe) → review+
Comment on attachment 430620 [details] [diff] [review]
combined patch 2

Thanks! r=me
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
Attachment #430620 - Flags: review?(surkov.alexander) → review+
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)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Backed out: http://hg.mozilla.org/mozilla-central/rev/5de3bdcca2cf
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I can recreate this locally, which is good.
Alex, please ping me if you add cycle collector smarts.
(In reply to comment #38)
> Alex, please ping me if you add cycle collector smarts.

done, in bug 545467
Attached patch updated patch (obsolete) — Splinter Review
Well this got a lot simpler! Tests pass, no crash :)
Attachment #430620 - Attachment is obsolete: true
Attachment #437899 - Flags: review?(surkov.alexander)
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?
Attachment #437899 - Flags: review?(surkov.alexander) → review+
Attached patch patch to landSplinter Review
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.
Attachment #437899 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.