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

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

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

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	WebKit EFL (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Linux

Importance:	P2 Normal
Assignee:	Nobody

URL:
Keywords:

Depends on:	106447 108058
Blocks:
	Show dependency tree / graph

Reported:	2013-01-07 06:25 PST by Krzysztof Czech
Modified:	2013-01-28 04:14 PST (History)
CC List:	13 users (show)

See Also:

Attachments
[EFL] Expose accessibility hierarchy in WebKit2-EFL. (5.14 KB, patch) 2013-01-07 06:28 PST, Krzysztof Czech	eflews.bot: commit-queue-	Details \| Formatted Diff \| Diff
[EFL] Expose accessibility hierarchy in WebKit2-EFL. (6.20 KB, patch) 2013-01-07 07:51 PST, Krzysztof Czech	gtk-ews: commit-queue-	Details \| Formatted Diff \| Diff
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL. (6.47 KB, patch) 2013-01-09 07:50 PST, Krzysztof Czech	no flags	Details \| Formatted Diff \| Diff
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL. (6.47 KB, patch) 2013-01-11 02:46 PST, Krzysztof Czech	no flags	Details \| Formatted Diff \| Diff
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL. (6.41 KB, patch) 2013-01-14 04:55 PST, Krzysztof Czech	no flags	Details \| Formatted Diff \| Diff
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL. (6.35 KB, patch) 2013-01-15 02:01 PST, Krzysztof Czech	no flags	Details \| Formatted Diff \| Diff
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL. (6.24 KB, patch) 2013-01-15 04:00 PST, Krzysztof Czech	gyuyoung.kim: review- gtk-ews: commit-queue-	Details \| Formatted Diff \| Diff
[EFL][WK2] Expose accessibility hierarchy in WebKit2-EFL. (6.25 KB, patch) 2013-01-16 03:09 PST, Krzysztof Czech	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (7) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

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.