Bug 137819 – [EFL] Introduce EFL Accessibility in WebKit

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Krzysztof Czech]

EFL Accessibility delivers set of APIs that are used to notify clients that specific actions occurs. In general notifications are sent through XWindow and each client (WebKit) is able to register itself on specific accessibility events and do some actions on accessibility tree.

[Krzysztof Czech]

Created attachment 240014 [details]
proposed patch

[Krzysztof Czech]

Created attachment 240015 [details]
proposed patch

[Krzysztof Czech]

This patch only stabs out required API and delivers event handler on specific accessibility notifications.

[Chris Dumez]

Comment on attachment 240015 [details]
proposed patch

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

Just a couple of drive-by comments.

> Source/WebKit2/UIProcess/API/efl/WebAccessibility.cpp:20
> +    , m_eventHandler(0)

m_eventHandler(ecore_event_handler_add(ECORE_X_EVENT_CLIENT_MESSAGE, eventHandler, this)) ?

> Source/WebKit2/UIProcess/API/efl/WebAccessibility.cpp:27
> +    if (m_eventHandler)

Does this check ever fail? m_eventHandler is initialized in the constructor and never reset elsewhere.

[Gyuyoung Kim]

Comment on attachment 240015 [details]
proposed patch

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

Basically this patch looks good. However I need to understand whole behavior new AX EFL APIs do. Let me take a look again. r- due to Chris and my comments.

> Source/WebKit2/UIProcess/API/efl/WebAccessibility.cpp:1
> +#include "config.h"

Missing License.

> Source/WebKit2/UIProcess/API/efl/WebAccessibility.cpp:36
> +

Please use #ifdef HAVE_ECORE_X macro for Ecore_X

> Source/WebKit2/UIProcess/API/efl/WebAccessibility.h:1
> +#ifndef WebAccessibility_h

Missing License.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:1037
> +EAPI Eina_Bool ewk_view_accessibility_action_next_get(const Evas_Object* o);

Wrong * place.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:1046
> +EAPI Eina_Bool ewk_view_accessibility_action_prev_get(const Evas_Object* o);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:1055
> +EAPI Eina_Bool ewk_view_accessibility_action_read_by_point_get(const Evas_Object* o);

ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_accessibility.cpp:1
> +#include "config.h"

Missing License.

[Krzysztof Czech]

(In reply to comment #4)
> Comment on attachment 240015 [details]
> proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240015&action=review
> 
> Just a couple of drive-by comments.
> 
> > Source/WebKit2/UIProcess/API/efl/WebAccessibility.cpp:20
> > +    , m_eventHandler(0)
> 
> m_eventHandler(ecore_event_handler_add(ECORE_X_EVENT_CLIENT_MESSAGE,
> eventHandler, this)) ?
Yes, right
> 
> > Source/WebKit2/UIProcess/API/efl/WebAccessibility.cpp:27
> > +    if (m_eventHandler)
> 
> Does this check ever fail? m_eventHandler is initialized in the constructor
> and never reset elsewhere.
Does not suppose to fail.

Thanks Chris.

[Krzysztof Czech]

(In reply to comment #5)
> Comment on attachment 240015 [details]
> proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240015&action=review
> 
> Basically this patch looks good. However I need to understand whole behavior
> new AX EFL APIs do. Let me take a look again. r- due to Chris and my
> comments.
> 
Let me put some light on this. Basically the simplest test case could look as follows. Take into account accessibility is rather for someone with disabilities to help utilize an application in terms of proper navigation through UI interface, but this is not a rule. For example we have a Browser and additional application that is able to read gestures or do some navigation. Now comes the EFL and accessibility API. Whenever there is a need to notify clients about some specific action that has just happened for example (go to the next element on a web page or next element in UI) ecore_x_e_illume_access_action_..... is sent. All the clients that now how to interpret it, do this in its own context.

[Krzysztof Czech]

> > Basically this patch looks good. However I need to understand whole behavior
> > new AX EFL APIs do. Let me take a look again. r- due to Chris and my
> > comments.
> > 
> Let me put some light on this. Basically the simplest test case could look
> as follows. Take into account accessibility is rather for someone with
> disabilities to help utilize an application in terms of proper navigation
> through UI interface, but this is not a rule. For example we have a Browser
> and additional application that is able to read gestures or do some
> navigation. Now comes the EFL and accessibility API. Whenever there is a
> need to notify clients about some specific action that has just happened for
> example (go to the next element on a web page or next element in UI)
> ecore_x_e_illume_access_action_..... is sent. All the clients that now how
> to interpret it, do this in its own context.

I provided a test case test_ewk2_accessibility and you can think of it as such an application that is able to recognize gestures or do some navigation.

[Krzysztof Czech]

Created attachment 240117 [details]
patch

[Krzysztof Czech]

Created attachment 240118 [details]
patch

[Gyuyoung Kim]

Comment on attachment 240118 [details]
patch

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

> Source/WebKit2/ChangeLog:48
> +        (WebKit::WebPageProxy::accessibilityObjectReadByPoint):

I think you need to explain why you add these functions.

> Source/WebKit2/UIProcess/API/efl/WebAccessibility.cpp:109
> +{

notImplemented() ?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_accessibility.cpp:49
> +    ASSERT_TRUE(ewk_view_accessibility_action_read_by_point_get(webView()));

I wonder how ewk_view_accessibility_action_read_by_point_get() can be true. Because eventually this function will return m_readAction. However m_readAction can be true by accessibilityObjectReadByPoint() though, accessibilityObjectReadByPoint() isn't implemented. Other functions looks same. 

m_readAction = m_ewkView->page()->accessibilityObjectReadByPoint(WebCore::IntPoint(m_currentPoint.x(), m_currentPoint.y()));

> Source/WebKit2/UIProcess/efl/WebPageProxyEfl.cpp:145
> +bool WebPageProxy::accessibilityObjectReadByPoint(const WebCore::IntPoint& point)

*point* argument will cause unused param build warning.

[Krzysztof Czech]

(In reply to comment #11)
> Comment on attachment 240118 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240118&action=review
> 
> > Source/WebKit2/ChangeLog:48
> > +        (WebKit::WebPageProxy::accessibilityObjectReadByPoint):
> 
> I think you need to explain why you add these functions.
Well it's not a final patch. These are only required empty stubs that will form a whole functionality. Basically those functions are needed to have a connection with WebProcess. WebProcess part it not ready yet. I wanted to split it into two parts or three parts.
> 
> > Source/WebKit2/UIProcess/API/efl/WebAccessibility.cpp:109
> > +{
> 
> notImplemented() ?
> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_accessibility.cpp:49
> > +    ASSERT_TRUE(ewk_view_accessibility_action_read_by_point_get(webView()));
> 
> I wonder how ewk_view_accessibility_action_read_by_point_get() can be true.
> Because eventually this function will return m_readAction. However
> m_readAction can be true by accessibilityObjectReadByPoint() though,
> accessibilityObjectReadByPoint() isn't implemented. Other functions looks
> same. 
> 
It will be failing until I make a WebProcess part.
> m_readAction =
> m_ewkView->page()->accessibilityObjectReadByPoint(WebCore::
> IntPoint(m_currentPoint.x(), m_currentPoint.y()));
> 
> > Source/WebKit2/UIProcess/efl/WebPageProxyEfl.cpp:145
> > +bool WebPageProxy::accessibilityObjectReadByPoint(const WebCore::IntPoint& point)
> 
> *point* argument will cause unused param build warning.

[Krzysztof Czech]

> > I wonder how ewk_view_accessibility_action_read_by_point_get() can be true.
> > Because eventually this function will return m_readAction. However
> > m_readAction can be true by accessibilityObjectReadByPoint() though,
> > accessibilityObjectReadByPoint() isn't implemented. Other functions looks
> > same. 
> > 
> It will be failing until I make a WebProcess part.
To be more clear. WebProcess part will be doing navigation across Accessibility Tree. Basically this means when ECORE_X_ATOM_E_ILLUME_ACCESS_ACTION_READ_NEXT happens we'll focus next element. What may be the result of focusing next element. Well, screen reader may read its content/description/role/name to the user. Ewk API will be getting a status whether for example focusing next element is properly done and test_ewk2_accessibility will be testing it. I already mentioned test_ewk2_accessibility triggering ecore_x_e_illume_... actions and read results. In real scenario screen reader is a target that should be able to speak aloud results (content/description/name) or do something with a role.

[Gyuyoung Kim]

(In reply to comment #12)
> (In reply to comment #11)
> > Comment on attachment 240118 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=240118&action=review
> > 
> > > Source/WebKit2/ChangeLog:48
> > > +        (WebKit::WebPageProxy::accessibilityObjectReadByPoint):
> > 
> > I think you need to explain why you add these functions.
> Well it's not a final patch. These are only required empty stubs that will
> form a whole functionality. Basically those functions are needed to have a
> connection with WebProcess. WebProcess part it not ready yet. I wanted to
> split it into two parts or three parts.

Aha, I missed test_ewk2_accessibility isn't included in API test execution list.

> > > Source/WebKit2/UIProcess/API/efl/WebAccessibility.cpp:109
> > > +{
> > 
> > notImplemented() ?
> > 
> > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_accessibility.cpp:49
> > > +    ASSERT_TRUE(ewk_view_accessibility_action_read_by_point_get(webView()));
> > 
> > I wonder how ewk_view_accessibility_action_read_by_point_get() can be true.
> > Because eventually this function will return m_readAction. However
> > m_readAction can be true by accessibilityObjectReadByPoint() though,
> > accessibilityObjectReadByPoint() isn't implemented. Other functions looks
> > same. 
> > 
> It will be failing until I make a WebProcess part.
> > m_readAction =
> > m_ewkView->page()->accessibilityObjectReadByPoint(WebCore::
> > IntPoint(m_currentPoint.x(), m_currentPoint.y()));
> > 
> > > Source/WebKit2/UIProcess/efl/WebPageProxyEfl.cpp:145
> > > +bool WebPageProxy::accessibilityObjectReadByPoint(const WebCore::IntPoint& point)
> > 
> > *point* argument will cause unused param build warning.

[Gyuyoung Kim]

Comment on attachment 240118 [details]
patch

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

LGTM.

>>> Source/WebKit2/UIProcess/efl/WebPageProxyEfl.cpp:145
>>> +bool WebPageProxy::accessibilityObjectReadByPoint(const WebCore::IntPoint& point)
>> 
>> *point* argument will cause unused param build warning.
> 
> 

Please land after removing "point" argument.

[Krzysztof Czech]

Created attachment 240273 [details]
attempt to check bots

[Krzysztof Czech]

Comment on attachment 240273 [details]
attempt to check bots

Ready to land

[WebKit Commit Bot]

Comment on attachment 240273 [details]
attempt to check bots

Clearing flags on attachment: 240273

Committed r175098: <http://trac.webkit.org/changeset/175098>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.