Last Comment Bug 652635 - Fallback missing @longdesc to aria-describedby pointing to <a href>
: Fallback missing @longdesc to aria-describedby pointing to <a href>
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: 7 Branch
: All All
: -- normal (vote)
: mozilla14
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on: 745287
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-25 13:42 PDT by Jonas Sicking (:sicking) No longer reading bugmail consistently
Modified: 2012-04-13 12:09 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to fix (4.27 KB, patch)
2011-04-25 13:42 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
patch (8.38 KB, patch)
2011-08-13 14:29 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
Patch (v2) (8.76 KB, patch)
2012-03-28 18:23 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch (v3) (8.46 KB, patch)
2012-03-29 01:18 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v4) (9.05 KB, patch)
2012-04-01 23:46 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v5) (9.07 KB, patch)
2012-04-02 01:31 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v6) (9.39 KB, patch)
2012-04-02 04:29 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v7) (9.87 KB, patch)
2012-04-02 06:28 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v8) (9.81 KB, patch)
2012-04-03 13:55 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v9) (9.52 KB, patch)
2012-04-04 07:22 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v10) (11.31 KB, patch)
2012-04-04 18:40 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v11) (11.30 KB, patch)
2012-04-04 20:35 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch (v12) (11.40 KB, patch)
2012-04-04 23:01 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v13) fixed nits (11.52 KB, patch)
2012-04-05 00:53 PDT, Mark Capella [:capella]
jonas: review+
Details | Diff | Splinter Review
Patch (v14) null check added (11.55 KB, patch)
2012-04-10 14:11 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-04-25 13:42:57 PDT
Created attachment 528164 [details] [diff] [review]
Patch to fix

If an image doesn't have a @longdesc attribute, but does have a aria-describedby which points to a <a href>, we could give the user instant access to the link destination.

This should not affect our normal aria-describedby implementation which will still be available.

Even better would be to make aria-describedby read the full semantic contents of whatever elements it points to, but since I don't know how to do that I'll leave that for a later bug.

Definitely let me know if you think this is code bloat and if we should go for the better aria-describedby solution instead.
Comment 1 alexander :surkov 2011-04-25 20:58:57 PDT
Comment on attachment 528164 [details] [diff] [review]
Patch to fix

Review of attachment 528164 [details] [diff] [review]:

The idea makes sense to me, though I would like to hear back David.

::: accessible/src/html/nsHTMLImageAccessible.cpp
@@ +184,5 @@
+                            nsAccessibilityAtoms::aria_describedby) &&
+          (doc = mContent->GetCurrentDoc())) {
+        nsWhitespaceTokenizer tokens(describedby);
+        while (token.hasMoreTokens()) {
+          nsIContent* target = doc->GetElementById(token.nextToken());

I'd prefer to use IDRefsIterator like

IDRefsIterator iter(mContent, nsAccessibilityAtoms::aria_describedby);
nsIContent* target = null;
while ((target = iter.NextElm()) {
  if (target-IsHTML)
}

@@ +256,5 @@
 nsHTMLImageAccessible::HasLongDesc()
 {
   if (IsDefunct())
     return PR_FALSE;
 

Makes us run through targets twice. Would it better to transform this method to nsIContent* LongDescContent() {}?
Comment 2 David Bolter [:davidb] 2011-05-04 08:41:16 PDT
(In reply to comment #1)
> Comment on attachment 528164 [details] [diff] [review] [review]
> Patch to fix
> 
> Review of attachment 528164 [details] [diff] [review] [review]:
> 
> The idea makes sense to me, though I would like to hear back David.

Yep. I'd like to see this proceed.
Comment 3 alexander :surkov 2011-07-17 22:49:09 PDT
Jonas, do you have time to finish it in nearest future or should we take care about it for you?
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-07-17 22:57:48 PDT
I'd love it if someone could pick this up. I'm currently busy trying to clear out my standards obligations as well as getting started on XBL changes.
Comment 5 alexander :surkov 2011-07-17 23:00:33 PDT
(In reply to comment #4)
> I'd love it if someone could pick this up.

Ok, Trevor, could you take it please? It should be easy for you since you know this code pretty well.
Comment 6 Trevor Saunders (:tbsaunde) 2011-07-25 01:31:03 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > I'd love it if someone could pick this up.
> 
> Ok, Trevor, could you take it please? It should be easy for you since you
> know this code pretty well.

sure, I'll take this, but I'll probably finish up a couple of the big patches I'm currently doing first.
Comment 7 Trevor Saunders (:tbsaunde) 2011-08-13 14:29:59 PDT
Created attachment 552905 [details] [diff] [review]
patch

Jonas, mind taking a quick look at this? I had to change the local include paths alittle to make things build, and I'm trusting your first patch for the static cast nsIContent -> nsGenericHTMLElement being correct there.
Comment 8 alexander :surkov 2011-08-13 17:17:15 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> Created attachment 552905 [details] [diff] [review]
> patch
> 
> Jonas, mind taking a quick look at this? I had to change the local include
> paths alittle to make things build, and I'm trusting your first patch for
> the static cast nsIContent -> nsGenericHTMLElement being correct there.

I don't think this is correct, at least for xhtml documents where other namespace elements can be hosted and referred by aria-describedby. I think GetURIAttr() should be moved to nsIContent and implemented it by nsGenericHTMLElement. Jonas, does it sound good?
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-13 18:42:19 PDT
It's totally fine to cast nsIContent->nsGenericHTMLElement if you first check that IsHTML returns true.
Comment 10 Trevor Saunders (:tbsaunde) 2011-08-13 19:10:04 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> Created attachment 552905 [details] [diff] [review]
> patch

I also noticed that we don't seem to have any tests that doAction(indexOf("show_longdesc")) actually does anything useful, but I'm not sure this is the write bug to fix the tests there.

(In reply to Jonas Sicking (:sicking) from comment #9)
> It's totally fine to cast nsIContent->nsGenericHTMLElement if you first
> check that IsHTML returns true.

ok, then I'll make sure we add a check mContent is html before returning it in the case mContent has a longdesc attribute
Comment 11 alexander :surkov 2011-08-13 19:39:03 PDT
please add a test, we need to have one for existing and new behaviour
Comment 12 alexander :surkov 2011-08-16 02:35:51 PDT
Comment on attachment 552905 [details] [diff] [review]
patch

Review of attachment 552905 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/nsHTMLImageAccessible.cpp
@@ +163,5 @@
>  
> +  if (!IsValidLongDescIndex(aIndex))
> +    return nsLinkableAccessible::DoAction(aIndex);
> +
> +  //get the long description uri and open in a new window

nit: space after //, get -> Get, dot in the end

@@ +164,5 @@
> +  if (!IsValidLongDescIndex(aIndex))
> +    return nsLinkableAccessible::DoAction(aIndex);
> +
> +  //get the long description uri and open in a new window
> +  nsGenericHTMLElement* longDescContent = LongDescContent();

longDescContent -> longDescElm

@@ +166,5 @@
> +
> +  //get the long description uri and open in a new window
> +  nsGenericHTMLElement* longDescContent = LongDescContent();
> +  if (!longDescContent)
> +    return NS_ERROR_UNEXPECTED;

it never fails because of IsValidLongDescIndex check

@@ +173,5 @@
> +  longDescContent->GetURIAttr(nsAccessibilityAtoms::href, nsnull, longDesc);
> +  nsIDocument* document = mContent->GetOwnerDoc();
> +  nsCOMPtr<nsPIDOMWindow> piWindow = document->GetWindow();
> +  nsCOMPtr<nsIDOMWindow> win = do_QueryInterface(piWindow);
> +  NS_ENSURE_TRUE(win, NS_ERROR_FAILURE);

NS_ENSURE_STATE(win) works here, since it's unexpected

@@ +239,5 @@
> +      if ((target->IsHTML(nsAccessibilityAtoms::a) ||
> +           target->IsHTML(nsAccessibilityAtoms::area)) &&
> +          target->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::href))
> +        return static_cast<nsGenericHTMLElement*>(target);
> +    }

wrong indentation of "while" body

::: accessible/src/html/nsHTMLImageAccessible.h
@@ +73,5 @@
>    virtual PRUint8 ActionCount();
>  
>  private:
>    /**
> +   * return the content for the longdesc attribute if any.

Return the element defining longdesc URL?

@@ +78,2 @@
>     */
> +  nsGenericHTMLElement* LongDescContent();

rename to LongDescElement()
make it const

::: accessible/tests/mochitest/test_nsIAccessibleImage.html
@@ +197,5 @@
>    <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!');"/>

wrong indentation and don't see a reason to change it at all

@@ +201,5 @@
> +  alt="Another image of Mozilla logo" onclick="alert('Clicked!');"/>
> +  <br>image described by a link to be treated as longdesc<br>
> +  <img id="longdesc" src="moz.png" aria-describedby
> +  alt="Image of Mozilla logo"/>
> +  <a id="describing_link" href="longdesc_src.html">link to description of image</a>

I don't see any tests for this
actually I'd like to see tests in actions tests.
Comment 13 Mark Capella [:capella] 2012-03-28 18:23:17 PDT
Created attachment 610383 [details] [diff] [review]
Patch (v2)

Ok, I re-based the original patch (fixed up merge errors), hacked several small bugs, addressed nits and added a test to the .html
Comment 14 alexander :surkov 2012-03-28 20:58:56 PDT
Comment on attachment 610383 [details] [diff] [review]
Patch (v2)

Review of attachment 610383 [details] [diff] [review]:
-----------------------------------------------------------------

I'll fix nits before landing, thank you for finishing this

::: accessible/src/html/nsHTMLImageAccessible.h
@@ +89,5 @@
>     *
>     * @returns  true if index is valid for longdesc action.
>     */
>    bool IsValidLongDescIndex(PRUint8 aIndex);
> +

nit: new line

::: accessible/tests/mochitest/test_nsIAccessibleImage.html
@@ +131,5 @@
> +      // Image with long desc
> +      actionNamesArray = null;
> +      actionNamesArray = new Array("showlongdesc");
> +      testThis("longdesc2", "moz.png",
> +               89, 38, 1, actionNamesArray);

old way to test (it doesn't invoke an action) but it's ok

@@ +144,5 @@
>  <body>
>  
>    <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=429659">Mozilla Bug 429659</a>
> +  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=652635">
> +     fall back missing @longdesc to aria-describedby pointing to a href</a>

bug name should be under @title
Comment 15 alexander :surkov 2012-03-28 21:12:58 PDT
Comment on attachment 610383 [details] [diff] [review]
Patch (v2)

Review of attachment 610383 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/nsHTMLImageAccessible.cpp
@@ +166,3 @@
>  
> +  // Get the long description uri and open in a new window.
> +  nsGenericHTMLElement* longDescElm = LongDescElement();

you're getting it twice, maybe convert
bool IsValidLongDescIndex(aIndex) ->
nsGenericHTMLElement* LongDescElementIfIndex(aIndex)

can't think of good name. Mark, Trevor?
Comment 16 Mark Capella [:capella] 2012-03-29 01:18:56 PDT
Created attachment 610478 [details] [diff] [review]
Patch (v3)

This avoids duplicate calls ... leaves the bool function as-is ... we could still rename it ...
Comment 17 alexander :surkov 2012-03-29 01:46:47 PDT
Comment on attachment 610478 [details] [diff] [review]
Patch (v3)

Review of attachment 610478 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/nsHTMLImageAccessible.cpp
@@ +162,5 @@
>      return NS_ERROR_FAILURE;
>  
> +  nsGenericHTMLElement* longDescElm = LongDescElement();
> +  if (!longDescElm)
> +    return nsLinkableAccessible::DoAction(aIndex);

if there's long desc elm then you invoke long desc action always not depending on the given index
Comment 18 Mark Capella [:capella] 2012-03-30 06:27:43 PDT
Did we decide that this is indeed finished? The original code defaults to performing DoAction(aIndex) ... looking at routine nsLinkableAccessible::ActionCount() shows it only returns a 1 / 0 for index
Comment 19 alexander :surkov 2012-03-30 06:37:42 PDT
(In reply to Mark Capella [:capella] from comment #18)
> Did we decide that this is indeed finished? 

comment #17 is not addressed, right?

> The original code defaults to 
> performing DoAction(aIndex) ... looking at routine
> nsLinkableAccessible::ActionCount() shows it only returns a 1 / 0 for index

can you reword this please?
Comment 20 Mark Capella [:capella] 2012-03-30 07:27:55 PDT
In nsHTMLImageAccessible::DoAction, LongDescElement() returns either the longdesc attribute/value or the aria-describedby, or null ... if null we call DoAction and exit as it always has.

If it returns a longdesc nsGenericHTMLElement we behave the same as before.

If it finds a new aria-describedby nsGenericHTMLElement it behaves like its a longdesc.

I don't understand why "given index" is an issue ... there either is or isn't one for the particular nsGenericHTMLElement ... there's no "list" of possible actions to choose from afaict where we'd need to get one based on an index ... 

right?
Comment 21 alexander :surkov 2012-03-30 07:42:40 PDT
ok, let's we have
<img onclick="alert('hello')" longdesc="www.mozilla.org">
the image accessible should expose two actions: click (0 index) inherited from nsLinkableAccessible and showlongdesc (1 index) defined by nsHTMLImageAccessible.

so if your last patch applied and you invoke click action, i.e. doAction(0) then LongDescElement is not null and therefore you invoke showlongdesc action (1 index).
Comment 22 steve faulkner 2012-03-30 07:44:41 PDT
can the DoAction be made NOT to open in a new tab/window as it currently does for longdesc? As use cases for this include pointing to content within the same page.

Or can multiple actions be exposed?
Comment 23 Mark Capella [:capella] 2012-03-30 07:54:54 PDT
Surkov,

  <img id="clickAndLongdesc" src="moz.png" longdesc="longdesc_src.html"
       alt="Another image of Mozilla logo" onclick="alert('Clicked!');"/>

is the closest example in the test html ... if you click you get alert box "Clicked!"... 

   Maybe you and Steve understand something more here? I can tweak as you need, but might have to have it explained to me.
Comment 24 alexander :surkov 2012-03-30 08:07:35 PDT
(In reply to steve faulkner from comment #22)
> can the DoAction be made NOT to open in a new tab/window as it currently
> does for longdesc? As use cases for this include pointing to content within
> the same page.
> 
> Or can multiple actions be exposed?

Steve, could you please file a separate bug for that?
Comment 25 alexander :surkov 2012-03-30 08:10:19 PDT
(In reply to Mark Capella [:capella] from comment #23)
> Surkov,
> 
>   <img id="clickAndLongdesc" src="moz.png" longdesc="longdesc_src.html"
>        alt="Another image of Mozilla logo" onclick="alert('Clicked!');"/>
> 
> is the closest example in the test html ... if you click you get alert box
> "Clicked!"... 

Mark, I talk about accessible actions, see nsIAccessible::doAction method. You might want to try DOMi to invoke accessible actions (https://addons.mozilla.org/en-US/firefox/addon/dom-inspector-6622/)
Comment 26 steve faulkner 2012-03-30 08:14:44 PDT
(In reply to alexander :surkov from comment #24)
> (In reply to steve faulkner from comment #22)
> > can the DoAction be made NOT to open in a new tab/window as it currently
> > does for longdesc? As use cases for this include pointing to content within
> > the same page.
> > 
> > Or can multiple actions be exposed?
> 
> Steve, could you please file a separate bug for that?

done https://bugzilla.mozilla.org/show_bug.cgi?id=740812 added you and bolter to cc list
Comment 27 steve faulkner 2012-03-30 08:16:54 PDT
I presume the link text will still be exposed as the accessible description?
Comment 28 alexander :surkov 2012-03-30 08:18:27 PDT
(In reply to steve faulkner from comment #27)
> I presume the link text will still be exposed as the accessible description?

yes
Comment 29 Rich Schwerdtfeger 2012-03-30 11:06:12 PDT
I would not want aria-describedby to be overloaded to take a URL now that aria-describedby has been out for 6 years or more. We are working on a new proposal for an aria-describedat property that will take a URL. This is being discussed with Google and Mozilla accessibility people. Also, ARIA currently has NO composite data types like this.
Comment 30 steve faulkner 2012-03-30 11:14:53 PDT
(In reply to Rich Schwerdtfeger from comment #29)
> I would not want aria-describedby to be overloaded to take a URL now that
> aria-describedby has been out for 6 years or more. We are working on a new
> proposal for an aria-describedat property that will take a URL. This is
> being discussed with Google and Mozilla accessibility people. Also, ARIA
> currently has NO composite data types like this.

as I understand it its not that aria-describedby takes a URL its if it references a link:

<img aria-describedby="poot">
...
...

<a id="poot" href=">description</a>

will result in the acc description the link text content (as usual) and an accessible action being provided to open the link.
Comment 31 alexander :surkov 2012-03-30 20:59:14 PDT
(In reply to Rich Schwerdtfeger from comment #29)
> I would not want aria-describedby to be overloaded to take a URL now that
> aria-describedby has been out for 6 years or more.

Rich, we don't make aria-describedby to take a URL. But if it's referenced to a link then we expose it as longdesc (see http://lists.w3.org/Archives/Public/public-html/2011Apr/0515.html)

> We are working on a new
> proposal for an aria-describedat property that will take a URL. This is
> being discussed with Google and Mozilla accessibility people. Also, ARIA
> currently has NO composite data types like this.

That's fine. Once this spec item is ready then we implement it. Please let me know when we can do that.
Comment 32 alexander :surkov 2012-03-30 21:00:20 PDT
(In reply to steve faulkner from comment #30)
> as I understand it its not that aria-describedby takes a URL its if it
> references a link:
> will result in the acc description the link text content (as usual) and an
> accessible action being provided to open the link.

yes, that's right
Comment 33 alexander :surkov 2012-03-30 21:01:30 PDT
So Trevor what was your idea you said on irc to make things nice?
Comment 34 Trevor Saunders (:tbsaunde) 2012-03-30 21:26:32 PDT
(In reply to alexander :surkov from comment #33)
> So Trevor what was your idea you said on irc to make things nice?

I think it would work to change IvalidLongDescIndex() to be IsLongDescIdx() or something that doesn't check that there is actually a long desc, just that the index would be correct if there was one and LongDesc() would stay the same and not care about indices.

then in ActionCount() you can just call LongDesc() as you do know.
in ActionDescription() you need to call both IsLongDescIdx() and LongDesc() to be sure the index is right and there is a longdesc before returning "longdesc" as the description.
in DoAction() you also need to call both but actually use the result of LongDesc() not just make sure it is non-null.
Comment 35 Mark Capella [:capella] 2012-04-01 23:46:59 PDT
Created attachment 611367 [details] [diff] [review]
Patch (v4)

Incorporating Trevors' suggestions, I've gotten this far. I'm posting this for review while I examine the test page with DomInspector. (still learning how to interpret it).
Comment 36 alexander :surkov 2012-04-02 00:33:53 PDT
Comment on attachment 611367 [details] [diff] [review]
Patch (v4)

Review of attachment 611367 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/nsHTMLImageAccessible.cpp
@@ +148,5 @@
>    if (IsDefunct())
>      return NS_ERROR_FAILURE;
>  
> +  if (IsLongDescIdx(aIndex) &&
> +      LongDescElement()) {

put it on the same line

::: accessible/src/html/nsHTMLImageAccessible.h
@@ +88,5 @@
>     * @param aIndex  The 0-based index to be tested.
>     *
>     * @returns  true if index is valid for longdesc action.
>     */
> +  bool IsLongDescIdx(PRUint8 aIndex);

I would use Index instead of Idx
Comment 37 Mark Capella [:capella] 2012-04-02 01:31:27 PDT
Created attachment 611392 [details] [diff] [review]
Patch (v5)
Comment 38 Trevor Saunders (:tbsaunde) 2012-04-02 02:05:15 PDT
Comment on attachment 611392 [details] [diff] [review]
Patch (v5)

># HG changeset patch
># Parent 4a8a5e8ef78bdcf1d781294ae5bef76bfa530f88
># User Mark Capella <markcapella@twcny.rr.com>
>Bug 652635 - Fallback missing @longdesc to aria-describedby pointing to <a href>, r=surkov, f=tbsaunde
>
>diff --git a/accessible/src/html/Makefile.in b/accessible/src/html/Makefile.in
>--- a/accessible/src/html/Makefile.in
>+++ b/accessible/src/html/Makefile.in
>@@ -70,11 +70,13 @@ EXPORTS = \
> FORCE_STATIC_LIB = 1
> 
> include $(topsrcdir)/config/rules.mk
> 
> LOCAL_INCLUDES = \
>   -I$(srcdir)/../base \
>   -I$(srcdir)/../generic \
>   -I$(srcdir)/../xpcom \
>+  -I$(srcdir)/../../../content/base/src \
>+  -I$(srcdir)/../../../content/html/content/src \
>   -I$(srcdir)/../../../layout/generic \
>   -I$(srcdir)/../../../layout/xul/base/src \
>   $(NULL)
>diff --git a/accessible/src/html/nsHTMLImageAccessible.cpp b/accessible/src/html/nsHTMLImageAccessible.cpp
>--- a/accessible/src/html/nsHTMLImageAccessible.cpp
>+++ b/accessible/src/html/nsHTMLImageAccessible.cpp
>@@ -35,18 +35,20 @@
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsHTMLImageAccessible.h"
> 
> #include "nsAccUtils.h"
> #include "Role.h"
>+#include "AccIterator.h"
> #include "States.h"
> 
>+#include "nsGenericHTMLElement.h"
> #include "imgIContainer.h"
> #include "imgIRequest.h"

nsGenericHTMLElement.H SHOULD BE IN ALPHABETICAL ORDER.

>+      nsAutoString longDesc;
>+      longDescElm->GetURIAttr(nsGkAtoms::href, nsnull, longDesc);

longDesc is a really strange name for that.

>+  if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::longdesc))
>+    return static_cast<nsGenericHTMLElement*>(mContent.get());
I think you should use kNamespaceID_XHTML to be sure that cast is valid.  (it would be nice if some body added casting methods)

>   /**
>    * Used by GetActionName and DoAction to ensure the index for opening the
>    * longdesc URL is valid.
>    * It is always assumed that the highest possible index opens the longdesc.
>    *
>    * @param aIndex  The 0-based index to be tested.
>    *
>    * @returns  true if index is valid for longdesc action.

you might want to update this to clarify it doesn't check there is a longDesc.

>+  bool IsLongDescIndex(PRUint8 aIndex);

you might want to make it inline.
Comment 39 Mark Capella [:capella] 2012-04-02 04:29:29 PDT
Created attachment 611419 [details] [diff] [review]
Patch (v6)

Posting what addresses all nits up to but not including a correction for 
the wrong thing for situation <img href="#foo" longdesc="#bar"> which will
open what href points to not longdesc ...
Comment 40 Mark Capella [:capella] 2012-04-02 06:28:32 PDT
Created attachment 611435 [details] [diff] [review]
Patch (v7)

This is mostly theoretical. I'm sure there's better coding style to still be put in place, but I want to check and see if this is what you were thinking of ... ie: if I haven't gone off in left field  :)
Comment 41 Trevor Saunders (:tbsaunde) 2012-04-02 17:56:08 PDT
Well, this is correct, but that out bool argument is rather magical.

Some other ideas
 would be store a static string in the function, and return a nsDependantString pointing to it.
or something like bool LongDesc(nsAString& aURI = nsnull);
or maybe we can store a static string in the class? then maybe we can do bool LongDesc(nsString* aURI = sDummyStr);
Comment 42 alexander :surkov 2012-04-02 21:04:53 PDT
use GetURIAttr version that returns nsIURI and transform LongDescElm to LongDescURI method.
Comment 43 Mark Capella [:capella] 2012-04-03 13:55:16 PDT
Created attachment 611968 [details] [diff] [review]
Patch (v8)

I tweaked out the changes Alex asked for in the last comment, but didn't go any further with revisions along the line of where Trev and I were heading, as I noticed that the patch started failing mochitests at (v7) and the magical bool value changes.

Prior to that, testing was fine. (I can't remember why we were further engineering it). Maybe we can all get together on this and polish it off? I'm unaware of the priority to be placed here.
Comment 44 Trevor Saunders (:tbsaunde) 2012-04-03 17:03:04 PDT
(In reply to alexander :surkov from comment #42)
> use GetURIAttr version that returns nsIURI and transform LongDescElm to
> LongDescURI method.

I like that idea, allthough you'll have to work out what to do once you have the nsIURI.
Comment 45 alexander :surkov 2012-04-03 21:18:41 PDT
Comment on attachment 611968 [details] [diff] [review]
Patch (v8)

Review of attachment 611968 [details] [diff] [review]:
-----------------------------------------------------------------

I put the idea into comments through the code. Can you list failing test here please?

::: accessible/src/html/nsHTMLImageAccessible.cpp
@@ +137,5 @@
>  nsHTMLImageAccessible::ActionCount()
>  {
>    PRUint8 actionCount = nsLinkableAccessible::ActionCount();
> +  bool tmp;
> +  return LongDescURI(&tmp) ? actionCount + 1 : actionCount;

add inline
bool HasLongDesc() const
{
  nsCOMPtr<nsIURI> uri = GetLondDescURI();
  return uri;
}

@@ +149,5 @@
>    if (IsDefunct())
>      return NS_ERROR_FAILURE;
>  
> +  bool tmp;
> +  if (IsLongDescIndex(aIndex) && LongDescURI(&tmp)) {

if (IsLongDescIndex(aIndex) && HasLongDesc()) {

@@ +169,3 @@
>  
> +  bool longDescType;
> +  nsGenericHTMLElement* longDescElm = LongDescURI(&longDescType);

nsCOMPtr<nsIURI> uri = GetLondDescURI();

@@ +169,5 @@
>  
> +  bool longDescType;
> +  nsGenericHTMLElement* longDescElm = LongDescURI(&longDescType);
> +  if (!longDescElm)
> +    return nsLinkableAccessible::DoAction(aIndex);

if (!uri)
  return NS_ERROR_INVALID_ARG;

@@ +186,4 @@
>    }
> +
> +  return win->Open(nsIURI, EmptyString(), EmptyString(),
> +                   getter_AddRefs(tmp));

nsCAutoString utf8spec;
uri->GetSpec(utf8spec);
NS_ConvertUTF8toUTF16 spec(utf8spec)

return win->Open(spec, ...);

@@ +232,5 @@
>  ////////////////////////////////////////////////////////////////////////////////
>  // Private methods
>  
> +nsGenericHTMLElement*
> +nsHTMLImageAccessible::LongDescURI(bool* aBool)

already_AddRefed<nsIURI>
nsHTMLImageAccessible::GetLongDescURI() const

@@ +237,4 @@
>  {
> +  if (mContent->HasAttr(kNameSpaceID_XHTML, nsGkAtoms::longdesc)) {
> +    *aBool = false;
> +    return static_cast<nsGenericHTMLElement*>(mContent.get());

nsGenericHTMLElement* element =
  static_cast<nsGenericHTMLElement*>(mContent.get());

nsCOMPtr<nsIURI> uri;
element->GetURIAttr(nsGkAtoms::longdesc, nsnull, getter_AddRefs(uri);
return uri.forget();

@@ +247,5 @@
> +    while (target = iter.NextElem()) {
> +      if ((target->IsHTML(nsGkAtoms::a) || target->IsHTML(nsGkAtoms::area)) &&
> +          target->HasAttr(kNameSpaceID_None, nsGkAtoms::href)) {
> +        *aBool = true;
> +        return static_cast<nsGenericHTMLElement*>(target);

nsGenericHTMLElement* element =
  static_cast<nsGenericHTMLElement*>(target);

nsCOMPtr<nsIURI> uri;
element->GetURIAttr(nsGkAtoms::href, nsnull, getter_AddRefs(uri);
return uri.forget();
Comment 46 Mark Capella [:capella] 2012-04-04 07:22:47 PDT
Created attachment 612181 [details] [diff] [review]
Patch (v9)

I put Alexs' changes in place, but I may have done something wrong here, as moving the two element->GetURIAttr calls down to the private function are both generating protection error:

'nsGenericHTMLElement::GetURIAttr' : cannot access protected member declared in class 'nsGenericHTMLElement'

My guess is it's because the nsCOMPtr<nsIURI> uri; is declared local in the function, but being new to c++ classes I'm stuck here.
Comment 47 alexander :surkov 2012-04-04 08:18:05 PDT
(In reply to Mark Capella [:capella] from comment #46)
> 'nsGenericHTMLElement::GetURIAttr' : cannot access protected member declared
> in class 'nsGenericHTMLElement'

because it's really protected (http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsGenericHTMLElement.h#726).

Olli, can we make this method public?
Comment 48 Trevor Saunders (:tbsaunde) 2012-04-04 11:50:48 PDT
(In reply to alexander :surkov from comment #47)
> (In reply to Mark Capella [:capella] from comment #46)
> > 'nsGenericHTMLElement::GetURIAttr' : cannot access protected member declared
> > in class 'nsGenericHTMLElement'
> 
> because it's really protected
> (http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/
> nsGenericHTMLElement.h#726).
> 
> Olli, can we make this method public?

or Jonas do you know? since you might be around sooner
Comment 49 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-04-04 14:16:32 PDT
Yes, we can definitely make that method public as long as we keep it on nsGenericHTMLElement or even nsGenericElement. I don't want to put it on any interfaces though.
Comment 50 Trevor Saunders (:tbsaunde) 2012-04-04 14:38:37 PDT
(In reply to Jonas Sicking (:sicking) from comment #49)
> Yes, we can definitely make that method public as long as we keep it on
> nsGenericHTMLElement or even nsGenericElement. I don't want to put it on any
> interfaces though.

sure, I'm not particularly interested in using xpcom interfaces to inspect the dom when we don't have to.  Though it would be nice to add a inline function to do the cast nsIContent -> nsGenericHTMLElement when valid, something on the order of

nsGenericHTMLElement*
AsHTMLElement()
{
  NS_ASSERTION(IsHTML(), "trying to convert non html content to nsGenericHTMLElement");
  return static_cast<nsGenericHTMLElement*>(this);
}
Comment 51 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-04-04 15:34:59 PDT
You mean like this:

http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsGenericHTMLElement.h#85

:)
Comment 52 Trevor Saunders (:tbsaunde) 2012-04-04 16:46:29 PDT
(In reply to Jonas Sicking (:sicking) from comment #51)
> You mean like this:
> 
> http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/
> nsGenericHTMLElement.h#85

nice! Mark please use.
Comment 53 Mark Capella [:capella] 2012-04-04 18:40:05 PDT
Created attachment 612428 [details] [diff] [review]
Patch (v10)

Alex, in response to comment45, after making your code suggestions, and making the GetURIAttr member public suggestion from comment47, the code patch (see attached) still fails two tests as listed here:

failed | Wrong number of actions for longdesc! - got 0, expected 1
failed | an unexpected uncaught JS exception reported through window.onerror - uncaught exception: [Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIAccessible.getActionName]" nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)" location: "JS frame :: chrome://mochitests/content/a11y/accessible/test_nsIAccessibleImage.html :: testThis :: line 101" data: no] at :0
Comment 54 Mark Capella [:capella] 2012-04-04 20:35:39 PDT
Created attachment 612440 [details] [diff] [review]
Patch (v11)

Ok, one of the suggestions made back in comment38 seems to have introduced the error described in the above. Backing it out has us passing all tests again, and wondering if this is now ready to close.
Comment 55 alexander :surkov 2012-04-04 21:29:49 PDT
Comment on attachment 612440 [details] [diff] [review]
Patch (v11)

Review of attachment 612440 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, I like this version.

::: accessible/src/html/nsHTMLImageAccessible.cpp
@@ +172,5 @@
> +  nsIDocument* document = mContent->OwnerDoc();
> +  nsCOMPtr<nsPIDOMWindow> piWindow = document->GetWindow();
> +  nsCOMPtr<nsIDOMWindow> win = do_QueryInterface(piWindow);
> +  NS_ENSURE_STATE(win);
> +  nsCOMPtr<nsIDOMWindow> tmp;

nit: move this tmp variable to Open method call (keep related things together)

@@ +238,5 @@
> +nsHTMLImageAccessible::GetLongDescURI()
> +{
> +  if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::longdesc)) {
> +    nsGenericHTMLElement* element = 
> +      static_cast<nsGenericHTMLElement*>(mContent.get());

do:
nsGenericHTMLElement* element = nsGenericHTMLElement::FromContent(mContent)

@@ +253,5 @@
> +    while (target = iter.NextElem()) {
> +      if ((target->IsHTML(nsGkAtoms::a) || target->IsHTML(nsGkAtoms::area)) &&
> +          target->HasAttr(kNameSpaceID_None, nsGkAtoms::href)) {
> +        nsGenericHTMLElement* element =
> +          static_cast<nsGenericHTMLElement*>(target);

same

::: accessible/src/html/nsHTMLImageAccessible.h
@@ +80,2 @@
>     */
> +  inline bool HasLongDesc();

you don't need to have inline keyword and please put method body here

@@ +80,5 @@
>     */
> +  inline bool HasLongDesc();
> +
> +  /**
> +   * Return the element defining longdesc URI.

Return an URI for showlongdesc action if any perhaps?

::: content/html/content/src/nsGenericHTMLElement.h
@@ +542,5 @@
> +   * nsIURI in the out param.
> +   *
> +   * @return true if we had the attr, false otherwise.
> +   */
> +  bool GetURIAttr(nsIAtom* aAttr, nsIAtom* aBaseAttr, nsIURI** aURI) const;

I'd say it makes sense to keep it together with other version of GetURIAttr. But up to your content reviewer.
Comment 56 Mark Capella [:capella] 2012-04-04 23:01:26 PDT
Created attachment 612460 [details] [diff] [review]
Patch (v12)

Not a problem! Each of the changes in the above comment has been incorporated in this new patch.
Comment 57 alexander :surkov 2012-04-04 23:18:17 PDT
Comment on attachment 612460 [details] [diff] [review]
Patch (v12)

Review of attachment 612460 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/nsHTMLImageAccessible.cpp
@@ +232,4 @@
>  {
> +  if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::longdesc)) {
> +    nsGenericHTMLElement* element = 
> +      nsGenericHTMLElement::FromContent(mContent.get());

I don't think you need get()

::: accessible/src/html/nsHTMLImageAccessible.h
@@ +86,5 @@
> +
> +  /**
> +   * Return an URI for showlongdesc action if any.
> +   */
> +  already_AddRefed<nsIURI> GetLongDescURI();

curious if you can keep these const?

::: content/html/content/src/nsGenericHTMLElement.h
@@ +526,5 @@
>    NS_HIDDEN_(nsresult) GetURIAttr(nsIAtom* aAttr, nsIAtom* aBaseAttr, nsAString& aResult);
>  
>    /**
> +   * Helper for GetURIAttr and GetHrefURIForAnchors which returns an
> +   * nsIURI in the out param.

btw, it's not a helper anymore. It makes to copy description from another verions of GetURIAttr() like:

* Gets the absolute URI value of an attribute, by resolving any relative
* URIs in the attribute against the baseuri of the element. If the attribute
* isn't a relative URI the value of the attribute is returned as is. Only
* works for attributes in null namespace.
Comment 58 Mark Capella [:capella] 2012-04-05 00:53:34 PDT
Created attachment 612470 [details] [diff] [review]
Patch (v13) fixed nits

Fixed a few more nitz.
Comment 59 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-04-09 10:47:14 PDT
Comment on attachment 612470 [details] [diff] [review]
Patch (v13) fixed nits

Review of attachment 612470 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/nsHTMLImageAccessible.cpp
@@ +232,4 @@
>  {
> +  if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::longdesc)) {
> +    nsGenericHTMLElement* element = 
> +      nsGenericHTMLElement::FromContent(mContent);

Note that this can return null if mContent is a non-HTML element. Are you sure that can't happen here? I.e. are you sure this can't be an <svg:image> for example?

@@ +236,3 @@
>  
> +    nsCOMPtr<nsIURI> uri;
> +    element->GetURIAttr(nsGkAtoms::longdesc, nsnull, getter_AddRefs(uri));

Would it be worth checking if longdesc contains any spaces? My understanding is that more often than not people misunderstand this attribute and put the actual description in the attribute, rather than a uri to it. Properly formatted URIs can't ever contain URIs in them, but I think that our URI parser will by default turn spaces into %20s which means that if a longdesc contains a description rather than a url, the user will be directed to a very strange url which surely will 404.
Comment 60 alexander :surkov 2012-04-09 20:00:28 PDT
(In reply to Jonas Sicking (:sicking) from comment #59)

> > +  if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::longdesc)) {
> > +    nsGenericHTMLElement* element = 
> > +      nsGenericHTMLElement::FromContent(mContent);
> 
> Note that this can return null if mContent is a non-HTML element. Are you
> sure that can't happen here? I.e. are you sure this can't be an <svg:image>
> for example?

nsHTMLImageAccessible is created by nsImageFrame (http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsImageFrame.cpp#202). If svg:image uses nsImageFrame then yes.

> @@ +236,3 @@
> >  
> > +    nsCOMPtr<nsIURI> uri;
> > +    element->GetURIAttr(nsGkAtoms::longdesc, nsnull, getter_AddRefs(uri));
> 
> Would it be worth checking if longdesc contains any spaces? My understanding
> is that more often than not people misunderstand this attribute and put the
> actual description in the attribute, rather than a uri to it. Properly
> formatted URIs can't ever contain URIs in them, but I think that our URI
> parser will by default turn spaces into %20s which means that if a longdesc
> contains a description rather than a url, the user will be directed to a
> very strange url which surely will 404.

Shouldn't it be a part nsGenericHTMLElement::GetURIAttr? I guess there's no better(good) way to say this string is url or not, right?
Comment 61 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-04-10 08:50:15 PDT
Im not sure when we use specific frame types. But I would expect that to change over time. So sounds like a null check is a good idea.

We can't change GetURIAttr because having spaces in Uris is something people actually depend on. I.e. there is markup out there like <a href="a page.html"> or <img src="my pic.jpg"> which needs to keep working. (again, we percent encode them).

So I was suggesting using a heuristic here since there is a lot of content which doesn't have uris in longdesc. But there is a risk that it would break contet like <img longdesc="my description.html">, so I'll leave that decision to you. My r+ stands either way.
Comment 62 alexander :surkov 2012-04-10 08:52:33 PDT
(In reply to Jonas Sicking (:sicking) from comment #61)
> Im not sure when we use specific frame types. But I would expect that to
> change over time. So sounds like a null check is a good idea.

Mark, please add null check.

> We can't change GetURIAttr because having spaces in Uris is something people
> actually depend on. I.e. there is markup out there like <a href="a
> page.html"> or <img src="my pic.jpg"> which needs to keep working. (again,
> we percent encode them).
> 
> So I was suggesting using a heuristic here since there is a lot of content
> which doesn't have uris in longdesc. But there is a risk that it would break
> contet like <img longdesc="my description.html">, so I'll leave that
> decision to you. My r+ stands either way.

and file a bug on this issue
Comment 63 Mark Capella [:capella] 2012-04-10 14:11:53 PDT
Created attachment 613761 [details] [diff] [review]
Patch (v14) null check added
Comment 64 alexander :surkov 2012-04-10 21:30:34 PDT
(In reply to alexander :surkov from comment #62)
> > We can't change GetURIAttr because having spaces in Uris is something people
> > actually depend on. I.e. there is markup out there like <a href="a
> > page.html"> or <img src="my pic.jpg"> which needs to keep working. (again,
> > we percent encode them).
> > 
> > So I was suggesting using a heuristic here since there is a lot of content
> > which doesn't have uris in longdesc. But there is a risk that it would break
> > contet like <img longdesc="my description.html">, so I'll leave that
> > decision to you. My r+ stands either way.
> 
> and file a bug on this issue

fyi bug 744144

Note You need to log in before you can comment on or make changes to this bug.