[webkit-reviews] review denied: [Bug 120421] [ATK] Missing WTR AccessibilityUIElement::addNotificationListener implementation : [Attachment 211567] Patch

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has denied Denis Nomiyama (dnomi)
<d.nomiyama at samsung.com>'s request for review:
Bug 120421: [ATK] Missing WTR AccessibilityUIElement::addNotificationListener
implementation
https://bugs.webkit.org/show_bug.cgi?id=120421

Attachment 211567: Patch
https://bugs.webkit.org/attachment.cgi?id=211567&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
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.c
pp: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