Closed Bug 343150 Opened 14 years ago Closed 14 years ago

Use AtkHyperlinkImpl

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: nian.liu)

References

Details

(Keywords: access, Whiteboard: testcase-needed)

Attachments

(3 files, 9 obsolete files)

This is the Firefox portion of this Gnome bug:
http://bugzilla.gnome.org/show_bug.cgi?id=344284

There is a new interfaces to help us deal with the fact that links are not interfaces in ATK, but objects. In Firefox we just treat it like another interface.

Take a look at this include file:
#include <atk/atkhyperlinkimpl.h>
Status: NEW → ASSIGNED
Assignee: ginn.chen → nian.liu
Status: ASSIGNED → NEW
Attached patch patch (obsolete) — Splinter Review
patch without new added files
Attachment #230092 - Flags: review?(aaronleventhal)
Attached patch .h file (obsolete) — Splinter Review
Attachment #230093 - Flags: review?(aaronleventhal)
Attached patch .cpp file (obsolete) — Splinter Review
Attachment #230094 - Flags: review?(aaronleventhal)
Attachment #230092 - Flags: review?(aaronleventhal) → review?(ginn.chen)
Attachment #230093 - Flags: review?(aaronleventhal) → review?(ginn.chen)
Comment on attachment 230094 [details] [diff] [review]
.cpp file

I'm traveling and won't be available to review for a few days. Anyway, Ginn understands that ATK/AT-SPI architecture better than I.
Attachment #230094 - Flags: review?(aaronleventhal) → review?(ginn.chen)
Comment on attachment 230092 [details] [diff] [review]
patch

+    if (accessInterfaceHyperlink) {
+       interfacesBits |= 1 << MAI_INTERFACE_HYPERLINK;
+    }
     //nsIAccessibleTable

Nit: add a blank line after }

   nsMaiInterfaceTable.cpp \
+  nsMaiInterfaceHyperlinkImpl.cpp \
   $(NULL)
we need ATK version check here.
And we need to make sure it can work well with low version ATK.
Attachment #230092 - Flags: review?(ginn.chen) → review-
Comment on attachment 230094 [details] [diff] [review]
.cpp file

    nsresult rv;

not used.

Neo, do you have a method or python script to verify the patch?
Attachment #230094 - Flags: review?(ginn.chen) → review-
Attached patch patchv2 (obsolete) — Splinter Review
addressing ginn's comment
Attachment #230092 - Attachment is obsolete: true
Attachment #230398 - Flags: review?(ginn.chen)
> Neo, do you have a method or python script to verify the patch?
> 

nope:(

it's not called from firefox side. add bill.haneman@sun.com to see if he can make one
Comment on attachment 230398 [details] [diff] [review]
patchv2

1) it should be ATK 1.12.1
2) it won't compile with ATK below 1.12.1
because you uses ATK_TYPE_HYPERLINK_IMPL in nsAccessibleWrap.cpp
Attachment #230398 - Flags: review?(ginn.chen) → review-
Attached patch patch (obsolete) — Splinter Review
oops, strong coffee needed
Attachment #230398 - Attachment is obsolete: true
Attachment #230402 - Flags: review?(ginn.chen)
Whiteboard: testcase-needed
Where are we at with this patch? It's probably the most important thing we need right now.
(In reply to comment #11)
> Where are we at with this patch? It's probably the most important thing we need
> right now.
> 

I would like to have a unit test to make sure it really works before checking in.
Ginn, would it help with testing to get this in? At least that way it won't bitrot. We can always fix it or totally remove it if it ends up being wrong in tests. It is trunk, and as long as we don't break anything too bad I think we should check it in if the code looks right.
at-spi doesn't have AtkHyperlinkImpl interface yet.
So these code can do nothing now.
Now that we're building with recent ATK headers, we should have an easier time testing this, correct? It seems like we should get this in soon, because it is blocking more testing by screen reader developers.

The AT-SPI Hyperlink interface is returning incorrect info for startIndex, endIndex and crashing for getURI. We think this will probably fix that.
Attached patch patch (obsolete) — Splinter Review
verified with at-poke at-spi atk link
Attachment #230093 - Attachment is obsolete: true
Attachment #230094 - Attachment is obsolete: true
Attachment #230402 - Attachment is obsolete: true
Attachment #235702 - Flags: review?(ginn.chen)
Attachment #230093 - Flags: review?(ginn.chen)
Attachment #230402 - Flags: review?(ginn.chen)
Attached patch .h fileSplinter Review
Attachment #235703 - Flags: review?(ginn.chen)
Attached patch .cpp fileSplinter Review
Attachment #235705 - Flags: review?(ginn.chen)
Attachment #235702 - Flags: review?(ginn.chen) → review+
Attachment #235703 - Flags: review?(ginn.chen) → review+
Attachment #235705 - Flags: review?(ginn.chen) → review+
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
patch backed out

It could not link on tinderbox machine.

undefined reference to `atk_hyperlink_impl_get_type'
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch to remove compile error (obsolete) — Splinter Review
Attachment #235729 - Flags: review?(ginn.chen)
Attachment #235729 - Attachment is obsolete: true
Attachment #235729 - Flags: review?(ginn.chen)
Attached patch patch to remove compile error (obsolete) — Splinter Review
Attachment #235702 - Attachment is obsolete: true
Attachment #235730 - Flags: review?(ginn.chen)
Attachment #235730 - Attachment is obsolete: true
Attachment #235853 - Flags: review?(ginn.chen)
Attachment #235730 - Flags: review?(ginn.chen)
I changed a little bit.
Attachment #235853 - Attachment is obsolete: true
Attachment #236042 - Flags: review?(nian.liu)
Attachment #235853 - Flags: review?(ginn.chen)
Comment on attachment 236042 [details] [diff] [review]
patch to remove compile error v2

fine for me
Attachment #236042 - Flags: review?(nian.liu) → review+
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
(In reply to comment #15)
> Now that we're building with recent ATK headers, we should have an easier time
> testing this, correct? It seems like we should get this in soon, because it is
> blocking more testing by screen reader developers.
> 
> The AT-SPI Hyperlink interface is returning incorrect info for startIndex,
> endIndex and crashing for getURI. We think this will probably fix that.
> 

Aaron, this patch does no help on above issue. 
Okay, file a bug. I think Ginn Chen will need to look at it, unless you think you can debug it.
You need to log in before you can comment on or make changes to this bug.