[webkit-reviews] review denied: [Bug 121593] REGRESSION(r155543): accessibility/radio-button-title-label.html is failing on GTK : [Attachment 215163] proposed patch

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has denied Krzysztof Wolanski
<k.wolanski at samsung.com>'s request for review:
Bug 121593: REGRESSION(r155543): accessibility/radio-button-title-label.html is
failing on GTK
https://bugs.webkit.org/show_bug.cgi?id=121593

Attachment 215163: proposed patch
https://bugs.webkit.org/attachment.cgi?id=215163&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
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!