Closed Bug 451535 Opened 16 years ago Closed 16 years ago

Make nsHTMLImageAccessible return proper number of actions for images with longdescs, provide a name for opening the longdesc, not just an index.

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: MarcoZ, Assigned: MarcoZ)

References

Details

(Keywords: access)

Attachments

(1 file, 4 obsolete files)

Patch5
11.75 KB, patch
surkov
: review+
Details | Diff | Splinter Review 
Currently, nsHTMLImageAccessible::DoAction is the only action-related method that is in this class. NumActions does not reflect that there may be more than 1 action in this accessible, and getActionName does not give back a useful name for the longdesc case.

Question:
What should the action name for the longdesc opening be?
Attached patch Patch (obsolete) — — Splinter Review 
1. Add nsHTMLImageAccessible::GetNumActions and nsHTMLImageAccessible::GetActionName to return proper number and name of action if a longdesc is present.
2. Renamed the "index" parameter of nsHTMLImageAccessible::DoAction to "aIndex" for consistency.
3. Added tests for numActions and GetActionName. Am unsure about DoAction since that opens a new window and will steal focus from the test.
Assignee: nobody → marco.zehe
Attachment #334881 - Flags: review?(surkov.alexander)
Comment on attachment 334881 [details] [diff] [review]
Patch

>-NS_IMETHODIMP nsHTMLImageAccessible::DoAction(PRUint8 index)
>+NS_IMETHODIMP
>+nsHTMLImageAccessible::GetNumActions(PRUint8 *aNumActions)
> {
>-  if (index == eAction_ShowLongDescription) {
>+  NS_ENSURE_ARG_POINTER(aNumActions);
>+  *aNumActions = 0;
>+
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>+  if (content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::longDesc)) {
>+    // To get at the long desc action, an index of 1 has to be passed in.
>+    // So lie to ATs and always say we have 2 actions if a longdesc is present.
>+    // Even true if there is no click action.
>+    // Click action will be handled by nsLinkableAccessible.
>+    *aNumActions = 2;

image may be not clickable, right? If so then below your code isn't correct as well. Reask the review if I'm wrong or put new patch :)

>+<html>
>+<head>
>+<title>Mozilla logo</title>
>+</head>
>+<body>
>+<p>This file would contain a longer description of the Mozilla logo, if I knew what it looked like.</p>
>+</body>
>+</html>
>\ No newline at end of file

I would prefer more talking name? :)

could you provide a link on longdesc?
Attachment #334881 - Flags: review?(surkov.alexander)
(In reply to comment #2)
> image may be not clickable, right? If so then below your code isn't correct as
> well. Reask the review if I'm wrong or put new patch :)

I was under the assumption that an index of 1 (meaning the second action) would always be used to open the longDesc. I don't think we should change that for legacy screen reader support. So, I am faking the 2 actions even though the image may not be clickable and there is actually only 1 action, and let nsLinkableAccessible, which nsHTMLImageAccessible inherits from, handle the case where there may not be an action 0. I'm worried that if there's only a longdesc, and that would suddenly become action 0, screen readers such as JAWS or Window-Eyes would no longer be able to open the longdesc.

> I would prefer more talking name? :)

Well, for this the content is actually not important. It is only there for testing purposes. I could put a comment at the top of the file saying that this is the longdesc file for bug 451535.

> could you provide a link on longdesc?

http://www.w3.org/WAI/GL/WCAG20/WD-WCAG20-TECHS/H45.html
(In reply to comment #3)
> (In reply to comment #2)
> > image may be not clickable, right? If so then below your code isn't correct as
> > well. Reask the review if I'm wrong or put new patch :)
> 
> I was under the assumption that an index of 1 (meaning the second action) would
> always be used to open the longDesc. I don't think we should change that for
> legacy screen reader support. So, I am faking the 2 actions even though the
> image may not be clickable and there is actually only 1 action, and let
> nsLinkableAccessible, which nsHTMLImageAccessible inherits from, handle the
> case where there may not be an action 0.

I thought AT uses action name not index to identify the action. No?

> I'm worried that if there's only a
> longdesc, and that would suddenly become action 0, screen readers such as JAWS
> or Window-Eyes would no longer be able to open the longdesc.

Sorry I didn't get about "suddenly become action 0".
(In reply to comment #3)

> Well, for this the content is actually not important. It is only there for
> testing purposes. I could put a comment at the top of the file saying that this
> is the longdesc file for bug 451535.

possibly longdesc_src or I don't know, I guess we should provide a hint how can I reuse this file in future.
(In reply to comment #4)
> I thought AT uses action name not index to identify the action. No?

Normally, yes, but since bug 343156 implemented this DoAction, and hardcoded an index of 1 for this, instead of implementing the others too, I'm a bit worried that we might break somebody. The good thing is, however, that this was specifically implemented for ATK, so we can ask Joanie to give us her opinion on this.
Aaron, who pointed me to this, is right: This is hacky at best the way it is currently implemented.

(In reply to comment #5)
> (In reply to comment #3)
> > Well, for this the content is actually not important. It is only there for
> > testing purposes. I could put a comment at the top of the file saying that this
> > is the longdesc file for bug 451535.
> possibly longdesc_src or I don't know, I guess we should provide a hint how can
> I reuse this file in future.

Ah OK, I can change the name of course.
Comment on attachment 334881 [details] [diff] [review]
Patch


>-NS_IMETHODIMP nsHTMLImageAccessible::DoAction(PRUint8 index)
>+NS_IMETHODIMP
>+nsHTMLImageAccessible::GetNumActions(PRUint8 *aNumActions)
> {
>-  if (index == eAction_ShowLongDescription) {
>+  NS_ENSURE_ARG_POINTER(aNumActions);
>+  *aNumActions = 0;
>+
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>+  if (content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::longDesc)) {

it sounds it's not enough, I think we should check additionally that the link in the longdesc attribute is valid
(In reply to comment #6)
> (In reply to comment #4)
> > I thought AT uses action name not index to identify the action. No?
> 
> Normally, yes, but since bug 343156 implemented this DoAction, and hardcoded an
> index of 1 for this, instead of implementing the others too, I'm a bit worried
> that we might break somebody. The good thing is, however, that this was
> specifically implemented for ATK, so we can ask Joanie to give us her opinion
> on this.
> Aaron, who pointed me to this, is right: This is hacky at best the way it is
> currently implemented.

It would be really nice to get opinion from those who implemented it already.

But if they use DoAction(1) which is hacky and if they do not check GetNumActions then why do we need to change this and introduce new hacks?

Possibly if someone uses it and we can't force them to change it then we could save DoAction(1) for them as a hack but for other guys we can implement correct approach. At least till we can have only two actions on image then it will work.
Ginn and Aaron, do you remember the history about bug 343156? There is only a very limited discussion in the bug itself. Do you remember why the index was hard-wired instead of doing proper numActions/getActionName work?
Marco, I think it was simple laziness or time crunch.

Can we just make sure there is always 1 action of no longdesc, and 2 actions if there is one? If longdesc, make action #1 the one to showlongdesc. I don't see why that would break anyone.
(In reply to comment #10)
> Can we just make sure there is always 1 action of no longdesc, and 2 actions if
> there is one? If longdesc, make action #1 the one to showlongdesc. I don't see
> why that would break anyone.

OK, and what should that always-available action 0 be? Just fake, and if it happens to have a click, let nsLinkableAccessible handle it?

If so, then my patch is all we need I think.
Ah, no, nevermind. Don't have a fake click. You're right you'll have to not have hardcoded action #s.

Check with the Orca folks but I don't think anyone just hardcodes action numbers (at least beyond 0).
Attached patch Patch2 (obsolete) — — Splinter Review 
Completely reworked the logic. Now the action for the longdesc is no longer hardcoded to 1. This means that if an image has a click, that is action 0, and if it has a longdesc, that's action 1. However, if there is only a longdesc, but no click, that is action 0. This means that screen reader vendors must check for the correct action name before calling DoAction to see if they actually open the longdesc or perform a click.
Attachment #334881 - Attachment is obsolete: true
Attachment #335271 - Flags: review?(surkov.alexander)
Comment on attachment 335271 [details] [diff] [review]
Patch2


>-NS_IMETHODIMP nsHTMLImageAccessible::DoAction(PRUint8 index)
>+PRBool
>+nsHTMLImageAccessible::HasLongDesc()

please ensure you group methods by interfaces because it seems you put this method into nsIAccessible interface section.

> {
>-  if (index == eAction_ShowLongDescription) {
>+  if (IsDefunct())
>+    return PR_FALSE;
>+
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>+  return (content && content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::longDesc));

I think it doesn't make sense to check 'content' here.

>+NS_IMETHODIMP
>+nsHTMLImageAccessible::GetNumActions(PRUint8 *aNumActions)
>+{
>+  NS_ENSURE_ARG_POINTER(aNumActions);
>+  *aNumActions = 0;
>+
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  PRUint8 numActions = 0;

you could use aNumActions directly I think.

>+NS_IMETHODIMP
>+nsHTMLImageAccessible::GetActionName(PRUint8 aIndex, nsAString& aName)
>+{
>+  nsAutoString name;
>+  nsresult rv = nsLinkableAccessible::GetActionName(aIndex, name);
>+  if (NS_SUCCEEDED(rv) && !name.IsEmpty()) {

I think it doesn't make sense to check name here at least I don't remember the case when we don't fail and return empty name.

>+    aName.Assign(name);
>+    return rv;
>+  }
>+
>+  if (HasLongDesc()) {
>+    aName.AssignLiteral("showlongdesc"); 
>+    return NS_OK;
>+  }
>+
>+  return NS_ERROR_INVALID_ARG;
>+}
>+
>+NS_IMETHODIMP
>+nsHTMLImageAccessible::DoAction(PRUint8 aIndex)
>+{
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  if (!IsValidActionIndex(aIndex)) {
>+    return NS_ERROR_INVALID_ARG;
>+  }
>+
>+  nsresult rv = nsLinkableAccessible::DoAction(aIndex);
>+  if (NS_SUCCEEDED(rv)) {
>+    return rv;
>+  }
>+
>+  if (HasLongDesc()) {

it doesn't make sense because IsValidActionIndex should false if HasLongDesc is false.

Possibly I would prefer the IsShowLongdescIndex(aIndex) approach, so you should get

if (IsShowLongdescIndex(aIndex))
  // longdesc proccessing
else
  redirection to nsLinkableAccessible

What do you think?


>+++ b/accessible/tests/mochitest/longdesc_src.html
>@@ -0,0 +1,8 @@
>+</html>
>\ No newline at end of file

add new line please?

>-    function testThis(aID, aName, aSRC, aWidth, aHeight)
>+    function testThis(aID, aName, aSRC, aWidth, aHeight,
>+                      aNumActions, aActionIndex, aActionName)

I would prefer actually to test all actions but it's up to you and you can bring this into following up bug.

>+
>+      if (aNumActions) {
>+        is(acc.numActions, aNumActions,
>+           "Wrong number of actions for " + aID + "!");
>+
>+        // only test for longdesc action name here

this comment doesn't make sense because you can test any valid action here and your tests do this. ;)

>+        if (aActionName) {
>+          is(acc.getActionName(aActionIndex), aActionName,
>+             "Wrong action name for " + aID + "!");
>+        }
>+      }
>     }
> 
>     function doTest()
>     {
>       gAccRetrieval = Components.classes["@mozilla.org/accessibleRetrieval;1"].
>                        getService(Components.interfaces.nsIAccessibleRetrieval);
> 
>       // Test non-linked image
>@@ -142,16 +154,26 @@ https://bugzilla.mozilla.org/show_bug.cg
>       testThis("linkedImageEmptyAlt", "", "moz.png", 93, 42);
> 
>       // Test simple image with empty alt attribute and title
>       testThis("nonLinkedImageEmptyAltAndTitle", "MozillaFoundation", "moz.png", 89, 38);
> 
>       // Test linked image with empty alt attribute and title
>       testThis("linkedImageEmptyAltAndTitle", "Link to Mozilla Foundation", "moz.png", 93, 42);
> 
>+      // Image with long desc
>+      testThis("longdesc", "Image of Mozilla logo", "moz.png", 89, 38, 1, 0,
>+               "showlongdesc");
>+
>+      // Image with long desc
>+      testThis("clickAndLongdesc", "Another image of Mozilla logo", "moz.png",
>+               89, 38, 2, 0, "click");
>+      testThis("clickAndLongdesc", "Another image of Mozilla logo", "moz.png",
>+               89, 38, 2, 1, "showlongdesc");
>+

would be nice to have test for image with 'click' action only.

>       SimpleTest.finish();
>     }
> 
>     SimpleTest.waitForExplicitFinish();
>     addLoadEvent(doTest);
>   </script>
> </head>
> <body>
>@@ -177,10 +199,16 @@ https://bugzilla.mozilla.org/show_bug.cg
>   <img id="nonLinkedImageEmptyAlt" src="moz.png" alt=""/>
>   <br>Linked image with empty alt:<br>
>   <a href="http://www.mozilla.org"><img id="linkedImageEmptyAlt" src="moz.png" alt=""/></a>
>   <br>Simple image with empty alt and title:<br>
>   <img id="nonLinkedImageEmptyAltAndTitle" src="moz.png" alt="" title="MozillaFoundation"/>
>   <br>Linked image with empty alt and title:<br>
>   <a href="http://www.mozilla.org"><img id="linkedImageEmptyAltAndTitle" src="moz.png" alt=""
>      title="Link to Mozilla Foundation"/></a>
>+  <br>Image with longdesc:<br>
>+  <img id="longdesc" src="moz.png" longdesc="longdesc_src.html"
>+       alt="Image of Mozilla logo"/>
>+  <br>Image with click and longdesc:<br>
>+  <img id="clickAndLongdesc" src="moz.png" longdesc="longdesc_src.html"
>+       alt="Another image of Mozilla logo" onclick="Alert('Clicked!');"/>

not Alert but alert
Attached patch Patch3 (obsolete) — — Splinter Review 
Addressed (hopefully all of) Alexander's comments.
Attachment #335271 - Attachment is obsolete: true
Attachment #335333 - Flags: review?(surkov.alexander)
Attachment #335271 - Flags: review?(surkov.alexander)
Comment on attachment 335333 [details] [diff] [review]
Patch3


>+  nsLinkableAccessible::GetNumActions(aNumActions);
>+
>+  if (HasLongDesc()) {
>+    (*aNumActions)++;
>+  }

actually I prefer to skip { } for single statement but it seems that's me only, so it's up to you :)

>+
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsHTMLImageAccessible::GetActionName(PRUint8 aIndex, nsAString& aName)
>+{

please use IsLongDescIndex() approach here also otherwise it's some kind of cool mix ;)

so I guess you can use something like this:

PRBool
nsHTMLImageAccessible::IsLongDescIndex(aIndex)
{
  if (!HasLongDesc())
    return PR_FALSE;

  PRUint8 numActions = 0;
  nsLinkableAccessible::GetNumActions(numActions);
  return numActions == aIndex;
}

let me look one more time
Attachment #335333 - Flags: review?(surkov.alexander)
Attached patch Patch4 (obsolete) — — Splinter Review 
Next try.
Attachment #335333 - Attachment is obsolete: true
Attachment #335372 - Flags: review?(surkov.alexander)
Comment on attachment 335372 [details] [diff] [review]
Patch4


>+NS_IMETHODIMP
>+nsHTMLImageAccessible::GetNumActions(PRUint8 *aNumActions)
> {
>-  if (index == eAction_ShowLongDescription) {
>+  NS_ENSURE_ARG_POINTER(aNumActions);
>+  *aNumActions = 0;
>+
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  nsLinkableAccessible::GetNumActions(aNumActions);

not sure about this case but the general rule is safer to check nsresult here.

>+
>+PRBool
>+nsHTMLImageAccessible::IsValidActionIndex(PRUint8 aIndex)

this the name confuses me because only longdesc image is valid ;) so I would prefer previous version of the name.

>+{
>+  if (!HasLongDesc())
>+    return PR_FALSE;
>+
>+  PRUint8 numActions = 0;
>+  GetNumActions(&numActions);  
>+  return (aIndex == numActions - 1);

Why I used nsLinkableAccessible because it doesn't make sense to call HasLongDesc() method twice, agree?

>+}
>diff --git a/accessible/src/html/nsHTMLImageAccessible.h b/accessible/src/html/nsHTMLImageAccessible.h
>--- a/accessible/src/html/nsHTMLImageAccessible.h
>+++ b/accessible/src/html/nsHTMLImageAccessible.h
>@@ -50,25 +50,24 @@
>  */
> class nsHTMLImageAccessible : public nsLinkableAccessible,
>                               public nsIAccessibleImage
> {
> 
>   NS_DECL_ISUPPORTS_INHERITED
> 
> public:
>-  //action0 may exist depends on whether an onclick is associated with it
>-  enum { eAction_ShowLongDescription = 1 };
>-
>   nsHTMLImageAccessible(nsIDOMNode* aDomNode, nsIWeakReference* aShell);
> 
>   // nsIAccessible
>   NS_IMETHOD GetName(nsAString& _retval); 
>   NS_IMETHOD GetState(PRUint32 *aState, PRUint32 *aExtraState);
>   NS_IMETHOD GetRole(PRUint32 *_retval);
>+  NS_IMETHOD GetNumActions(PRUint8 *aNumActions);
>+  NS_IMETHOD GetActionName(PRUint8 aIndex, nsAString& aName);
>   NS_IMETHOD DoAction(PRUint8 index);
> 
>   // nsIAccessibleHyperLink
>   NS_IMETHOD GetAnchorCount(PRInt32 *aAnchorCount);
>   NS_IMETHOD GetURI(PRInt32 aIndex, nsIURI **aURI);
>   NS_IMETHOD GetAnchor(PRInt32 aIndex, nsIAccessible **aAccessible);
> 
>   // nsPIAccessNode
>@@ -90,12 +89,31 @@ protected:
> 
>   // Reference on linked map element if any.
>   nsCOMPtr<nsIDOMHTMLMapElement> mMapElement;
> 
>   // Cache of area accessibles. We do not use common cache because images can
>   // share area elements but we need to have separate area accessibles for
>   // each image accessible.
>   nsAccessNodeHashtable *mAccessNodeCache;
>+
>+private:
>+  /**
>+   * Determine if this image accessible has a longdesc attribute.
>+   *
>+   * @returns  true if the longdesc attribute is present.
>+   */
>+  PRBool HasLongDesc();
>+  
>+  /**
>+   * Determine whether the given action index is within a valid range.
>+   * Used gy GetActionName and DoAction to prevent the index to go out of
>+   * bounds.

mispelling, gy -> by and description seems a bit different than what the method does (I mean my comment above) :)

next version?
Attachment #335372 - Flags: review?(surkov.alexander)
Attached patch Patch5 — — Splinter Review 
More comments addressed.
Attachment #335372 - Attachment is obsolete: true
Attachment #335378 - Flags: review?(surkov.alexander)
Comment on attachment 335378 [details] [diff] [review]
Patch5


>+  /**
>+   * Determine whether the given action index is valid for opening the longdesc
>+   * URL. Used by GetActionName and DoAction to ensure the index is valid.

it seems " to ensure the index is valid" is excess. I think just "Used by GetActionName and DoAction" is enough. Up to you.

>+    function testThis(aID, aName, aSRC, aWidth, aHeight,
>+                      aNumActions, aActionNames)
>     {

>+      if (aNumActions) {
>+        is(acc.numActions, aNumActions,
>+           "Wrong number of actions for " + aID + "!");
>+
>+        for (index = 0; index < aNumActions; index++) {
>+          is(acc.getActionName(index), aActionNames[index],
>+             "Wrong action name for " + aID + ", index " + index +"!");
>+        }
>+      }

you could use here aActionNames.length to simplify interface of function.

r=me, thank you for the patience :)
Attachment #335378 - Flags: review?(surkov.alexander) → review+
Pushed in changeset:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/0da0593988c3
with Surkov's final comments addressed.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: