Bug 105638 – [GTK] accessibility/aria-labelledby-overrides-label.html requires a proper baseline

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zan Dobersek]

The accessibility/aria-labelledby-overrides-label.html layout test has been added in r138175 and is missing the platform-specific baseline.
http://trac.webkit.org/changeset/138175

This is the current output, seems to indicate a test failure:
Shut down computer after  minutes
This tests that if aria-labelledby is used, then label elements are not used

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


PASS text.description is 'AXDescription: Shut down computer after 10 minutes'
FAIL text.titleUIElement() != null && text.titleUIElement().isValid should be false. Was true.
Label element role is: AXRole: AXUnknown
PASS successfullyParsed is true

TEST COMPLETE

[Joanmarie Diggs]

Created attachment 180760 [details]
Patch

[Martin Robinson]

Comment on attachment 180760 [details]
Patch

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

So I really think we should be unskipping these tests for WebKit2 as well. Perhaps we can make the fix general enough for both ports?

> Tools/DumpRenderTree/gtk/AccessibilityControllerGtk.cpp:79
> +    AtkObject* accessible = DumpRenderTreeSupportGtk::accessibleElementById(mainFrame, id);
> +    if (!accessible)
> +        return 0;
> +
> +    return AccessibilityUIElement(accessible);

Hrm. I'm not certain, but maybe one way to make this work for WebKit1 and WebKit2 (instead of using DumpRenderTree support) would be to embed an atk text attribute into the object and just walk the tree.

[Dominic Mazzoni]

Comment on attachment 180760 [details]
Patch

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

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:308
> +AtkObject* DumpRenderTreeSupportGtk::accessibleElementById(WebKitWebFrame* frame, JSStringRef id)

This implementation isn't exactly incorrect, but it's not what I intended - the way I implemented this for Mac and Chromium was to walk the accessibility tree from the top until we reach one with the right id. That way:

* It makes sure the element really is in the tree, reachable from the root
* It makes sure the element isn't ignored - I could be wrong, but I think this might return an ignored element (which can still have an AccessibleObject, but wouldn't be in the tree)
* It creates accessible objects for all other elements along the way - a side-effect that I know at least one of the tests might depend on
* Finally, the overall spirit of the tests is to test the external API - to just query the accessible tree directly - rather than reaching behind the scenes to convert a DOM node into an accessible object (something a screen reader could never do)

>> Tools/DumpRenderTree/gtk/AccessibilityControllerGtk.cpp:79
>> +    return AccessibilityUIElement(accessible);
> 
> Hrm. I'm not certain, but maybe one way to make this work for WebKit1 and WebKit2 (instead of using DumpRenderTree support) would be to embed an atk text attribute into the object and just walk the tree.

Yep, a side benefit is that then it should work for both WK1 and WK2.

[Joanmarie Diggs]

(In reply to comment #3)
> (From update of attachment 180760 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180760&action=review
> 
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:308
> > +AtkObject* DumpRenderTreeSupportGtk::accessibleElementById(WebKitWebFrame* frame, JSStringRef id)
> 
> This implementation isn't exactly incorrect, but it's not what I intended - the way I implemented this for Mac and Chromium was to walk the accessibility tree from the top until we reach one with the right id.

My bad. I didn't get that intent from the method name. I'm happy to walk the tree for the reasons you specify and will do so.

I'm less jazzed about adding an AtkObject Attribute whose purpose is not to support accessibility but to make it possible to run tests in WK2. ;) So if it is possible, I'd prefer to not go that route. For instance, walk the accessible tree, doing an equality check for the accessible object resulting from the DOM node conversion. Would that be a BadIdea(tm)? If so, why? If not, is it something that you just cannot do in WK2?

Apologies if my questions are dumb. I know I really need to get up to speed on it and will make some time to do so. In the meantime, thanks in advance for shedding some light on the above. :)

[Joanmarie Diggs]

Comment on attachment 180760 [details]
Patch

So in chatting with Martin about this, I have learned that I cannot get an element by id in the UIProcess. Drat!

I continue to not like the idea of exposing something **which no assistive technology needs** (namely the HTML element's ID) as an accessible attribute. That just feels like a gross hack to me.

By the same token, the method in question seems to be used by a sufficient number of tests that the greater evil is the skippage. So patch with gross hack coming up.

[Joanmarie Diggs]

Created attachment 180832 [details]
Patch

[Joanmarie Diggs]

Comment on attachment 180832 [details]
Patch

I hadn't flagged this for review because I wanted to see if the EWS would spit up. EWS seems happy, so reviews would be appreciated. TIA!

[Dominic Mazzoni]

Comment on attachment 180832 [details]
Patch

Unofficial review, looks good.

View in context: https://bugs.webkit.org/attachment.cgi?id=180832&action=review
> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:451
> +    // cannot be done from the UIProcess. Assistive technologies have no need

FWIW, Firefox has been exposing the "id" attribute to assistive technology on Windows for a while, it's documented here:

https://developer.mozilla.org/en-US/docs/Accessibility/AT-APIs/Gecko/Attrs

So maybe it's not *that* much of a hack. I totally understand your concern - one should be cautious about exposing interfaces that are only intended for testing.

I think the benefits outweigh the concerns in this case, still - because 'id' is used as an accessibility attribute on some platforms, and also because we really needed some mechanism to uniquely identify an accessible node, in a cross-platform way, without using an attribute that could possibly modify the accessible tree!

Still, would it be possible to only expose this attribute when used for testing? Maybe DRT/WKTR could call some global function to enable unit testing support in WebKit/ATK, that way this extra attribute wouldn't be exposed under normal circumstances in a web browser?

[chris fleizach]

Comment on attachment 180832 [details]
Patch

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

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:455
> +        String id = toElement(node)->getIdAttribute().string();

I agree. AccessibilityObject should have a idAttribute() method on it that you can call. It's pretty gnarly for you to check the node of the core object

[WebKit Review Bot]

Comment on attachment 180832 [details]
Patch

Clearing flags on attachment: 180832

Committed r139250: <http://trac.webkit.org/changeset/139250>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.