Bug 118359 – [ATK] Do not expose '\n' for wrapped lines with ATK_TEXT_BOUNDARY_CHAR

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Mario Sanchez Prada]

Currently, WebKitGTK+ is exposing '\n' for every single visual line break in paragraphs, while it should only be doing it for those which are enforced by the author of the content (e.g. by using <br>), and not for every single "visual end of line" that might happen for other reasons, such as text wrapping (where it should be exposing just a " " if it's not the end of the paragraph). 

This is a regression compared to previous versions and has slipped in due to not have proper tests in place for it, so we should fix it and provide new tests to avoid the hassle in the future.

[Mario Sanchez Prada]

Created attachment 206007 [details]
Patch proposal plus new Unit test

Here's the patch. It's as simple as removing the specific code I added some weeks ago by mistake, believing it was the expected behaviour (which today I double checked that it is NOT).

Anyway, the most important part of the patch IMHO is the unit test which should help avoiding these issues in the future, and which also helped me find another bug in GAIL.

[Martin Robinson]

Comment on attachment 206007 [details]
Patch proposal plus new Unit test

Looks reasonable to me, but it probably makes sense to ensure that all new a11y tests are WebKit2 tests.

[Carlos Garcia Campos]

Comment on attachment 206007 [details]
Patch proposal plus new Unit test

Can this be tested in WebKit2? I'm not sure it's worth it adding new unit tests for wk1 API.

[Mario Sanchez Prada]

(In reply to comment #3)
> (From update of attachment 206007 [details])
> Can this be tested in WebKit2? I'm not sure it's worth it adding new unit tests > for wk1 API.

We can not test this in WebKit1 because we are checking the implementation of ATK interfaces here, and we only can use ATK APIs if we are in the same process where the accessibility hierarchy lives.

What we can do in WebKit2, though, is to check the accessibility hierarchy (and indirectly the ATK implementation) through AT-SPI2, but then you introduce several points of failure more, such as AT-SPI2, the registry, D-Bus and the ATK bridge. And on top of that you would not be testing the ATK implementation either.

So, that's why, at the moment, in WebKit2 we use AT-SPI2 to check only the "connection" between the two process by terms of AtkSocket/AtkPlug, and we leave the actual checking of the ATK implementation to the unit tests in WebKit1.

Additionally, these specific tests wouldn't work either in WebKit2 even if we implemented them in terms of AT-SPI2, since the LINE boundary will be completely broken there while we keep using Pango/Gail for that (see bugs 73433 and 114867).

However, I agree that we should think of a solution for the future at some point, maybe by migrating all the ATK-specific tests to AT-SPI2 specific ones. But for the time being it would be great if we could keep using this testatk.c file.

At least until we manage to get rid of the Pango/Gail dependency, which is the other reason why WebKit2 is not fully accessible right now.

[Carlos Garcia Campos]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 206007 [details] [details])
> > Can this be tested in WebKit2? I'm not sure it's worth it adding new unit tests > for wk1 API.
> 
> We can not test this in WebKit1 because we are checking the implementation of ATK interfaces here, and we only can use ATK APIs if we are in the same process where the accessibility hierarchy lives.
> 
> What we can do in WebKit2, though, is to check the accessibility hierarchy (and indirectly the ATK implementation) through AT-SPI2, but then you introduce several points of failure more, such as AT-SPI2, the registry, D-Bus and the ATK bridge. And on top of that you would not be testing the ATK implementation either.
> 
> So, that's why, at the moment, in WebKit2 we use AT-SPI2 to check only the "connection" between the two process by terms of AtkSocket/AtkPlug, and we leave the actual checking of the ATK implementation to the unit tests in WebKit1.
> 
> Additionally, these specific tests wouldn't work either in WebKit2 even if we implemented them in terms of AT-SPI2, since the LINE boundary will be completely broken there while we keep using Pango/Gail for that (see bugs 73433 and 114867).
> 
> However, I agree that we should think of a solution for the future at some point, maybe by migrating all the ATK-specific tests to AT-SPI2 specific ones. But for the time being it would be great if we could keep using this testatk.c file.
> 
> At least until we manage to get rid of the Pango/Gail dependency, which is the other reason why WebKit2 is not fully accessible right now.

Ok, we'll port it then when we add support for tests in the web process using injected bundle.

[Carlos Garcia Campos]

Comment on attachment 206007 [details]
Patch proposal plus new Unit test

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

> Source/WebKit/gtk/tests/testatk.c:952
> +    webkit_web_view_load_string(webView, contentsWithWrappedLines, 0, 0, 0);

Don't you need to run a main loop and wait until the page is loaded to continue with the test?

> Source/WebKit/gtk/tests/testatk.c:956
> +    g_object_set(G_OBJECT(settings), "enable-caret-browsing", TRUE, NULL);

g_object_set receives a gpointer, not a GObject, so you don't need the cast.

> Source/WebKit/gtk/tests/testatk.c:995
> +    /* Check the paragraph with the text wrapped because of <br> elements. */

You are mixing C and C++ comments in this test, please fix it before landing.

[Mario Sanchez Prada]

Thanks for the review Carlos. I'm submitting this patch soon with your points addressed.

(In reply to comment #6)
> (From update of attachment 206007 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206007&action=review
> 
> > Source/WebKit/gtk/tests/testatk.c:952
> > +    webkit_web_view_load_string(webView, contentsWithWrappedLines, 0, 0, 0);
> 
> Don't you need to run a main loop and wait until the page is loaded to continue with the test?

Not anymore. It's enough with spinning manually the current loop to ensure the web is loaded when getting the root element, which is performed internally in getWebAreaObject().

> > Source/WebKit/gtk/tests/testatk.c:956
> > +    g_object_set(G_OBJECT(settings), "enable-caret-browsing", TRUE, NULL);
> 
> g_object_set receives a gpointer, not a GObject, so you don't need the cast.

Will fix.

> > Source/WebKit/gtk/tests/testatk.c:995
> > +    /* Check the paragraph with the text wrapped because of <br> elements. */
> 
> You are mixing C and C++ comments in this test, please fix it before landing.

Will fix.

[Mario Sanchez Prada]

Committed r152396: <http://trac.webkit.org/changeset/152396>