343156 - New ATK: expose new action for images, to open long description (longdesc) in a new tab

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

Rather than just exposing the URL, or asynchronously trying to load the URL and exposing that via description, it's better just to open longdesc's in a new window and let the user browse the info. It could contain tables and forms, etc.

We should expose a new action on images which does this.

Comment 1

[Nian Liu(n/a in a long time)]

Attachment: patch

Comment 2

[Aaron Leventhal]

Comment on attachment 229443 [details] [diff] [review]
patch

I believe this means that images have no action #1, and no action #0 that does anyuthing unless there is an onlick associated with it. I think the enums belong in each accessible class that implement actions.

+    nsresult rv;
+    nsCOMPtr<nsIDOMHTMLImageElement> element(do_QueryInterface(mDOMNode, &rv));
+    NS_ENSURE_SUCCESS(rv, rv);
we don't bother with rv on QI's. We just check the resulting pointer.
so do:
nsCOMPtr<nsIDOMHTMLImageElement> element(do_QueryInterface(mDOMNode));
NS_ENSURE_TRUE(mDOMNode, NS_ERROR_FAILURE);
That will also output a warning if it fails. If it's normal that the QI could fail, then don't use that, instead do |if (mDOMNode) return NS_OK;|

You can keep the rv on method calls. However, if it should never fail, you can also consider using an assertion.

+    win->OpenDialog(longDesc, 
+                    NS_LITERAL_STRING(""),
+                    NS_LITERAL_STRING(""),
+                    nsnull, getter_AddRefs(tmp));
+    return NS_OK;

Maybe you just want to do:
return win->OpenDialog(...);

Comment 3

[Nian Liu(n/a in a long time)]

Attachment: patchv2

addressing Aaron's comment

Comment 4

[Aaron Leventhal]

Comment on attachment 229626 [details] [diff] [review]
patchv2

r=aaronlev, but could you move the enums for image actions into nsHTMLImageAccessible.h? It would also be great if the others were moved where they belong.

Comment 5

[[e-mail]]

Comment on attachment 229626 [details] [diff] [review]
patchv2

>+    nsCOMPtr<nsPIDOMWindow> piWindow = document->GetWindow();
>+    nsCOMPtr<nsIDOMWindowInternal> win(do_QueryInterface(piWindow));
>+    NS_ENSURE_TRUE(win, NS_ERROR_FAILURE);
>+    nsCOMPtr<nsIDOMWindow> tmp;
>+    return win->OpenDialog(longDesc, 
>+                           NS_LITERAL_STRING(""),
>+                           NS_LITERAL_STRING(""),
>+                           nsnull, getter_AddRefs(tmp));
I think you should write document->GetWindow()->Open(...) here but ask bz if you want to be really sure. sr=me with that fixed.

Aaron, are you allowed to use EmptyString() here?

Comment 6

[Aaron Leventhal]

Yes we use EmptyString() all the time.

Comment 7

[Nian Liu(n/a in a long time)]

nsCOMPtr<nsIDocument> document(do_QueryInterface(domDocumenti)); should be
nsCOMPtr<nsIDocument> document(do_QueryInterface(domDocument));

Comment 8

[Nian Liu(n/a in a long time)]

Attachment: final patch

patch addressing neil's comment and confirmed with bz

this patch is manually modified based on previous patch since i'm working on several bugs and cvs diff really messed all up.

ginn, pls. help me build it before check in

Comment 9

[Ginn Chen]

Comment on attachment 230073 [details] [diff] [review]
final patch

+    return win->Open(longDesc, NS_LITERAL_STRING(""), NS_LITERAL_STRING(""));

It doesn't compile.

Comment 10

[Nian Liu(n/a in a long time)]

Attachment: cpp file patch

oops, manually modified patch makes the mistake.

Comment 11

[Gérard Talbot]

Added longdesc in summary to help find this bug