Bug 120421 – [ATK] Missing WTR AccessibilityUIElement::addNotificationListener implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Denis Nomiyama (dnomi)]

AccessibilityUIElement for WTR in the GTK+ port does not support event notification listeners (AccessilibityNotificationHandler).

[Denis Nomiyama (dnomi)]

Created attachment 211317 [details]
Patch

[EFL EWS Bot]

Comment on attachment 211317 [details]
Patch

Attachment 211317 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1736954

[Denis Nomiyama (dnomi)]

Comment on attachment 211317 [details]
Patch

I will fix the compilation issue on efl-wk2.

[Denis Nomiyama (dnomi)]

Created attachment 211337 [details]
Patch

Fixed compilation problem on efl-wk2.

[chris fleizach]

Comment on attachment 211337 [details]
Patch

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

> Tools/ChangeLog:77
> [phone]  Denis Nomiyama  <[e-mail]>

two change log entries

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:71
> +static gboolean axObjectEventListener(GSignalInvocationHint *signalHint, guint numParamValues, const GValue *paramValues, gpointer data)

* are on wrong side

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:74
> +    if (numParamValues < 1)

this looks like it should be if (!numParamValues) return

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:79
> +        return TRUE;

true instead of TRUE

[Denis Nomiyama (dnomi)]

Comment on attachment 211337 [details]
Patch

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

Thanks a lot for the review and sorry for the mistakes. I will fix them and I'll also apply some of the fixes for the issues found at bug 120669.

>> Tools/ChangeLog:77
>> [phone]  Denis Nomiyama  <[e-mail]>
> 
> two change log entries

Ops. I'll fix it.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:71
>> +static gboolean axObjectEventListener(GSignalInvocationHint *signalHint, guint numParamValues, const GValue *paramValues, gpointer data)
> 
> * are on wrong side

Sure, I'll fix it.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:74
>> +    if (numParamValues < 1)
> 
> this looks like it should be if (!numParamValues) return

Nice one. I'll change it.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:79
>> +        return TRUE;
> 
> true instead of TRUE

Ok I'll fix it.

[Denis Nomiyama (dnomi)]

Created attachment 211366 [details]
Patch

Fixed the patch according to the review comments.

[Mario Sanchez Prada]

Comment on attachment 211366 [details]
Patch

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

Thanks for the patch Denis. It looks good already but still needs some more work. See my review below...

> Tools/ChangeLog:37
> +        * WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:
> +        Added.

I guess in cases like this is better to leave the "Added." in the previous line, even it's long, since it makes it more readable.

> Tools/WebKitTestRunner/CMakeLists.txt:27
> +    ${WEBKIT_TESTRUNNER_DIR}/InjectedBundle/atk
>      ${WEBKIT_TESTRUNNER_DIR}/InjectedBundle/Bindings

Wrong alphabetical order. I believe uppercase 'B' should come before lowercase 'a'

> Tools/WebKitTestRunner/GNUmakefile.am:137
> +	-I$(srcdir)/Tools/WebKitTestRunner/InjectedBundle/atk \

Same here.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:26
> +

You don't need this extra line. See "#include Statements" in http://www.webkit.org/coding/coding-style.html

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:49
> +static guint stateChangeListenerId = 0;
> +static guint focusEventListenerId = 0;
> +static guint activeDescendantChangedListenerId = 0;
> +static guint childrenChangedListenerId = 0;
> +static guint propertyChangedListenerId = 0;
> +static guint visibleDataChangedListenerId = 0;
> +static NotificationHandlersMap notificationHandlers;
> +static AccessibilityNotificationHandler* globalNotificationHandler = 0;
> +static bool loggingAccessibilityEvents = false;
> +
> +static void printAccessibilityEvent(AtkObject* accessible, const gchar* signalName, const gchar* signalValue)

This refactoring might be a good opportunity to move into using unnamed namespaces for this static declarations, which are the recommended alternative in C++

Also, it might be a good opportunity too to move from glib specific types (e.g. guint) into more general types (e.g. unsigned) for things that are not going to be part of public APIs

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:59
> +    const gchar* objectName = atk_object_get_name(accessible);

Following the lead of the comment above, s/gchar/char. Similar considerations in the rest of the code after this point

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:107
> +#if PLATFORM(GTK)

Couldn't this be #if PLATFORM(GTK) || PLATFORM(EFL) ?

I guess the answer is "probably yes" but that it's perhaps better to leave it out of this patch because of consistency with what we did for DRT, since we can't test it really in EFL. However, I think it might be worth to let EFL guys know about it because that bit might help them pass some additional tests as well.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:166
> +#if PLATFORM(GTK)

Same consideration here about EFL

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:197
> +#if PLATFORM(GTK)

And here

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1135
> +

Nit. I would probably remove this blank line.

[Denis Nomiyama (dnomi)]

Comment on attachment 211366 [details]
Patch

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

Many thanks for the review. I'll make the changes.
I have done some extra tests and found a problem with disconnectAccessibilityCallbacks(), which is not returning a value (true) at the end.
Another problem was that connectAccessibilityCallbacks() assumes that notification handlers are added to HashMap after the call. And I modified logAccessibilityEvents() and setNotificationFunctionCallback() to reflect this assumption.

>> Tools/ChangeLog:37
>> +        Added.
> 
> I guess in cases like this is better to leave the "Added." in the previous line, even it's long, since it makes it more readable.

Ok.

>> Tools/WebKitTestRunner/CMakeLists.txt:27
>>      ${WEBKIT_TESTRUNNER_DIR}/InjectedBundle/Bindings
> 
> Wrong alphabetical order. I believe uppercase 'B' should come before lowercase 'a'

Ops. I'll fix it.

>> Tools/WebKitTestRunner/GNUmakefile.am:137
>> +	-I$(srcdir)/Tools/WebKitTestRunner/InjectedBundle/atk \
> 
> Same here.

Ok.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:26
>> +
> 
> You don't need this extra line. See "#include Statements" in http://www.webkit.org/coding/coding-style.html

Ok. I'll fix it.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:49
>> +static void printAccessibilityEvent(AtkObject* accessible, const gchar* signalName, const gchar* signalValue)
> 
> This refactoring might be a good opportunity to move into using unnamed namespaces for this static declarations, which are the recommended alternative in C++
> 
> Also, it might be a good opportunity too to move from glib specific types (e.g. guint) into more general types (e.g. unsigned) for things that are not going to be part of public APIs

Sure, I'll modify them.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:107
>> +#if PLATFORM(GTK)
> 
> Couldn't this be #if PLATFORM(GTK) || PLATFORM(EFL) ?
> 
> I guess the answer is "probably yes" but that it's perhaps better to leave it out of this patch because of consistency with what we did for DRT, since we can't test it really in EFL. However, I think it might be worth to let EFL guys know about it because that bit might help them pass some additional tests as well.

Ok. Search for the respective bug report for EFL or will create one if not there yet.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1135
>> +
> 
> Nit. I would probably remove this blank line.

Ok. I'll remove it.

[Chris Dumez]

Comment on attachment 211366 [details]
Patch

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

> Tools/ChangeLog:3
> +        [GTK] Missing WTR AccessibilityUIElement::addNotificationListener implementation

This patch does not seem GTK-specific. Please use [ATK] flag instead.

[Chris Dumez]

Comment on attachment 211366 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:267
> +#if PLATFORM(GTK) || PLATFORM(EFL)

#if PLATFORM(GTK) || (PLATFORM(EFL) && HAVE(ACCESSIBILITY))

Ditto in the other places.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp:52
> +        m_globalNotificationHandler = adoptRef(new AccessibilityNotificationHandler);

Please use the factory method.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.h:38
> +    AccessibilityNotificationHandler(void);

Please make this constructor private and omit the "void". There is already a create() factory method.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.h:42
> +    GRefPtr<AtkObject> platformElement(void) const { return m_platformElement; }

Ditto for the void

[Denis Nomiyama (dnomi)]

Comment on attachment 211366 [details]
Patch

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

Many thanks for the comments. I will address the issues.

>> Tools/ChangeLog:3
>> +        [GTK] Missing WTR AccessibilityUIElement::addNotificationListener implementation
> 
> This patch does not seem GTK-specific. Please use [ATK] flag instead.

Sure. I have changed the bug title and will update the ChangeLog.

>> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:267
>> +#if PLATFORM(GTK) || PLATFORM(EFL)
> 
> #if PLATFORM(GTK) || (PLATFORM(EFL) && HAVE(ACCESSIBILITY))
> 
> Ditto in the other places.

Ok. I'll fix them.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp:52
>> +        m_globalNotificationHandler = adoptRef(new AccessibilityNotificationHandler);
> 
> Please use the factory method.

Ok. I'll change it.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.h:38
>> +    AccessibilityNotificationHandler(void);
> 
> Please make this constructor private and omit the "void". There is already a create() factory method.

Sure, I'll fix it.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.h:42
>> +    GRefPtr<AtkObject> platformElement(void) const { return m_platformElement; }
> 
> Ditto for the void

Ok.

[Denis Nomiyama (dnomi)]

Created attachment 211426 [details]
Patch

Changed patch according to review comments.

[Mario Sanchez Prada]

(In reply to comment #13)
> Created an attachment (id=211426) [details]
> Patch
> 
> Changed patch according to review comments.

The patch looks good to me, but I'd rather give Christopher a chance to comment on it, and approve it he wants so, before actually going for it.

[Chris Dumez]

Comment on attachment 211426 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp:51
> +    if (m_globalNotificationHandler)

Don't you mean the opposite?

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp:53
> +    m_globalNotificationHandler->logAccessibilityEvents();

Weird that this does not crash

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:85
> +    String notificationName;

Couldn't this be a const char* ?

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:108
> +#if PLATFORM(GTK)

Why is this GTK-specific? Are we missing something in EFL?

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:169
> +#if PLATFORM(GTK)

Why no EFL?

[Denis Nomiyama (dnomi)]

Comment on attachment 211426 [details]
Patch

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

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp:51
>> +    if (m_globalNotificationHandler)
> 
> Don't you mean the opposite?

Ops, yes. I'll fix it.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp:53
>> +    m_globalNotificationHandler->logAccessibilityEvents();
> 
> Weird that this does not crash

The pointer is ok, but a new global handler is created every time the logging is enabled.. I'll fix it.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:85
>> +    String notificationName;
> 
> Couldn't this be a const char* ?

True and it makes the code simpler and quicker. I'll give it a try.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:108
>> +#if PLATFORM(GTK)
> 
> Why is this GTK-specific? Are we missing something in EFL?

I'm not able to test the EFL port to ensure this also works on EFL. I'm planning to raise a bug to let EFL engineers know about it.

[Denis Nomiyama (dnomi)]

Created attachment 211567 [details]
Patch

Modified the patch according to the review comments.

[Mario Sanchez Prada]

Comment on attachment 211567 [details]
Patch

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

Curiously enough, I found a couple of things now that I thing it would be great to fix/improve before landing the patch, which is already great.

Setting r- because of those. See comments below...

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:93
> +        if (!g_strcmp0(g_value_get_string(&paramValues[1]), "invalid-entry"))
> +            notificationName = "AXInvalidStatusChanged";

I think we should also cover the other cases with this patch that are managed by DRT already: CheckedStateChanged, AXLayoutComplete, AXFocusedUIElementChanged and AXValueChanged.

Potentially, that change might help us remove more tests from TestExpectations, or at least help us get closer to that (in case there were additional issues pending to be resolved)

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:577
> +
> +    // In case of 'aria-invalid' when the attribute empty or has "false" for ATK
> +    // according to http://www.w3.org/WAI/PF/aria-implementation/#mapping attribute
> +    // is not mapped but layout tests will expect 'false'.
> +    if (attributeValue.isEmpty() && atkAttributeName == "aria-invalid")
> +        return JSStringCreateWithUTF8CString("false");
> +

This bit is not actually related to this bug, since it just adds the mapping in WKTR to read the exposed value for aria-invalid and has nothing to do with the notifications stuff.

Maybe file a new bug just for it depending on this bug, and move the removal of aria-invalid test from TestExpectations file there?

> LayoutTests/platform/gtk-wk2/TestExpectations:-504
> -# [GTK] Missing WTR AccessibilityUIElement::addNotificationListener implementation
> -webkit.org/b/120421 accessibility/aria-invalid.html [ Timeout ]
> -

As mentioned above, this could be moved to a separate bug if we file one for the aria-invalid mapping thing

[Denis Nomiyama (dnomi)]

Comment on attachment 211567 [details]
Patch

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

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:93
>> +            notificationName = "AXInvalidStatusChanged";
> 
> I think we should also cover the other cases with this patch that are managed by DRT already: CheckedStateChanged, AXLayoutComplete, AXFocusedUIElementChanged and AXValueChanged.
> 
> Potentially, that change might help us remove more tests from TestExpectations, or at least help us get closer to that (in case there were additional issues pending to be resolved)

Sure, good point. I'll add the other cases.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:577
>> +
> 
> This bit is not actually related to this bug, since it just adds the mapping in WKTR to read the exposed value for aria-invalid and has nothing to do with the notifications stuff.
> 
> Maybe file a new bug just for it depending on this bug, and move the removal of aria-invalid test from TestExpectations file there?

Ok. I've raised another bug for it (bug 121668)

>> LayoutTests/platform/gtk-wk2/TestExpectations:-504
>> -
> 
> As mentioned above, this could be moved to a separate bug if we file one for the aria-invalid mapping thing

Ok.

[Denis Nomiyama (dnomi)]

Created attachment 212155 [details]
Patch

Modified patch according to review comments.

[Mario Sanchez Prada]

Comment on attachment 212155 [details]
Patch

Thanks

[WebKit Commit Bot]

Comment on attachment 212155 [details]
Patch

Clearing flags on attachment: 212155

Committed r156164: <http://trac.webkit.org/changeset/156164>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.