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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=121593


Mario Sanchez Prada <mario at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #215163|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #2 from Mario Sanchez Prada <mario at webkit.org>  2013-10-25 05:34:03 PST ---
(From update of attachment 215163)
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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.