Bug 99578 – [EFL] Share WebKit-Gtk's Accessibility implementation with others WebKit ports.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Krzysztof Czech]

WebKit-GTK ensures Accessibility support with ATK library.
WebKit-EFL's implementation will be based on ATK as well. I think it does not make sense to
duplicate the implementation. Core parts of the code could be shared among the ports and differences put inside
specific platform macros. I propose to move the implementation from ./WebCore/accessibility/gtk to ./WebCore/accessibility/atk.

[Mario Sanchez Prada]

Adding Piñeiro and Joanmarie to this. I think they might be interested too.

[Alejandro Piñeiro]

(In reply to comment #0)
> WebKit-GTK ensures Accessibility support with ATK library.
> WebKit-EFL's implementation will be based on ATK as well. I think it does not make sense to
> duplicate the implementation. Core parts of the code could be shared among the ports and differences put inside
> specific platform macros. I propose to move the implementation from ./WebCore/accessibility/gtk to ./WebCore/accessibility/atk.

I agree that sharing the ATK support as much as possible makes total sense. That directory name makes sense to me.

Anyway, I think that the first step could check if there are some kind of any "pure gtk" code on current accessibility/gtk directory. If so, next step would be split that code, and check if we need to maintain a accessibility/gtk directory with that gtk-specific code, or move it to the general gtk code (as I'm thinking on gtk-widget initialization related code)

[Mario Sanchez Prada]

(In reply to comment #2)
> [...]
> Anyway, I think that the first step could check if there are some kind of any
> "pure gtk" code on current accessibility/gtk directory. If so, next step would
> be split that code, and check if we need to maintain a accessibility/gtk
> directory with that gtk-specific code, or move it to the general gtk code

As far as I know, there are a couple of references to GtkWidget in WebKitAccessibleWrapperAtk.cpp and then some usages of GAIL's gail_text_util_get_text() in WebKitAccessibilityInterfaceText.cpp. Other than that, the remaining stuff should be reusable among ports, I think.

> (as I'm thinking on gtk-widget initialization related code)

Not sure what you mean. If you're talking about the initialization of the WebView (which is a GtkWidget) and its implementation of gtk_widget_get_accessible(), such code is not here in WebCore/accessibility/gtk, but in WebKit/gtk/webkit (for WK1) and WebKit2/UIProcess/API/gtk (for WK2).

Btw, forgot to mention it before, but I also agree with sharing this a11y code between the GTK and the EFL ports looks like a good idea

[Alejandro Piñeiro]

(In reply to comment #3)
> (In reply to comment #2)
> > [...]
> > Anyway, I think that the first step could check if there are some kind of any
> > "pure gtk" code on current accessibility/gtk directory. If so, next step would
> > be split that code, and check if we need to maintain a accessibility/gtk
> > directory with that gtk-specific code, or move it to the general gtk code
> 
> As far as I know, there are a couple of references to GtkWidget in WebKitAccessibleWrapperAtk.cpp and then some usages of GAIL's gail_text_util_get_text() in WebKitAccessibilityInterfaceText.cpp. Other than that, the remaining stuff should be reusable among ports, I think.

Hmm, good point. The problematic point would be the usage of gail_text_util methods on the AtkText implementation. FWIW, GTK and clutter are not using them anymore, but using a private fully pango-based library. So another task would be check what EFL is using for the text rendering.

Probably this bug will became a metabug of all the tasks needed to make the move. Your paragraph detects two.

> > (as I'm thinking on gtk-widget initialization related code)
> 
> Not sure what you mean. If you're talking about the initialization of the WebView (which is a GtkWidget) and its implementation of gtk_widget_get_accessible(), such code is not here in WebCore/accessibility/gtk, but in WebKit/gtk/webkit (for WK1) and WebKit2/UIProcess/API/gtk (for WK2).

Yes I was talking about that. So good to know that we can discard my concern as a new task.


> Btw, forgot to mention it before, but I also agree with sharing this a11y code between the GTK and the EFL ports looks like a good idea

[Mario Sanchez Prada]

(In reply to comment #4)
> [...]
> Hmm, good point. The problematic point would be the usage of gail_text_util 
> methods on the AtkText implementation. FWIW, GTK and clutter are not using 
> them anymore, but using a private fully pango-based library.

Yes, which reminds me of another issue with the a11y code here: pango is still being used in some places there and, as far as I know, its usage inside WebKit is discouraged and should be avoided.

I think the right replacement is TextBreakIterator, not 100% sure, though.

> So another task would be check what EFL is using for the text rendering.

Text rendering code is not an issue in WebCore/accessibility/gtk. Pango is being used only to get a PangoLayout to pass to gail_text_util_get_text() so I think getting rid of gail would be enough (as easy as it sounds, as hard as it might be :-))

And AFAIK, the EFL port is already avoiding pango and relying in TextBreakIterator instead, but Krysztof will know better, I suppose.

> Probably this bug will became a metabug of all the tasks needed to make the 
> move. Your paragraph detects two.

Yes, could be.

> > > (as I'm thinking on gtk-widget initialization related code)
> > 
> > Not sure what you mean. If you're talking about the initialization of the 
> > WebView (which is a GtkWidget) and its implementation of 
> > gtk_widget_get_accessible(), such code is not here in 
> > WebCore/accessibility/gtk, but in WebKit/gtk/webkit (for WK1) and 
> > WebKit2/UIProcess/API/gtk (for WK2).
> 
> Yes I was talking about that. So good to know that we can discard my concern
> as a new task.

Ok, great.

[Grzegorz Czajkowski]

(In reply to comment #5)
> (In reply to comment #4)
> > [...]
> > Hmm, good point. The problematic point would be the usage of gail_text_util 
> > methods on the AtkText implementation. FWIW, GTK and clutter are not using 
> > them anymore, but using a private fully pango-based library.
> 
> Yes, which reminds me of another issue with the a11y code here: pango is still being used in some places there and, as far as I know, its usage inside WebKit is discouraged and should be avoided.
> 
> I think the right replacement is TextBreakIterator, not 100% sure, though.

Pango has been finally removed from WebKit-EFL dependencies. We found out all needed replacements for example, the spell checker feature uses TextBreakIterator to determine the beginning/end of the word (bug 94320).

> 
> > So another task would be check what EFL is using for the text rendering.
> 
> Text rendering code is not an issue in WebCore/accessibility/gtk. Pango is being used only to get a PangoLayout to pass to gail_text_util_get_text() so I think getting rid of gail would be enough (as easy as it sounds, as hard as it might be :-))

I completely agree with you. IMHO it looks like  WebCore methods could be used (like TextIterator) instead of gail_text_util_get_text(). The current implementation already uses it and based on Range (WebCore/dom/Range.h) we could manipulate TextIterator object in the same way as gail is doing.

[Mario Sanchez Prada]

Created attachment 170876 [details]
1. Isolate the GTK/Gail/Pango specific code in accessibility/gtk

(In reply to comment #6)
> [...]
> I completely agree with you. IMHO it looks like  WebCore methods could be used (like TextIterator) instead of gail_text_util_get_text(). 
> The current implementation already uses it and based on Range (WebCore/dom/Range.h) we could manipulate TextIterator object in the same
> way as gail is doing.

I'm attaching a quick patch I did by adding some #if PLATFORM(GTK) (and removing 'GTK' from some comments too) as a starting point, in order to show which parts inside accessibility/gtk are really depending on GTK+/Gail/Pango and which ones can be already shared with other ports, as they are just depending on GLib and ATK.

As you can see the big deal is the implementation of atk_text_get_text*() functions, in WebKitAccessibleInterfaceText.cpp, and finding the parent accessible object for the root AtkObject, in WebKitAccessibleWrapperAtk.cpp. For the later, I'm afraid there're no big chances we can reuse things so we probably won't be able to get rid of that "#if PLATFORM" there, but for the atk_text_get_text*() functions I think that the implementation needed for EFL could be reused for GTK too, helping WebKitGtk+ get rid of Pango/Gail dependencies there, which would be a big win.

What do you think? If you think this makes sense, the remaining step for making the patch for this bug complete would be just to rename the directory to atk and update the makefiles.

[Mario Sanchez Prada]

Created attachment 170877 [details]
Renamed accessibility/gtk to accessibility/atk

This would be the other one

[Alejandro Piñeiro]

(In reply to comment #7)
> Created an attachment (id=170876) [details]
> 1. Isolate the GTK/Gail/Pango specific code in accessibility/gtk
> 
> (In reply to comment #6)
> > [...]
> > I completely agree with you. IMHO it looks like  WebCore methods could be used (like TextIterator) instead of gail_text_util_get_text(). 
> > The current implementation already uses it and based on Range (WebCore/dom/Range.h) we could manipulate TextIterator object in the same
> > way as gail is doing.
> 
> I'm attaching a quick patch I did by adding some #if PLATFORM(GTK) (and removing 'GTK' from some comments too) as a starting point, in order to show which parts inside accessibility/gtk are really depending on GTK+/Gail/Pango and which ones can be already shared with other ports, as they are just depending on GLib and ATK.

Seems a good starting point, thanks for it.

> As you can see the big deal is the implementation of atk_text_get_text*() functions, in WebKitAccessibleInterfaceText.cpp, and finding the parent accessible object for the root AtkObject, in WebKitAccessibleWrapperAtk.cpp. For the later, I'm afraid there're no big chances we can reuse things so we probably won't be able to get rid of that "#if PLATFORM" there, but for the atk_text_get_text*() functions I think that the implementation needed for EFL could be reused for GTK too, helping WebKitGtk+ get rid of Pango/Gail dependencies there, which would be a big win.

Yeah, probably it would be hard to remove the GTK specific code in order to get the parent. Anyway, this is just one method, and most of the gtk specific code are the text related methods.

> What do you think? If you think this makes sense, the remaining step for making the patch for this bug complete would be just to rename the directory to atk and update the makefiles.

That is your next patch ;)

[Gyuyoung Kim]

I think you have to change glib data type with common things in order to move from gtk to atk.

For example, 

g_value_init(gValue, G_TYPE_FLOAT);
GValue* gValue
gboolean
g_value_set_float
gchar
g_return_val_if_fail

and so on.

Those files you are moving are using above data types or glib macros. In my humble opinion, we can't move from gtk to atk without this glib dependency.

[Gyuyoung Kim]

(In reply to comment #10)

> Those files you are moving are using above data types or glib macros. In my humble opinion, we can't move from gtk to atk without this glib dependency.

Typo : without -> with.

[Martin Robinson]

(In reply to comment #10)
 
> Those files you are moving are using above data types or glib macros. In my humble opinion, we can't move from gtk to atk without this glib dependency.

ATK itself depends on GLib, so what is the purpose of trying to remove calls to GLib in the ATK accessibility code? Perhaps I'm misunderstanding what you are saying though.

[Mario Sanchez Prada]

(In reply to comment #12)
> (In reply to comment #10)
> 
> > Those files you are moving are using above data types or glib macros. In my humble opinion, we can't move from gtk to atk without this glib dependency.
> 
> ATK itself depends on GLib, so what is the purpose of trying to remove calls to 
> GLib in the ATK accessibility code? Perhaps I'm misunderstanding what you are saying though.

Exactly. I think the confusion might come from mixing GTK with GLib? GTK depends on Glib, exactly in the same way ATK does, and we are talking about going away  from GTK-specific code only, I think, so we should keep those data types around.

Remember that ATK is not just a toolkit-independent definition of interfaces for accessibility related stuff, that is AT-SPI, which is what assistive technologies like screen readers understand in Linux based accessible environments.

However, and according to [1], ATK is "a development toolkit from GNOME which allows programmers to use common GNOME accessibility features", full of GObject interfaces which is what applications/toolkits willing to expose accessibility related information must implement.

Communication between ATK-based accessible applications and AT-SPI based assistive technologies happens thanks to the AT-SPI<->ATK bridge, which makes all the mapping through IPC (D-Bus) between those two worlds.

So, in a nutshell, if the EFL port is willing to reuse the ATK implementation of WebKitGTK+ it needs to keep Glib. Other option is to go ahead and implement their own "ATK-like thing", if it's not there yet, or to go and speak directly with the AT-SPI world from EFL's widget library. That's what they do in Qt with their atspi-qt bridge I think.

[1] http://www.linuxfoundation.org/collaborate/workgroups/accessibility/atk/at-spi

[Martin Robinson]

(In reply to comment #13)

> So, in a nutshell, if the EFL port is willing to reuse the ATK implementation of WebKitGTK+ it needs to keep Glib. Other option is to go ahead and implement their own "ATK-like thing", if it's not there yet, or to go and speak directly with the AT-SPI world from EFL's widget library. That's what they do in Qt with their atspi-qt bridge I think.

It's also important to keep in mind that EFL has an implicit dependency on GLib anyway via the libsoup backend.

[Gyuyoung Kim]

(In reply to comment #14)
> (In reply to comment #13)
> 
> > So, in a nutshell, if the EFL port is willing to reuse the ATK implementation of WebKitGTK+ it needs to keep Glib. Other option is to go ahead and implement their own "ATK-like thing", if it's not there yet, or to go and speak directly with the AT-SPI world from EFL's widget library. That's what they do in Qt with their atspi-qt bridge I think.
> 
> It's also important to keep in mind that EFL has an implicit dependency on GLib anyway via the libsoup backend.

If ATK depends on glib, EFL port also should use glib as libsoup case.  I just thought common module should not have specific port dependency. If this is able to be allowed, I don't mind.


Krzysztof, 

As we know, EFL itself is going to support a library for accessibility. Will you use ATK for EFL port though ?

[Krzysztof Czech]

This common module "atk" will not have any specific port dependency, the only dependency is glib, but we are OK with it, regarding what has been said about it.

Well, yes EFL as a framework has some support for Accessibility, but not in terms of ATK and WebKit.
WebKit-GTK had already done that. Mappings to atk are already implemented and as Alejendro said they're are going to be improved. The only thing we (EFL developers) should take care of, are toolkit differences in terms of proper communication with EFL.
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > 
> > > So, in a nutshell, if the EFL port is willing to reuse the ATK implementation of WebKitGTK+ it needs to keep Glib. Other option is to go ahead and implement their own "ATK-like thing", if it's not there yet, or to go and speak directly with the AT-SPI world from EFL's widget library. That's what they do in Qt with their atspi-qt bridge I think.
> > 
> > It's also important to keep in mind that EFL has an implicit dependency on GLib anyway via the libsoup backend.
> 
> If ATK depends on glib, EFL port also should use glib as libsoup case.  I just thought common module should not have specific port dependency. If this is able to be allowed, I don't mind.
> 
> 
> Krzysztof, 
> 
> As we know, EFL itself is going to support a library for accessibility. Will you use ATK for EFL port though ?

[Gyuyoung Kim]

(In reply to comment #16)
> This common module "atk" will not have any specific port dependency, the only dependency is glib, but we are OK with it, regarding what has been said about it.

IMO, EFL should not use glib, but we have been used it because there are no alternative solution yet. If EFL supports network library, I think we didn't use libsoup.

> Well, yes EFL as a framework has some support for Accessibility, but not in terms of ATK and WebKit.
> WebKit-GTK had already done that. Mappings to atk are already implemented and as Alejendro said they're are going to be improved. The only thing we (EFL developers) should take care of, are toolkit differences in terms of proper communication with EFL.

Thank you for your explanation. If there is no alternative solution in EFL, I'm ok for now though I don't like to have new glib dependency,

[Krzysztof Czech]

> Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceText.cpp:582
> +    notImplemented()

Missing semicolon.

> Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceText.cpp:592
> +    notImplemented()

Missing semicolon.

> Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:246
> +#endif // PLATFORM(GTK)

Compilation error on other platforms.
Should be moved two lines forward.

> Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:248
>          }

#endif // PLATFORM(GTK)
Should be placed here.


Would like me to continue this bug ?. I think it can be already shipped to mainstream. I found only some minor mistakes

Patches do not make any problems on GTK regarding a11y layout tests.
Results are the same before and after the change.

Found 157 tests; running 138, skipping 19.
All 138 tests run as expected.

[Mario Sanchez Prada]

(In reply to comment #18)
> > Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceText.cpp:582
> > +    notImplemented()
> 
> Missing semicolon.
> 
> > Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceText.cpp:592
> > +    notImplemented()
> 
> Missing semicolon.
>
> > Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:246
> > +#endif // PLATFORM(GTK)
> 
> Compilation error on other platforms.
> Should be moved two lines forward.

Oops! Sometimes I'm so absentminded... thanks for catching these.

> > Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:248
> >          }
> 
> #endif // PLATFORM(GTK)
> Should be placed here.
> 
> Would like me to continue this bug ?. I think it can be already shipped to 
> mainstream. I found only some minor mistakes

No problem. If the patches are good for you already as they are (as it seems to be), I think I can do it myself. Actually I have filed a new bug for the first part (see bug 101727) and plan to provide an updated patch for the second one (moving from gtk/ to atk/) now.

> Patches do not make any problems on GTK regarding a11y layout tests.
> Results are the same before and after the change.
> 
> Found 157 tests; running 138, skipping 19.
> All 138 tests run as expected.

Great! Happy to help

[Mario Sanchez Prada]

Comment on attachment 170877 [details]
Renamed accessibility/gtk to accessibility/atk

(In reply to comment #19)
> [...]
> No problem. If the patches are good for you already as they are (as it 
> seems to be), I think I can do it myself. Actually I have filed a new bug 
> for the first part (see bug 101727) and plan to provide an updated patch 
> for the second one (moving from gtk/ to atk/) now.

Well, actually there's no new patch to attach, since I'm afraid the second one already attached in this bug is the one we would need to get in.

Asking for review then

[WebKit Review Bot]

Attachment 170877 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/accessibility/atk/WebKitAccessibleUtil.cpp:167:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/accessibility/atk/WebKitAccessibleHyperlink.cpp:342:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/accessibility/atk/WebKitAccessibleHyperlink.cpp:343:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:206:  webkit_accessible_parent_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:624:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:747:  webkit_accessible_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:771:  type_volatile is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:788:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:967:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:968:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:972:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:210:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:217:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.h:49:  parent_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.h:52:  webkit_accessible_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 17 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Krzysztof Czech]

(In reply to comment #19)
> (In reply to comment #18)
> > > Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceText.cpp:582
> > > +    notImplemented()
> > 
> > Missing semicolon.
> > 
> > > Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceText.cpp:592
> > > +    notImplemented()
> > 
> > Missing semicolon.
> >
> > > Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:246
> > > +#endif // PLATFORM(GTK)
> > 
> > Compilation error on other platforms.
> > Should be moved two lines forward.
> 
> Oops! Sometimes I'm so absentminded... thanks for catching these.
> 
> > > Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:248
> > >          }
> > 
> > #endif // PLATFORM(GTK)
> > Should be placed here.
> > 
> > Would like me to continue this bug ?. I think it can be already shipped to 
> > mainstream. I found only some minor mistakes
> 
> No problem. If the patches are good for you already as they are (as it seems to be), I think I can do it myself. Actually I have filed a new bug for the first part (see bug 101727) and plan to provide an updated patch for the second one (moving from gtk/ to atk/) now.
Yes, patches are good for me. I've started EFL part (100848). I think WebProcess part can be also shared. I'm on the way to propose a patch.
> 
> > Patches do not make any problems on GTK regarding a11y layout tests.
> > Results are the same before and after the change.
> > 
> > Found 157 tests; running 138, skipping 19.
> > All 138 tests run as expected.
> 
> Great! Happy to help

[Mario Sanchez Prada]

(In reply to comment #21)
> Attachment 170877 [details] did not pass style-queue:
> [...] 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

This patch is just about renaming a directory, so I don't think it makes much sense to pay attention to this coding style issues in this case.

Ping reviewers?

[Martin Robinson]

Comment on attachment 170877 [details]
Renamed accessibility/gtk to accessibility/atk

Might as well fix the style issues when landing the patch.

[Krzysztof Czech]

Mario, could take a look at https://bugs.webkit.org/show_bug.cgi?id=101748

[Mario Sanchez Prada]

(In reply to comment #24)
> (From update of attachment 170877 [details])
> Might as well fix the style issues when landing the patch.

:-) Ok, I'll do. They are not that many issues anyway.

Thanks for the review

[Mario Sanchez Prada]

(In reply to comment #25)
> Mario, could take a look at https://bugs.webkit.org/show_bug.cgi?id=101748

I see Martin has already r+'ed it, but sure I'll take a look anyway.

[Mario Sanchez Prada]

Committed r134939: <http://trac.webkit.org/changeset/134939>