Bug 106216 – [EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Krzysztof Czech]

Create WebPage's accessibility object on WebPage initialization.

[Krzysztof Czech]

Created attachment 181499 [details]
[EFL] Expose accessibility hierarchy in WebKit2-EFL.

[EFL EWS Bot]

Comment on attachment 181499 [details]
[EFL] Expose accessibility hierarchy in WebKit2-EFL.

Attachment 181499 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15731792

[kov's GTK+ EWS bot]

Comment on attachment 181499 [details]
[EFL] Expose accessibility hierarchy in WebKit2-EFL.

Attachment 181499 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15732742

[Krzysztof Czech]

Created attachment 181508 [details]
[EFL] Expose accessibility hierarchy in WebKit2-EFL.

[kov's GTK+ EWS bot]

Comment on attachment 181508 [details]
[EFL] Expose accessibility hierarchy in WebKit2-EFL.

Attachment 181508 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15739772

[Gyuyoung Kim]

Comment on attachment 181508 [details]
[EFL] Expose accessibility hierarchy in WebKit2-EFL.

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

> Source/WebKit2/PlatformEfl.cmake:414
> +        WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp

I move files which are already guarded by conditionals to main cmake list. Please take a look http://trac.webkit.org/changeset/138681

[Krzysztof Czech]

I see, so you mean to move this file to main cmake list ?
> (From update of attachment 181508 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181508&action=review
> 
> > Source/WebKit2/PlatformEfl.cmake:414
> > +        WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp
> 
> I move files which are already guarded by conditionals to main cmake list. Please take a look http://trac.webkit.org/changeset/138681

[Gyuyoung Kim]

(In reply to comment #7)
> I see, so you mean to move this file to main cmake list ?
> > (From update of attachment 181508 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=181508&action=review
> > 
> > > Source/WebKit2/PlatformEfl.cmake:414
> > > +        WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp
> > 
> > I move files which are already guarded by conditionals to main cmake list. Please take a look http://trac.webkit.org/changeset/138681

Yes, if there is no HAVE(ACCESSIBILITY) inside files, I think you need to add it to the files.

[Krzysztof Czech]

Yes, there is not HAVE(ACCESSIBILITY) inside, but it depends on ATK library. If ENABLE_ACCESSIBILITY is off then we will have build breaks. I think we should have this under ENABLE_ACCESSIBILITY. What do you think about that ?.
> (In reply to comment #7)
> > I see, so you mean to move this file to main cmake list ?
> > > (From update of attachment 181508 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=181508&action=review
> > > 
> > > > Source/WebKit2/PlatformEfl.cmake:414
> > > > +        WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp
> > > 
> > > I move files which are already guarded by conditionals to main cmake list. Please take a look http://trac.webkit.org/changeset/138681
> 
> Yes, if there is no HAVE(ACCESSIBILITY) inside files, I think you need to add it to the files.

[Gyuyoung Kim]

(In reply to comment #9)
> Yes, there is not HAVE(ACCESSIBILITY) inside, but it depends on ATK library. If ENABLE_ACCESSIBILITY is off then we will have build breaks. I think we should have this under ENABLE_ACCESSIBILITY. What do you think about that ?.

I think you have to add accessibility guard to atk files as http://trac.webkit.org/changeset/139159

[Krzysztof Czech]

Created attachment 181927 [details]
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

[Krzysztof Czech]

Created attachment 182306 [details]
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

[Krzysztof Czech]

Created attachment 182546 [details]
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

[Gyuyoung Kim]

Comment on attachment 182546 [details]
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

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

> Source/WebKit2/PlatformEfl.cmake:121
> +    WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp

Wrong alphabetic order.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:475
> +#if PLATFORM(GTK) || (PLATFORM(EFL) && HAVE(ACCESSIBILITY))

I wonder why EFL port only needs to use HAVE(ACCESSIBILITY) together. EFL port also enabled HAVE(ACCESSIBILITY) guard as GTK port.


#if !defined(HAVE_ACCESSIBILITY)
#if PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) && !OS(ANDROID)) || PLATFORM(EFL)
#define HAVE_ACCESSIBILITY 1
#endif

> Source/WebKit2/WebProcess/WebPage/WebPage.h:849
> +#elif PLATFORM(GTK) || (PLATFORM(EFL) && HAVE(ACCESSIBILITY))

ditto.

> Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:78
> +#if PLATFORM(EFL)

Is #if HAVE(ACCESSIBILITY) better for GTK port as well ?

> Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:112
> +#if PLATFORM(EFL)

ditto ?

[Krzysztof Czech]

> I wonder why EFL port only needs to use HAVE(ACCESSIBILITY) together. EFL port also enabled HAVE(ACCESSIBILITY) guard as GTK port.

Because we wanted to have a possibility of turning Accessibility on/off. This is a matter of having ATK library. In case ATK is not available, we can easily turning it off. See Source/cmake/OptionsEfl.cmake (line 50). Why PLATFORM(EFL) and HAVE(ACCESSIBILITY) go together?. Because PLATFORM(EFL) alone will not cover every occurrence of ATK callings. In GTK, ATK is integral part of the framework. I think it cannot be switched off. GTK always depends on it. EFL does not. That's way PLATFORM(GTK) is enough for GTK port in terms of Accessibility.

> (From update of attachment 182546 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182546&action=review
> 
> > Source/WebKit2/PlatformEfl.cmake:121
> > +    WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp
> 
> Wrong alphabetic order.
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:475
> > +#if PLATFORM(GTK) || (PLATFORM(EFL) && HAVE(ACCESSIBILITY))
> 
> I wonder why EFL port only needs to use HAVE(ACCESSIBILITY) together. EFL port also enabled HAVE(ACCESSIBILITY) guard as GTK port.
> 
> 
> #if !defined(HAVE_ACCESSIBILITY)
> #if PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) && !OS(ANDROID)) || PLATFORM(EFL)
> #define HAVE_ACCESSIBILITY 1
> #endif
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:849
> > +#elif PLATFORM(GTK) || (PLATFORM(EFL) && HAVE(ACCESSIBILITY))
> 
> ditto.
> 
> > Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:78
> > +#if PLATFORM(EFL)
> 
> Is #if HAVE(ACCESSIBILITY) better for GTK port as well ?
> 
> > Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:112
> > +#if PLATFORM(EFL)
> 
> ditto ?

[Krzysztof Czech]

Created attachment 182721 [details]
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

[Krzysztof Czech]

> > > Source/WebKit2/PlatformEfl.cmake:121
> > > +    WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp
> > 
> > Wrong alphabetic order.
Done.

[Gyuyoung Kim]

(In reply to comment #15)
> > I wonder why EFL port only needs to use HAVE(ACCESSIBILITY) together. EFL port also enabled HAVE(ACCESSIBILITY) guard as GTK port.
> 
> Because we wanted to have a possibility of turning Accessibility on/off. This is a matter of having ATK library. In case ATK is not available, we can easily turning it off. See Source/cmake/OptionsEfl.cmake (line 50). Why PLATFORM(EFL) and HAVE(ACCESSIBILITY) go together?. Because PLATFORM(EFL) alone will not cover every occurrence of ATK callings.

I don't understand this well. OptionsEfl.cmake enables ENABLE_ACCESSIBILITY to find ATK package. 

 if (ENABLE_ACCESSIBILITY)
    find_package(ATK REQUIRED)
 else ()
    add_definitions(-DHAVE_ACCESSIBILITY=0)
 endif ()

Then, the HAVE_ACCESSIBILITY is enabled by Platform.h

 #if !defined(HAVE_ACCESSIBILITY)
 #if PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(GTK) ||  (PLATFORM(CHROMIUM) && !OS(ANDROID)) || PLATFORM(EFL)
 #define HAVE_ACCESSIBILITY 1
 #endif
 #endif /* !defined(HAVE_ACCESSIBILITY) */

It means EFL port will enable HAVE_ACCESSIBILITY if we don't off ENABLE_ACCESSIBILITY in OptionsEfl.cmake. If we off ENABLE_ACCESSIBILITY in OptionsEFl.cmake, HAVE_ACCESSIBILITY is defined with disabled. So, it won't be enabled in Platform.h again. It seems to me that we already can control HAVE_ACCESSIBILITY by OptionsEfl.cmake definition. If I miss something, please let me know.

[Gyuyoung Kim]

Comment on attachment 182721 [details]
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

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

> Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:78
> +#if PLATFORM(EFL)

I think you don't need to use PLATFORM(EFL) because UNUSED_PARAM() is for all ports. It would be good to remove atkObject parameter name.

> Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:113
> +    UNUSED_PARAM(accessible);

ditto.

[Krzysztof Czech]

(In reply to comment #18)
> (In reply to comment #15)
> > > I wonder why EFL port only needs to use HAVE(ACCESSIBILITY) together. EFL port also enabled HAVE(ACCESSIBILITY) guard as GTK port.
> > 
> > Because we wanted to have a possibility of turning Accessibility on/off. This is a matter of having ATK library. In case ATK is not available, we can easily turning it off. See Source/cmake/OptionsEfl.cmake (line 50). Why PLATFORM(EFL) and HAVE(ACCESSIBILITY) go together?. Because PLATFORM(EFL) alone will not cover every occurrence of ATK callings.
> 
> I don't understand this well. OptionsEfl.cmake enables ENABLE_ACCESSIBILITY to find ATK package. 
> 
>  if (ENABLE_ACCESSIBILITY)
>     find_package(ATK REQUIRED)
>  else ()
>     add_definitions(-DHAVE_ACCESSIBILITY=0)
>  endif ()
> 
> Then, the HAVE_ACCESSIBILITY is enabled by Platform.h
> 
>  #if !defined(HAVE_ACCESSIBILITY)
>  #if PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(GTK) ||  (PLATFORM(CHROMIUM) && !OS(ANDROID)) || PLATFORM(EFL)
>  #define HAVE_ACCESSIBILITY 1
>  #endif
>  #endif /* !defined(HAVE_ACCESSIBILITY) */
> 
> It means EFL port will enable HAVE_ACCESSIBILITY if we don't off ENABLE_ACCESSIBILITY in OptionsEfl.cmake. If we off ENABLE_ACCESSIBILITY in OptionsEFl.cmake, HAVE_ACCESSIBILITY is defined with disabled. So, it won't be enabled in Platform.h again. It seems to me that we already can control HAVE_ACCESSIBILITY by OptionsEfl.cmake definition. If I miss something, please let me know.

It's a bit tricky, but you are right. We can control it, by setting HAVE_ACCESSIBILITY in OptionsEfl.cmake. The thing is, other ports have HAVE_ACCESSIBILITY enabled by default:

#if !defined(HAVE_ACCESSIBILITY)
#if PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(GTK) ||  (PLATFORM(CHROMIUM) && !OS(ANDROID)) || PLATFORM(EFL)
#define HAVE_ACCESSIBILITY 1
#endif
#endif /* !defined(HAVE_ACCESSIBILITY) */

There are parts of the code, that had not been covered by HAVE(ACCESSIBILITY) macro example: 

Source/WebKit2/WebProcess/WebPage/WebPage.h
814 #if PLATFORM(MAC)
839 #elif PLATFORM(GTK)
840     GRefPtr<WebPageAccessibilityObject> m_accessibilityObject;

Avoiding adding global HAVE(ACCESSIBILITY), I'm adding it along with PLATFORM(EFL).

Regarding my patch you are right, the are things that can be changed. I will propose a new one.

[Krzysztof Czech]

Created attachment 182733 [details]
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

[kov's GTK+ EWS bot]

Comment on attachment 182733 [details]
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

Attachment 182733 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15860981

[Gyuyoung Kim]

Comment on attachment 182733 [details]
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

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

r- because of GTK build fail and wrong UNUSED_PARAM usage.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:83
> +#include "GRefPtr.h"

#if HAVE(ACCESSIBILITY) && (PLATFORM(GTK) || PLATFORM(EFL)) How about this one ?

> Source/WebKit2/WebProcess/WebPage/WebPage.h:474
> +#if HAVE(ACCESSIBILITY)

ditto ?

> Source/WebKit2/WebProcess/WebPage/WebPage.h:850
> +#elif PLATFORM(GTK) || (PLATFORM(EFL) && HAVE(ACCESSIBILITY))

ditto ?

> Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:78
> +#if PLATFORM(EFL)

As I said in comment #19, WebKit prefers to remove unused parameter. Look at this mailling list. - http://lists.webkit.org/pipermail/webkit-dev/2012-October/022369.html

> Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:112
> +#if PLATFORM(EFL)

ditto.

[Krzysztof Czech]

Created attachment 182952 [details]
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:109:  web_page_accessibility_object_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 6 files


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

[Krzysztof Czech]

(In reply to comment #25)
> Attachment 182952 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:109:  web_page_accessibility_object_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Total errors found: 1 in 6 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

It's a matter of glib's initialization approach. Underscores are needed.

[Krzysztof Czech]

(In reply to comment #23)
> (From update of attachment 182733 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182733&action=review
> 
> r- because of GTK build fail and wrong UNUSED_PARAM usage.
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:83
> > +#include "GRefPtr.h"
> 
> #if HAVE(ACCESSIBILITY) && (PLATFORM(GTK) || PLATFORM(EFL)) How about this one ?
Agree, looks better.
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:474
> > +#if HAVE(ACCESSIBILITY)
> 
> ditto ?
Done.
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:850
> > +#elif PLATFORM(GTK) || (PLATFORM(EFL) && HAVE(ACCESSIBILITY))
> 
> ditto ?
Done.
> 
> > Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:78
> > +#if PLATFORM(EFL)
> 
> As I said in comment #19, WebKit prefers to remove unused parameter. Look at this mailling list. - http://lists.webkit.org/pipermail/webkit-dev/2012-October/022369.html
> 
Thanks for the tip. I removed unused parameters.
> > Source/WebKit2/WebProcess/WebPage/atk/WebPageAccessibilityObjectAtk.cpp:112
> > +#if PLATFORM(EFL)
> 
> ditto.
Done.

[Gyuyoung Kim]

Comment on attachment 182952 [details]
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

Looks good to me.

[Chris Dumez]

Comment on attachment 182952 [details]
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

LGTM.

[Gyuyoung Kim]

Comment on attachment 182952 [details]
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

Benjamin or Martin might want to have a final look.

[Benjamin Poulain]

> Benjamin or Martin might want to have a final look.

I don't know enough about accessibility to have a say here.

[Gyuyoung Kim]

Comment on attachment 182952 [details]
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

According to new WebKit2 review policy, I clear my r+.

[WebKit Review Bot]

Comment on attachment 182952 [details]
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

Clearing flags on attachment: 182952

Committed r140667: <http://trac.webkit.org/changeset/140667>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

It seems that after this patch, we started experiencing Bug 108058.