Bug 118966 – [ATK] Implement allAttributes() for AccessibilityUIElement

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

Implementing this function would help to run properly the following tests, now in TestExpectations:

  accessibility/table-detection.html
  accessibility/table-one-cell.html
  accessibility/table-with-rules.html
  accessibility/th-as-title-ui.html
  accessibility/transformed-element.html

Additionally, it should help to to provide more accurate (and useful!) results for the following tests:

  deleting-iframe-destroys-axcache.html
  image-link-inline-cont.html
  image-link.html
  non-data-table-cell-title-ui-element.html
  table-cell-spans.html
  table-cells.html

[Mario Sanchez Prada]

Created attachment 207521 [details]
Patch proposal

This patch implements the missing functionality in DRT & WKTR, removes four tests from TestExpectations and updates expectations for 3 more tests.

Please review :)

[Mario Sanchez Prada]

Adding to CC people that might be interested.

[Mario Sanchez Prada]

*** Bug 98381 has been marked as a duplicate of this bug. ***

[Gustavo Noronha (kov)]

Comment on attachment 207521 [details]
Patch proposal

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

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:76
> +    GString* str = g_string_new(0);

Might be worth it taking advantage of the change to use StringBuilder and String here =)

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:241
> +        if (parentName && g_utf8_strlen(parentName, -1)) {
> +            attributeLine.set(g_strdup_printf(": %s", parentName));
> +
> +        }

No need for braces, empty line

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:257
> +        attributeLine.set(g_strdup_printf("%s\n", title.utf8().data()));

How about using String::format("%s\n", title.utf8().data())? Then you can have attributeLine live on the stack and avoid dealing with char pointers.

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:303
> +    // We append the ATK specific attributes as a single line at the end.
> +    GOwnPtr<char> attributeData(getAtkAttributeSetAsString(element->platformUIElement()));
> +    attributeLine.set(g_strdup_printf("AXPlatformAttributes: %s", attributeData.get()));
> +    builder.append(attributeLine.get());

Ditto.

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:493
> -    String roleString = roleToString(role);
> -    return JSStringCreateWithUTF8CString(roleString.utf8().data());
> +    GOwnPtr<char> roleStringWithPrefix(g_strdup_printf("AXRole: %s", roleToString(role)));
> +    return JSStringCreateWithUTF8CString(roleStringWithPrefix.get());

Ditto.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:292
> +    attributeLine.set(g_strdup_printf("%s\n", element->role()->string().utf8().data()));
> +    builder.append(attributeLine.get());

This file also has a number of potential changes from char* to String.

[Mario Sanchez Prada]

Thanks for the review, Gustavo. See some comments below...

(In reply to comment #4)
> (From update of attachment 207521 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207521&action=review
> 
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:76
> > +    GString* str = g_string_new(0);
> 
> Might be worth it taking advantage of the change to use StringBuilder and String here =)

Sure. Changed.

> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:241
> > +        if (parentName && g_utf8_strlen(parentName, -1)) {
> > +            attributeLine.set(g_strdup_printf(": %s", parentName));
> > +
> > +        }
> 
> No need for braces, empty line

Ok.

> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:257
> > +        attributeLine.set(g_strdup_printf("%s\n", title.utf8().data()));
> 
> How about using String::format("%s\n", title.utf8().data())? Then you can have attributeLine live on the stack and avoid dealing with char pointers.

Ok. Changed the patch to do that.

> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:303
> > +    // We append the ATK specific attributes as a single line at the end.
> > +    GOwnPtr<char> attributeData(getAtkAttributeSetAsString(element->platformUIElement()));
> > +    attributeLine.set(g_strdup_printf("AXPlatformAttributes: %s", attributeData.get()));
> > +    builder.append(attributeLine.get());
> 
> Ditto.

Done.

> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:493
> > -    String roleString = roleToString(role);
> > -    return JSStringCreateWithUTF8CString(roleString.utf8().data());
> > +    GOwnPtr<char> roleStringWithPrefix(g_strdup_printf("AXRole: %s", roleToString(role)));
> > +    return JSStringCreateWithUTF8CString(roleStringWithPrefix.get());
> 
> Ditto.

Done.
 
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:292
> > +    attributeLine.set(g_strdup_printf("%s\n", element->role()->string().utf8().data()));
> > +    builder.append(attributeLine.get());
> 
> This file also has a number of potential changes from char* to String.

I agree with that, but that's probably something better to tackle in different patches.

[Mario Sanchez Prada]

Committed r153432: <http://trac.webkit.org/changeset/153432>

[Mario Sanchez Prada]

EFL guys might be interested in rebaselining their tests (and maybe unskipping some too), now we are dumping some extra information

[Krzysztof Czech]

> EFL guys might be interested in rebaselining their tests (and maybe unskipping some too), now we are dumping some extra information

Thanks Mario for information. I will take a look at it.