Bug 33785 – [Gtk] AtkHyperlink needs to be implemented

• Component: Accessibility
• Status: RESOLVED FIXED

[Joanmarie Diggs]

WebKitGtk does not implement AtkHyperlink (http://library.gnome.org/devel/atk/unstable/AtkHyperlink.html)

Impact: ATs cannot identify anything about a link other than the fact that it is a link.

[Joanmarie Diggs]

Sorry to be spammy. It's 3 AM....

This is especially true and needed for image map links. 

What's mainly missing (and needed) w.r.t. text links is the associated URI (We know the state, etc. of the link). We should, however, also implement the start and end indices at the very least....

[Diego Escalante Urrelo]

Hi Joanie, I was wondering how precisely to do this. So far I can see that different AtkInterfaces are implemented/enabled depending on the role detected on object creation time. But I wonder what to do with the AtkHyperlink *object*. I'm a bit clueless of where or what other objects -not interfaces- are implemented or where would I fit this.
I'm wondering if it should be in accesibility/ as a new file or if it's something only relevant for gtk/.

I'm discovering a new world.

[Joanmarie Diggs]

Hi Diego.

I'm afraid I didn't leave much detail. I just assumed I was doing this one. It would be nice to have edit bug privs so that we could assign bugs to ourselves.

[Joanmarie Diggs]

Created attachment 66519 [details]
test case

(Apologies for just now getting back to this.)

This may need to be split this into multiple bugs, but I think having a test case in one location might help. So....

Steps to reproduce:

1. Open the attached test case and Accerciser.

2. Within Accerciser's object tree, locate the paragraph in the test case. With that paragraph selected:

Expected results from Accerciser's iPython console:

In [1]: ht = acc.queryHypertext()
In [2]: nLinks = ht.getNLinks()
In [3]: links = [ht.getLink(n) for n in range(nLinks)]
In [4]: nLinks
Out[4]: 2
In [5]: links
Out[5]: 
[<CORBA.Object 'IDL:Accessibility/Hyperlink:1.0' at 0x96686e8>,
 <CORBA.Object 'IDL:Accessibility/Hyperlink:1.0' at 0x96a43d0>]
In [6]: [(l.startIndex, l.endIndex, l.getURI(0)) for l in links]
Out[6]: [(10, 14, 'http://www.google.com'), (25, 36, 'http://live.gnome.org/')]

In terms of Orca + Epiphany and Yelp 3.0, the above is what I must have. Having the rest of the implementation of AtkHypertext [1] and AtkHyperlink [2] is strongly desired, but much lower priority -- at least as far as I am concerned.

Thanks!

[1] http://library.gnome.org/devel/atk/unstable/AtkHypertext.html
[2] http://library.gnome.org/devel/atk/unstable/AtkHyperlink.html

[Mario Sanchez Prada]

(In reply to comment #2)
> Hi Joanie, I was wondering how precisely to do this. So far I can see that different 
> AtkInterfaces are implemented/enabled depending on the role detected on object
> creation time. But I wonder what to do with the AtkHyperlink *object*. 

Same doubts here. As long as I could figure it out so far, it looks to me that a new class would be needed to be implemented as a subclass of AtkHyperLink, which would hold references to WebKitAccessible objects implementing the AtkHyperlinkImpl *interface*, so calling to the methods like atk_hyperlink_get_uri(int index) would actually return the desired value.

> I'm a bit clueless of where or what other objects -not interfaces- are implemented 
> or where would I fit this.

If possible, I think the way to go here would be to somehow implement it inside the wrapper file as well, or in another one inside the gtk/ directory, but that would "break" the common layout for all the ports (platform specific versions of AXObjectCache, AccessibilityObject and AccessibilityObjectWrapper only).

> I'm wondering if it should be in accesibility/ as a new file or if it's something only
> relevant for gtk/.

I don't see anything in cross-platform code similar to the structure we need in here (HyperText - HyperLink - link) so I guess going for gtk specific code would be the way to go at this moment.
 
> I'm discovering a new world.

Me too :-O

[Mario Sanchez Prada]

Just sharing a link that I think could be helpful for this stuff:

http://www.mozilla.org/access/unix/new-atk.html

[Mario Sanchez Prada]

(In reply to comment #6)
> Just sharing a link that I think could be helpful for this stuff:
> 
> http://www.mozilla.org/access/unix/new-atk.html

Yet another one:
http://accessibility.linuxfoundation.org/~gk4/a11y/ddc06/guide/atkguideddc06.html

[Mario Sanchez Prada]

Created attachment 69480 [details]
WIP: Initial version of the patch

After working for these past days on this stuff and trying to figure out how all this stuff works I currently have an initial version of the patch that could one day become the fix for this bug, not only for the use cases mentioned by Joanmarie as "a must" but perhaps even also for the rest of the needs derived of the implementation of this AtkHypetext/AtkHyperlink thing.

Hence, attaching the patch to allow others check it, propose ideas, changes, corrections... and *try it out*, so I could get some feedback that most likely could lead me to change it everything, but that's how software is made, isn't it? :-)

Brief description of the patch, as written in the commit log message:


    Implementing the AtkHypertext/AtkHyperlink stuff in the GTK port
    
    Done:
    
     - Implemented AtkHypertext and AtkHyperlinkImpl interfaces in the ATK
       accessibility wrapper (WebKitAccessible).
    
     - Added new GObject in accessibility/gtk to implement instances of the
       AtkHyperlink abstract class. Called it WebKitAccessibleHyperlink.
    
     - Added a basic method to provide a lazy mechanism to 'get or create'
       instances of WebKitAccessibleHyperlink for a given AtkObject
       implementing the AtkHyperlinkImpl interface. It's basic, but works
       fine (just a global hashtable and proper methods to deal with it).
    
     - Implemented all the functions of the AtkHypertext interface and the
       AtkHyperlink abstract class *but* considering only one anchor per
       instance of the AtkHyperlink class for the time being. It should be
       enough at this moment but will need changes for sure if we want to
       support image maps the same way firefox does it, for instance.
    
     - AtkAction interface not implemented in the WebKitAccessibleHyperlink
       class yet (it should be possible) Pending to check how to do it.
    
    TODO:
    
     - Check how to allow more than one anchor per AtkHyperlink
    
     - Check how to implement AtkAction interface in WebKitAccessibleHyperlink
    
     - Implement a LayoutTest and/or unit tests to check all this new code
    
     - Improve the code :-)


Thanks!

[Mario Sanchez Prada]

Created attachment 70236 [details]
WIP: Patch proposal (unit test pending)

WebKit/gtk/ChangeLogUploading  a new patch with some new stuff from the previous TODO list done.

(In reply to comment #8)
> [...]
>     TODO:
>      - Check how to allow more than one anchor per AtkHyperlink

I talked to Joanie and it seems that, in terms of the screen reader, just having one anchor per AtkHyperlink is enough and correct so, I didn't change this by now as I think the right thing to do would be to ship this patch with this limitation/feature in and, if needed, improve it later on in this aspect as a new bug or something.

>      - Check how to implement AtkAction interface in WebKitAccessibleHyperlink

Implemented AtkAction interface for those WebKitAccessibleHyperlink's where the wrapped AtkObject implements the AtkAction interface, that is, all of them :-) (but a check was added)

>      - Implement a LayoutTest and/or unit tests to check all this new code

Written an unit test to check all the new stuff coming from the AtkHypertext/AtkHyperlink thing. Still pending to add some code to testatk.c to check the correctness of the AtkAction implementation (couldn't do it this week since my devel environment is kind of broken and the unit tests are not working properly now).

>      - Improve the code :-)

Fixed some things here and there, such as how it's made the decision of implement the AtkHyperlinkImpl interface for a AtkObject or how to deal with the special case of list items.

*Not* asking for review yet. Still need to finish some things, such as the unit test for the AtkAction interface

[Mario Sanchez Prada]

Created attachment 71301 [details]
Patch proposal + unit test

(In reply to comment #9)
> [...] 
> *Not* asking for review yet. Still need to finish some things, such as the unit test for the AtkAction interface

After two weeks of first getting my devel environment broken and then see how my laptop's motherboard died, I'm finally back at work... :-)

Attaching now a new patch with some minor issues fixed and adding the pending unit test to check the implementation of the AtkAction interface in the new class added as subclass of AtkHyperlink.

Hence, finally asking for review now.

[Martin Robinson]

Comment on attachment 71301 [details]
Patch proposal + unit test

View in context: https://bugs.webkit.org/attachment.cgi?id=71301&action=review

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1940
> +static AtkHyperlink* webkitAccessibleHypertextGetLink(AtkHypertext* hypertext, gint index)

I think you can change the index parameter type to size_t and avoid the cast below.

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1943
> +    AccessibilityObject::AccessibilityChildrenVector children = coreObject->children();

Could this just core(hypertext->children();

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1966
> +static gint webkitAccessibleHypertextGetNLinks(AtkHypertext* hypertext)

Avoid abbreviations here. webkitAccessibleHypertextGetLinkCount or webkitAccessibleHypertextGetNumberOfLinks. The method could also return a size_t.

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1972
> +    for (unsigned i = 0; i < children.size(); i++) {

I think you should use size_t here instead of unsigned.

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1985
> +    gint nLinks = webkitAccessibleHypertextGetNLinks(hypertext);
> +    if (!nLinks)
> +        return -1;

Should be numberOfLinks or perhaps linksCount. Avoid abbreviations. The type should also probably be size_t.

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1987
> +    for (gint i = 0; i < nLinks; i++) {

size_t again.

> WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:102
> +static gint webkitAccessibleHyperlinkActionGetNActions(AtkAction* action)

Avoid the abbreviation here as well.

> WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:165
> +static gchar* webkitAccessibleHyperlinkGetUri(AtkHyperlink* link, gint index)

Should be webkitAccessibleHyperlinkGetURI

> WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:168
> +    // FIXME Do NOT support more than one instance of an AtkObject

Very small nit! FIXME -> FIXME:

> WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:303
> +GType
> +webkitAccessibleHyperlinkGetType(void)

Don't use a newline after GType here, if it can be avoided remove the void.

> WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:342
> +    g_hash_table_remove(hashTable, (void*)linkImpl);

I could be wrong, but I don't think you need to cast to void*. If you do, you should use a static_cast<void*>().

[Mario Sanchez Prada]

Created attachment 71406 [details]
Patch proposal + unit test

Attaching a new patch addressing some of these issues. Others couldn't be changed due to the reasons explained below...

(In reply to comment #11)
> (From update of attachment 71301 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71301&action=review
> 
> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1940
> > +static AtkHyperlink* webkitAccessibleHypertextGetLink(AtkHypertext* hypertext, gint index)
> 
> I think you can change the index parameter type to size_t and avoid the cast below.

As discussed by IRC, I can't change that since it must be compliant with the signature of the atk_hypertext_get_link() method, from the ATK interface. So the cast must stay as well :-(
 
> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1943
> > +    AccessibilityObject::AccessibilityChildrenVector children = coreObject->children();
> 
> Could this just core(hypertext->children();

Sure! Done.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1966
> > +static gint webkitAccessibleHypertextGetNLinks(AtkHypertext* hypertext)
> 
> Avoid abbreviations here. webkitAccessibleHypertextGetLinkCount or webkitAccessibleHypertextGetNumberOfLinks. The method could also return a size_t.

As discussed in IRC, in this case it's better to leave it that way so (1) it matches better the method it's implementing from the ATK interface and (2) because it's the way it's done all around in the ATK wrapper, so changing that in this case would lead to confusion in further readings of the code.
Hence, left as it was in the new patch.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1972
> > +    for (unsigned i = 0; i < children.size(); i++) {
> 
> I think you should use size_t here instead of unsigned.

Done.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1985
> > +    gint nLinks = webkitAccessibleHypertextGetNLinks(hypertext);
> > +    if (!nLinks)
> > +        return -1;
> 
> Should be numberOfLinks or perhaps linksCount. Avoid abbreviations. The type should also probably be size_t.

Agreed to both points. Done.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1987
> > +    for (gint i = 0; i < nLinks; i++) {
> 
> size_t again.

Done.

> > WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:102
> > +static gint webkitAccessibleHyperlinkActionGetNActions(AtkAction* action)
> 
> Avoid the abbreviation here as well.

Can't do it, same reasons as explained above (match method from ATK interface)

> > WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:165
> > +static gchar* webkitAccessibleHyperlinkGetUri(AtkHyperlink* link, gint index)
> 
> Should be webkitAccessibleHyperlinkGetURI

Can't do it, same reasons as explained above (match method from ATK interface)
 
> > WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:168
> > +    // FIXME Do NOT support more than one instance of an AtkObject
> 
> Very small nit! FIXME -> FIXME:

Done.
 
> > WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:303
> > +GType
> > +webkitAccessibleHyperlinkGetType(void)
> 
> Don't use a newline after GType here, if it can be avoided remove the void.

Opps! I missed that one, sorry. Fixed.

> > WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:342
> > +    g_hash_table_remove(hashTable, (void*)linkImpl);
> 
> I could be wrong, but I don't think you need to cast to void*. If you do, you should use a static_cast<void*>().

Actually you're right and I'm wrong :-). Fixed

Thanks for the review!

[Martin Robinson]

Comment on attachment 71406 [details]
Patch proposal + unit test

View in context: https://bugs.webkit.org/attachment.cgi?id=71406&action=review

Looks great, but r- pending further investigation and some small cleanup.

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1969
> +    if (!children.size())
> +      return 0;

The indentation looks wrong here.

> WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:44
> +// Use this hash table to help reusing instances of the
> +// WebKitAccessibleHyperlink class throughout the
> +// webkitAccessibleHyperlinkGetOrCreate method
> +static GHashTable* hashTable = 0;

I offered a suggestion via Jabber about how this might be avoided.

> WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:151
> +    if (!ATK_IS_ACTION(WEBKIT_ACCESSIBLE_HYPERLINK(action)->linkImpl))
> +        return FALSE;
> +
> +    AccessibilityObject* coreObject = core(action);
> +    if (!coreObject)
> +        return FALSE;

The return type of this method is gchar*. I don't think FALSE is correct here.

> WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:194
> +    gint baseLength = TextIterator::rangeLength(range);

I think when not exposed to the API, we prefer to use C types. So int.

> WebKit/gtk/tests/testatk.c:979
> +    g_idle_add((GSourceFunc)bail_out, loop);
> +    g_main_loop_run(loop);

Do we need this extra main loop or can we just spin the main main loop?

[Mario Sanchez Prada]

Created attachment 72159 [details]
Patch proposal + unit test

(In reply to comment #13)
> (From update of attachment 71406 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71406&action=review
> 
> Looks great, but r- pending further investigation and some small cleanup.

Attaching a new patch now, and I honestly think it's way better thank to your feedback. Thanks!

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1969
> > +    if (!children.size())
> > +      return 0;
> 
> The indentation looks wrong here.

Fixed.

> > WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:44
> > +// Use this hash table to help reusing instances of the
> > +// WebKitAccessibleHyperlink class throughout the
> > +// webkitAccessibleHyperlinkGetOrCreate method
> > +static GHashTable* hashTable = 0;
> 
> I offered a suggestion via Jabber about how this might be avoided.

True, and I finally implemented it as it's way better than using this global hashtable:

It basically consists of just having a property in the WebKitAccessible instance implementing the AtkHyperlinkImpl interface to keep a reference to the WebKitAccessibleHyperlink instance, so we can just create and assign that property the first time it's required to create the AtkHyperlink, and reuse it later. Also, from the side of the WebKitAccessibleHyperlink instance, a weak reference is kept to the WebKitAccessible instance, so we can safely unref that AtkHyperlink when the AtkObject is destroyed too.

It's cleaner, safer and IMHO better, and it works like a charm. Thanks a lot for the suggestion

> > WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:151
> > +    if (!ATK_IS_ACTION(WEBKIT_ACCESSIBLE_HYPERLINK(action)->linkImpl))
> > +        return FALSE;
> > +
> > +    AccessibilityObject* coreObject = core(action);
> > +    if (!coreObject)
> > +        return FALSE;
> 
> The return type of this method is gchar*. I don't think FALSE is correct here.

Ooops! Fixed.

> > WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:194
> > +    gint baseLength = TextIterator::rangeLength(range);
> 
> I think when not exposed to the API, we prefer to use C types. So int.

Got it. Fixed.

> > WebKit/gtk/tests/testatk.c:979
> > +    g_idle_add((GSourceFunc)bail_out, loop);
> > +    g_main_loop_run(loop);
> 
> Do we need this extra main loop or can we just spin the main main loop?

No, we actually don't need it, just spinning the main loop would be enough. Fixed (in fashion with the new trend :-))

Asking for review again then...

[Mario Sanchez Prada]

(In reply to comment #14)
> [...]
> > > WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:44
> > > +// Use this hash table to help reusing instances of the
> > > +// WebKitAccessibleHyperlink class throughout the
> > > +// webkitAccessibleHyperlinkGetOrCreate method
> > > +static GHashTable* hashTable = 0;
> > 
> > I offered a suggestion via Jabber about how this might be avoided.
> 
> True, and I finally implemented it as it's way better than using this global hashtable:
> 
> It basically consists of just having a property in the WebKitAccessible instance implementing the 
> AtkHyperlinkImpl interface to keep a reference to the WebKitAccessibleHyperlink instance, so we can just 
> create and assign that property the first time it's required to create the AtkHyperlink, and reuse it later. 
> Also, from the side of the WebKitAccessibleHyperlink instance, a weak reference is kept to the 
> WebKitAccessible instance, so we can safely unref that AtkHyperlink when the AtkObject is destroyed too.

Sorry, I didn't give the right explanation here:

I did not use a property in the WebKitAccessible class to hold a pointer to WebKitAccessibleHyperlinkg, basically because that would affect to all the subclasses of WebKitAccessible, no matter what interface they implemented, and so that would require some dirty code to only actually use it in those instances of classes implementing the AtkHyperlinkImpl interface (which are dinamically generated, btw).

Besides, as the classes actually being used are dinamically defined and registered (depending on the interfaces being implemented) I couldn't install such a property in any of them either...

So, the final solution was basically to use the g_object_set_data() / g_object_get_data() mechanism to make sure the 'property' is used only when needed, from webkitAccessibleHyperlinkImplGetHyperlink():

  static AtkHyperlink* webkitAccessibleHyperlinkImplGetHyperlink(AtkHyperlinkImpl* hyperlink)
  {
      AtkHyperlink* hyperlinkObject = ATK_HYPERLINK(g_object_get_data(G_OBJECT(hyperlink), "hyperlink-object"));
      if (!hyperlinkObject) {
          hyperlinkObject = ATK_HYPERLINK(webkitAccessibleHyperlinkNew(hyperlink));
          g_object_set_data(G_OBJECT(hyperlink), "hyperlink-object", hyperlinkObject);
      }
      return hyperlinkObject;
  }

Sorry for the wrong explanation before, I was thinking of the orignal proposal (a property) not in the actual final implementation :/

[Martin Robinson]

Comment on attachment 72159 [details]
Patch proposal + unit test

Thanks!

[Mario Sanchez Prada]

Committed r71026: <http://trac.webkit.org/changeset/71026>