Closed Bug 343156 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: nian.liu)

References

Details

(Keywords: access)

Attachments

(3 files, 1 obsolete file)

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.
Assignee: aaronleventhal → nian.liu
Attached patch patch (obsolete) — Splinter Review
Attachment #229443 - Flags: review?(aaronleventhal)
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(...);
Attachment #229443 - Flags: review?(aaronleventhal) → review-
Attached patch patchv2Splinter Review
addressing Aaron's comment
Attachment #229443 - Attachment is obsolete: true
Attachment #229626 - Flags: review?(aaronleventhal)
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.
Attachment #229626 - Flags: review?(aaronleventhal) → review+
Attachment #229626 - Flags: superreview?
Blocks: 345275
Attachment #229626 - Flags: superreview? → superreview?(neil)
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?
Attachment #229626 - Flags: superreview?(neil) → superreview+
Yes we use EmptyString() all the time.
nsCOMPtr<nsIDocument> document(do_QueryInterface(domDocumenti)); should be
nsCOMPtr<nsIDocument> document(do_QueryInterface(domDocument));
Attached patch final patchSplinter Review
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 on attachment 230073 [details] [diff] [review]
final patch

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

It doesn't compile.
Attachment #230073 - Flags: review-
Attached patch cpp file patchSplinter Review
oops, manually modified patch makes the mistake.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Added longdesc in summary to help find this bug
Summary: New ATK: expose new action for images, to open long description in a new tab → New ATK: expose new action for images, to open long description (longdesc) in a new tab
You need to log in before you can comment on or make changes to this bug.