Bug 121593 – REGRESSION(r155543): accessibility/radio-button-title-label.html is failing on GTK

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zan Dobersek]

The accessibility/radio-button-title-label.html layout test is failing on the GTK port since r155543.
http://trac.webkit.org/changeset/155543

http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=accessibility%2Fradio-button-title-label.html

Diff:
--- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/accessibility/radio-button-title-label-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/accessibility/radio-button-title-label-actual.txt
@@ -3,7 +3,7 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS radio1.title is 'AXTitle: '
+FAIL radio1.title should be AXTitle: . Was AXTitle: LABEL.
 PASS titleUIElement.isEqual(accessibilityController.accessibleElementById('label1')) is true
 PASS radio2.description is 'AXDescription: LABEL2a'
 PASS radio2.title is 'AXTitle: '

[Krzysztof Wolanski]

Created attachment 215163 [details]
proposed patch

[Mario Sanchez Prada]

Comment on attachment 215163 [details]
proposed patch

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

> Source/WebCore/accessibility/atk/WebKitAccessibleUtil.cpp:-117
> -
> -        // FIXME: The title tag is used in certain cases for the title. This usage should
> -        // probably be in the description field since it's not "visible".
> -        if (text.textSource == TitleTagText && !titleTagShouldBeUsedInDescriptionField(coreObject))
> -            return text.text;

I'm not sure this is correct. If any, I think the change here should probably be to make it more similar to what the Mac wrapper currently does, which also runs away from using titleTagShouldBeUsedInDescriptionField() to make this decision, but also do some other things.

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:131
> -            if (ATK_IS_TEXT(atkObject))
> +            // If the embedded control is a text field, use its value.
> +            if (ATK_IS_TEXT(atkObject) && coreObject->isTextControl())

This is not correct, and I think Joanie will agree with me too:

The problem is that here you are exposing the text inside the text control (that might be a text area, for instance, with a lot of text) as the name of the AtkObject, and that's not what ATs will expect at all for this cases. What ATs would expect here is more something like "if there's a explicit label for this control (aria-label, another element labelling it...) use it for the name, otherwise just don't put a name". And what this change does is a step more in the wrong direction, since the current situation is wrong anyway.

Yes, I know that's what the W3C says in 2B, but it's definitely what ATK based ATs expect (embedded objects should be represented with the "object replacement character") and in any case that 2B feature is marked at risk, so it's not 100% clear that's the right thing to do anyway.

Btw, this issue was already in my radar after a conversation with Joanie, but I was waiting for bug 123153 to get fixed first before changing that, since that will probably impact it.

But in any case, if we wanted to move closer to the right direction already now, I would do something more like this:

--- a/Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp
+++ b/Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp
@@ -131,6 +131,10 @@ static const gchar* webkitAccessibleGetName(AtkObject* object)
             return atk_text_get_text(ATK_TEXT(atkObject), 0, -1);
     }
 
+   // We don't return the text inside a text control as its name ever.
+   if (coreObject->isTextControl())
+       return 0;
+
    // Try text under the node.
    String textUnder = coreObject->textUnderElement();
    if (textUnder.length())

    [...]


Anyway, there's definitely an issue here and I still think this patch should be fixed. Could you investigate whether the suggestions I commented above could help passing this test too?

Thanks!

[Diego Pino]

There are no references to this bug in any TestExpectations. It's probable this bug was solved at some point but it wasn't marked as closed. I'm closing this bug now. If you think this bug report is still valid, please reopen it and add an entry to TestExpectations.