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

RESOLVED FIXED in mozilla14

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sicking, Assigned: capella)

Tracking

7 Branch
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 14 obsolete attachments)

11.55 KB, patch
Details | Diff | Splinter Review
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

6 years ago
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() {}?

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All
(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

6 years ago
Jonas, do you have time to finish it in nearest future or should we take care about it for you?
Target Milestone: --- → mozilla6
Version: Trunk → 7 Branch

Updated

6 years ago
Target Milestone: mozilla6 → ---
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

6 years ago
(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.
(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.
Assignee: jonas → trev.saunders
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.
Attachment #528164 - Attachment is obsolete: true
Attachment #552905 - Flags: review?(surkov.alexander)

Comment 8

6 years ago
(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?
It's totally fine to cast nsIContent->nsGenericHTMLElement if you first check that IsHTML returns true.
(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

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

Comment 12

6 years ago
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.
Attachment #552905 - Flags: review?(surkov.alexander)
(Assignee)

Comment 13

5 years ago
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
Attachment #610383 - Flags: review?(surkov.alexander)
(Assignee)

Updated

5 years ago
Assignee: trev.saunders → markcapella

Comment 14

5 years ago
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
Attachment #610383 - Flags: review?(surkov.alexander) → review+

Comment 15

5 years ago
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?
(Assignee)

Comment 16

5 years ago
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

5 years ago
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
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 18

5 years ago
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

5 years ago
(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?
(Assignee)

Comment 20

5 years ago
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

5 years ago
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

5 years ago
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?
(Assignee)

Comment 23

5 years ago
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

5 years ago
(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

5 years ago
(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

5 years ago
(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

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

Comment 28

5 years ago
(In reply to steve faulkner from comment #27)
> I presume the link text will still be exposed as the accessible description?

yes

Comment 29

5 years ago
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

5 years ago
(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

5 years ago
(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

5 years ago
(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

5 years ago
So Trevor what was your idea you said on irc to make things nice?
(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.
(Assignee)

Comment 35

5 years ago
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

5 years ago
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
Attachment #611367 - Flags: review?(trev.saunders)
(Assignee)

Comment 37

5 years ago
Created attachment 611392 [details] [diff] [review]
Patch (v5)
Attachment #552905 - Attachment is obsolete: true
Attachment #610478 - Attachment is obsolete: true
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.
Attachment #611367 - Flags: review?(trev.saunders)
(Assignee)

Comment 39

5 years ago
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 ...
Attachment #611367 - Attachment is obsolete: true
Attachment #611392 - Attachment is obsolete: true
(Assignee)

Comment 40

5 years ago
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  :)
Attachment #611419 - Attachment is obsolete: true
Attachment #611435 - Flags: feedback?(trev.saunders)
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

5 years ago
use GetURIAttr version that returns nsIURI and transform LongDescElm to LongDescURI method.
(Assignee)

Comment 43

5 years ago
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.
Attachment #610383 - Attachment is obsolete: true
Attachment #611435 - Attachment is obsolete: true
Attachment #611435 - Flags: feedback?(trev.saunders)
(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

5 years ago
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();
(Assignee)

Comment 46

5 years ago
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.
Attachment #611968 - Attachment is obsolete: true

Comment 47

5 years ago
(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?
(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
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.
(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);
}
You mean like this:

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

:)
(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.
(Assignee)

Comment 53

5 years ago
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
Attachment #612181 - Attachment is obsolete: true
(Assignee)

Comment 54

5 years ago
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.
Attachment #612428 - Attachment is obsolete: true

Comment 55

5 years ago
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.
Attachment #612440 - Flags: review+

Updated

5 years ago
Attachment #612440 - Flags: review?(jonas)
(Assignee)

Comment 56

5 years ago
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.
Attachment #612440 - Attachment is obsolete: true
Attachment #612440 - Flags: review?(jonas)
(Assignee)

Updated

5 years ago
Attachment #612460 - Flags: review?(jonas)

Comment 57

5 years ago
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.
(Assignee)

Comment 58

5 years ago
Created attachment 612470 [details] [diff] [review]
Patch (v13) fixed nits

Fixed a few more nitz.

Updated

5 years ago
Attachment #612470 - Flags: review?(jonas)

Updated

5 years ago
Attachment #612460 - Attachment is obsolete: true
Attachment #612460 - Flags: review?(jonas)
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.
Attachment #612470 - Flags: review?(jonas) → review+

Comment 60

5 years ago
(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?
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

5 years ago
(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
(Assignee)

Comment 63

5 years ago
Created attachment 613761 [details] [diff] [review]
Patch (v14) null check added
Attachment #612470 - Attachment is obsolete: true

Comment 64

5 years ago
(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

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a46ebf1b9d5
https://hg.mozilla.org/mozilla-central/rev/1a46ebf1b9d5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14

Updated

5 years ago
Depends on: 745287
You need to log in before you can comment on or make changes to this bug.