652635 - Fallback missing @longdesc to aria-describedby pointing to <a href>

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: 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 (:asurkov)]

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] (NeedInfo me for attention)]

(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 (:asurkov)]

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]

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 (:asurkov)]

(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)]

(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)]

Attachment: 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 (:asurkov)]

(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]

It's totally fine to cast nsIContent->nsGenericHTMLElement if you first check that IsHTML returns true.

Comment 10

[Trevor Saunders (:tbsaunde)]

(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 (:asurkov)]

please add a test, we need to have one for existing and new behaviour

Comment 12

[alexander :surkov (:asurkov)]

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]]

Attachment: 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 (:asurkov)]

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 (:asurkov)]

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]]

Attachment: Patch (v3)

This avoids duplicate calls ... leaves the bool function as-is ... we could still rename it ...

Comment 17

[alexander :surkov (:asurkov)]

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]]

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 (:asurkov)]

(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]]

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 (:asurkov)]

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]

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]]

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 (:asurkov)]

(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 (:asurkov)]

(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]

(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]

I presume the link text will still be exposed as the accessible description?

Comment 28

[alexander :surkov (:asurkov)]

(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]

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]

(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 (:asurkov)]

(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 (:asurkov)]

(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 (:asurkov)]

So Trevor what was your idea you said on irc to make things nice?

Comment 34

[Trevor Saunders (:tbsaunde)]

(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]]

Attachment: 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 (:asurkov)]

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]]

Attachment: Patch (v5)

Comment 38

[Trevor Saunders (:tbsaunde)]

Comment on attachment 611392 [details] [diff] [review]
Patch (v5)

># HG changeset patch
># Parent 4a8a5e8ef78bdcf1d781294ae5bef76bfa530f88
># User Mark Capella <[e-mail]>
>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]]

Attachment: 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]]

Attachment: 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)]

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 (:asurkov)]

use GetURIAttr version that returns nsIURI and transform LongDescElm to LongDescURI method.

Comment 43

[Mark Capella [:capella]]

Attachment: 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)]

(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 (:asurkov)]

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]]

Attachment: 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 (:asurkov)]

(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)]

(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]

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)]

(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]

You mean like this:

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

:)

Comment 52

[Trevor Saunders (:tbsaunde)]

(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]]

Attachment: 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]]

Attachment: 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 (:asurkov)]

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]]

Attachment: Patch (v12)

Not a problem! Each of the changes in the above comment has been incorporated in this new patch.

Comment 57

[alexander :surkov (:asurkov)]

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]]

Attachment: Patch (v13) fixed nits

Fixed a few more nitz.

Comment 59

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 (:asurkov)]

(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]

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 (:asurkov)]

(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]]

Attachment: Patch (v14) null check added

Comment 64

[alexander :surkov (:asurkov)]

(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

Comment 65

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a46ebf1b9d5

Comment 66

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/1a46ebf1b9d5