Bug 106216

Summary:

[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.

Product:

WebKit

Reporter:

Krzysztof Czech <k.czech>

Component:

WebKit EFL

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, cdumez, cfleizach, gtk-ews, gyuyoung.kim, laszlo.gombos, lucas.de.marchi, mrobinson, philn, rakuco, rniwa, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Linux

Bug Depends on:

106447, 108058

Bug Blocks:

Attachments:

Description	Flags
[EFL] Expose accessibility hierarchy in WebKit2-EFL.	eflews.bot: commit-queue-
[EFL] Expose accessibility hierarchy in WebKit2-EFL.	gtk-ews: commit-queue-
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.	none
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.	none
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.	none
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.	none
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.	gyuyoung.kim: review-, gtk-ews: commit-queue-
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL.	none

Description Krzysztof Czech 2013-01-07 06:25:19 PST

Create WebPage's accessibility object on WebPage initialization.

Comment 1 Krzysztof Czech 2013-01-07 06:28:54 PST

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

Comment 2 EFL EWS Bot 2013-01-07 06:37:01 PST

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

Comment 3 kov's GTK+ EWS bot 2013-01-07 06:37:39 PST

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

Comment 4 Krzysztof Czech 2013-01-07 07:51:10 PST

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

Comment 5 kov's GTK+ EWS bot 2013-01-07 07:59:11 PST

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

Comment 6 Gyuyoung Kim 2013-01-07 20:09:37 PST

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

Comment 7 Krzysztof Czech 2013-01-08 02:14:54 PST

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

Comment 8 Gyuyoung Kim 2013-01-08 21:24:08 PST

(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.

Comment 9 Krzysztof Czech 2013-01-09 01:11:31 PST

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.

Comment 10 Gyuyoung Kim 2013-01-09 05:14:59 PST

(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

Comment 11 Krzysztof Czech 2013-01-09 07:50:35 PST

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

Comment 12 Krzysztof Czech 2013-01-11 02:46:27 PST

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

Comment 13 Krzysztof Czech 2013-01-14 04:55:25 PST

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

Comment 14 Gyuyoung Kim 2013-01-14 18:18:40 PST

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 ?

Comment 15 Krzysztof Czech 2013-01-15 01:55:10 PST

> 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 ?

Comment 16 Krzysztof Czech 2013-01-15 02:01:34 PST

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

Comment 17 Krzysztof Czech 2013-01-15 02:02:11 PST

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

Comment 18 Gyuyoung Kim 2013-01-15 02:37:56 PST

(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.

Comment 19 Gyuyoung Kim 2013-01-15 02:43:03 PST

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.

Comment 20 Krzysztof Czech 2013-01-15 03:42:44 PST

(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.

Comment 21 Krzysztof Czech 2013-01-15 04:00:03 PST

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

Comment 22 kov's GTK+ EWS bot 2013-01-15 08:22:40 PST

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

Comment 23 Gyuyoung Kim 2013-01-15 16:49:00 PST

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.

Comment 24 Krzysztof Czech 2013-01-16 03:09:47 PST

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

Comment 25 WebKit Review Bot 2013-01-16 03:11:47 PST

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.

Comment 26 Krzysztof Czech 2013-01-16 03:29:46 PST


(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.

Comment 27 Krzysztof Czech 2013-01-16 03:30:47 PST

(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.

Comment 28 Gyuyoung Kim 2013-01-17 00:48:25 PST

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

Looks good to me.

Comment 29 Chris Dumez 2013-01-17 00:51:00 PST

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

LGTM.

Comment 30 Gyuyoung Kim 2013-01-17 21:31:33 PST

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

Benjamin or Martin might want to have a final look.

Comment 31 Benjamin Poulain 2013-01-21 12:32:23 PST

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

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

Comment 32 Gyuyoung Kim 2013-01-21 22:45:03 PST

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

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

Comment 33 WebKit Review Bot 2013-01-24 02:13:21 PST

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>

Comment 34 WebKit Review Bot 2013-01-24 02:13:27 PST

All reviewed patches have been landed.  Closing bug.

Comment 35 Chris Dumez 2013-01-28 04:14:08 PST

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