Bug 98350

Summary: [GTK] accessibility/aria-invalid.html times out
Product: WebKit Reporter: Zan Dobersek <zan>
Component: WebKitGTKAssignee: Anton Obzhirov <obzhirov>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, d.nomiyama, jdiggs, mario, obzhirov
Priority: P2 Keywords: Gtk, LayoutTestFailure
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 119883, 120416, 120421    
Bug Blocks: 98347    
Attachments:
Description Flags
WIP patch
none
Patch
none
Patch none

Comment 1 Anton Obzhirov 2013-08-15 06:35:12 PDT
Created attachment 208804 [details]
WIP patch

It depends on addNotificationListener implementation.
Comment 2 Mario Sanchez Prada 2013-08-15 07:06:05 PDT
Comment on attachment 208804 [details]
WIP patch

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

> LayoutTests/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * platform/gtk/TestExpectations:

You need a small description here

> Source/WebCore/ChangeLog:13
> +        * accessibility/atk/AXObjectCacheAtk.cpp:
> +        (WebCore::AXObjectCache::postPlatformNotification):
> +        * accessibility/atk/WebKitAccessibleWrapperAtk.cpp:
> +        (webkitAccessibleGetAttributes):

I know not everyone does it (even though it's in the guidelines), but I normally like to have shorts descriptions for every function touched in the ChangeLog.

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:169
> +    } else if (notification == AXInvalidStatusChanged) {
> +        g_signal_emit_by_name(axObject, "state-change", "invalid-entry", coreObject->invalidStatus() != "false");
>      }

You do not need { } if it's a one line branch

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:898
> +
> +    // 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");
> +

You probably need to add this aria-invalid specific mapping to WKTR too
Comment 3 Anton Obzhirov 2013-08-16 03:18:48 PDT
Comment on attachment 208804 [details]
WIP patch

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

>> LayoutTests/ChangeLog:7
>> +        * platform/gtk/TestExpectations:
> 
> You need a small description here

ok

>> Source/WebCore/ChangeLog:13
>> +        (webkitAccessibleGetAttributes):
> 
> I know not everyone does it (even though it's in the guidelines), but I normally like to have shorts descriptions for every function touched in the ChangeLog.

ok

>> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:169
>>      }
> 
> You do not need { } if it's a one line branch

ok

>> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:898
>> +
> 
> You probably need to add this aria-invalid specific mapping to WKTR too

will do
Comment 4 Anton Obzhirov 2013-08-29 08:03:46 PDT
Created attachment 209982 [details]
Patch
Comment 5 Mario Sanchez Prada 2013-09-02 07:10:49 PDT
Comment on attachment 209982 [details]
Patch

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

(In reply to comment #4)
> Created an attachment (id=209982) [details]
> Patch

Looks good to me now (and after SVN revision r154697 the addNotificationListener implementation is no longer an issue in DRT. However, I believe you will need to keep the aria-invalid test skipped for WK2 only since we still don't have addNotificationListener for AccessibilityUIElement in WKTR, hence it will still timeout there.

> LayoutTests/platform/gtk/TestExpectations:-783
> -webkit.org/b/98350 accessibility/aria-invalid.html [ Timeout ]

I would suggest to move this line to the TestExpectations file in gtk-wk2 and reference it against bug 120421 (which tracks the missing implementation of addNotificationListner in WKTR
Comment 6 Anton Obzhirov 2013-09-02 07:59:47 PDT
Created attachment 210286 [details]
Patch
Comment 7 Mario Sanchez Prada 2013-09-02 08:13:00 PDT
Comment on attachment 210286 [details]
Patch

Thanks for the quick response. LGTM now.
Comment 8 WebKit Commit Bot 2013-09-02 08:43:26 PDT
Comment on attachment 210286 [details]
Patch

Clearing flags on attachment: 210286

Committed r154960: <http://trac.webkit.org/changeset/154960>
Comment 9 WebKit Commit Bot 2013-09-02 08:43:29 PDT
All reviewed patches have been landed.  Closing bug.