343150 - Use AtkHyperlinkImpl

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

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>

Comment 1

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

Attachment: patch

patch without new added files

Comment 2

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

Attachment: .h file

Comment 3

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

Attachment: .cpp file

Comment 4

[Aaron Leventhal]

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.

Comment 5

[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.

Comment 6

[Ginn Chen]

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?

Comment 7

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

Attachment: patchv2

addressing ginn's comment

Comment 8

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

> Neo, do you have a method or python script to verify the patch?
> 

nope:(

it's not called from firefox side. add [e-mail] to see if he can make one

Comment 9

[Ginn Chen]

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

Comment 10

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

Attachment: patch

oops, strong coffee needed

Comment 11

[Aaron Leventhal]

Where are we at with this patch? It's probably the most important thing we need right now.

Comment 12

[Ginn Chen]

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

Comment 13

[Aaron Leventhal]

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.

Comment 14

[Ginn Chen]

at-spi doesn't have AtkHyperlinkImpl interface yet.
So these code can do nothing now.

Comment 15

[Aaron Leventhal]

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.

Comment 16

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

Attachment: patch

verified with at-poke at-spi atk link

Comment 17

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

Attachment: .h file

Comment 18

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

Attachment: .cpp file

Comment 19

[Ginn Chen]

patch backed out

It could not link on tinderbox machine.

undefined reference to `atk_hyperlink_impl_get_type'

Comment 20

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

Attachment: patch to remove compile error

Comment 21

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

Attachment: patch to remove compile error

Comment 22

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

Attachment: patch to remove compile error v2(variable name convension modify)

Comment 23

[Ginn Chen]

Attachment: patch to remove compile error v2

I changed a little bit.

Comment 24

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

Comment on attachment 236042 [details] [diff] [review]
patch to remove compile error v2

fine for me

Comment 25

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

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

Comment 26

[Aaron Leventhal]

Okay, file a bug. I think Ginn Chen will need to look at it, unless you think you can debug it.