Bug 120638 – [ATK] Implement new API in AtkText: atk_text_get_string_at_offset()

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

New API for AtkText has been introduced in ATK 2.9.4 (see release notes in [1]) after the work started during GUADEC as part of GNOME bug 705580 [2], so now implementors such as WebKitGTK+ should provide an implementation for this new function atk_text_get_string_at_offset(), which should provide exactly the same semantics than the old atk_text_get_text_at_offset() for ATK_TEXT_BOUNDARY_*_START boundaries.

We can't just remove the implementation of the other functions yet because we can't raise the ATK dependency up to 2.9.4 yet, and also because we can't expect systems were WebKitGTK+ is running to have a new enough version of the AT-SPI2 subsystem (we would need at least AT-SPI 2.9.90 for that, see [3]), but we can add some conditional code (and tests) now so we are sure once the moment comes WebKitGTK+ will be ready to provide the requested information.

Of course, once we are sure we can assume modern enough versions of ATK and AT-SPI, we should do some cleaning in WebKitAccessibleInterfaceText.cpp and get rid of a lot of code we won't longer need, that is, the implementation of atk_text_get_text_*_offset() and the need to consider ATK_TEXT_BOUNDARY_*_END boundaries.

[1] https://mail.gnome.org/archives/gnome-accessibility-devel/2013-August/msg00021.html
[2] https://bugzilla.gnome.org/show_bug.cgi?id=705580
[3] https://mail.gnome.org/archives/gnome-accessibility-devel/2013-August/msg00022.html

[Mario Sanchez Prada]

Created attachment 210383 [details]
Patch proposal plus new Unit tests

Here comes the patch. As commented before, part of the code that is being added here is based/depending on the one for the previous API, which will eventually be removed once we can assume modern enough versions of ATK and AT-SPI, so don't be scared of it :)

[Mario Sanchez Prada]

I think it would be specially interesting to get this reviewed by other GTK guys, so I'm adding Gustavo, Martin and Xan to cc as well.

[Gustavo Noronha (kov)]

Comment on attachment 210383 [details]
Patch proposal plus new Unit tests

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

Looks sane to me =)

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1106
> +static gchar* webkitAccessibleTextGetStringAtOffset(AtkText* text, gint offset, AtkTextGranularity granularity, gint* startOffset, gint* endOffset)

Nit: we have been using gints elsewhere in the a11y implementation, but I'd rather use the regular types where possible to avoid confusion.

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1110
> +    // ATK_TEXT_BOUNDARY_*_END boundaries, so for now we just need to translate the

s/END/START/

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1140
> +        g_assert_not_reached();

ASSERT_NOT_REACHED() instead?

> Source/WebKit/gtk/tests/testatk.c:1402
> +    webkit_web_view_set_settings(webView, settings);

This should not be needed since you're using a settings object you got from the view instead of creating a new one. Out of curiosity, why do you have to enable caret browsing here? I thought you were not using the caret to decide where to get text from.

> Source/WebKit/gtk/tests/testatk.c:1450
> +    webkit_web_view_set_settings(webView, settings);

Ditto.

> Tools/gtk/jhbuild.modules:234
> -    <branch module="pub/GNOME/sources/atk/2.8/atk-2.8.0.tar.xz" version="2.8.0"
> +    <branch module="pub/GNOME/sources/atk/2.9/atk-2.9.4.tar.xz" version="2.9.4"
>              repo="ftp.gnome.org"
> -            hash="sha256:b22519176226f3e07cf6d932b77852e6b6be4780977770704b32d0f4e0686df4"/>
> +            hash="sha256:755c9582ca7cf01e5eaa0ac972376877eb365902f388262c21ea3425e0c4d631"/>

This will help with testing I assume? Do we not need new versions of the at-spi stuff as well?

[Mario Sanchez Prada]

Comment on attachment 210383 [details]
Patch proposal plus new Unit tests

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

Thanks for the review! See some comments below...

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1106
>> +static gchar* webkitAccessibleTextGetStringAtOffset(AtkText* text, gint offset, AtkTextGranularity granularity, gint* startOffset, gint* endOffset)
> 
> Nit: we have been using gints elsewhere in the a11y implementation, but I'd rather use the regular types where possible to avoid confusion.

This the implementation of a function from AtkText's public API, which explicitly uses gint in the signature so, while I agree with your comment, I guess in this case it's better to leave that one.

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1110
>> +    // ATK_TEXT_BOUNDARY_*_END boundaries, so for now we just need to translate the
> 
> s/END/START/

Good catch. Thanks.

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1140
>> +        g_assert_not_reached();
> 
> ASSERT_NOT_REACHED() instead?

Probably yes.

>> Source/WebKit/gtk/tests/testatk.c:1402
>> +    webkit_web_view_set_settings(webView, settings);
> 
> This should not be needed since you're using a settings object you got from the view instead of creating a new one. Out of curiosity, why do you have to enable caret browsing here? I thought you were not using the caret to decide where to get text from.

You are right about the webkit_web_view_set_settings() part.

About the caret browsing, that's a very good point. It's just here in this patch just because these "new" tests are basically new versions of the ones that are already present, which probably don't need this mode to be enabled anyway other than for testWebkitAtkCaretOffsetsAndExtranousWhiteSpaces. In other words, it might very well a nasty side effect of copy&past that passed overlooked until now. I will remove this and test it locally before landing.

>> Source/WebKit/gtk/tests/testatk.c:1450
>> +    webkit_web_view_set_settings(webView, settings);
> 
> Ditto.

Yep

>> Tools/gtk/jhbuild.modules:234
>> +            hash="sha256:755c9582ca7cf01e5eaa0ac972376877eb365902f388262c21ea3425e0c4d631"/>
> 
> This will help with testing I assume? Do we not need new versions of the at-spi stuff as well?

It's really not needed strictly speaking, since the tests here use only ATK and not AT-SPI.

However, raising to a version of AT-SPI that do support the new API (2.9.90, featuring atspi_text_get_string_at_offset) might be a good idea anyway for two reasons:

  1. It keeps the versions of ATK and AT-SPI closer, which is usually a good idea since they use to be released (almost) together
  2. It paves the way for when we move to testing all these things from WebKit2 directly, since we do not have other way to do it there than using at-spi

I will add those bits in the patch before landing

[Mario Sanchez Prada]

Committed r157165: <http://trac.webkit.org/changeset/157165>