[webkit-reviews] review denied: [Bug 117729] [ATK] Missing aria roles mappings. : [Attachment 216289] Patch set #9 - to be discussed

Mon Nov 11 09:57:58 PST 2013

Mario Sanchez Prada <mario at webkit.org> has denied Lukasz Gajowy
<l.gajowy at samsung.com>'s request for review:
Bug 117729: [ATK] Missing aria roles mappings.
https://bugs.webkit.org/show_bug.cgi?id=117729

Attachment 216289: Patch set #9 - to be discussed
https://bugs.webkit.org/attachment.cgi?id=216289&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=216289&action=review

This looks better now. Thanks!

Still, setting r- for now because of a couple of issues in the patch. See my
comments below...

(In reply to comment #35)
> Created an attachment (id=216289) [details]
> Patch set #9 - to be discussed
> 
> I fixed it as you proposed... but:
> I don't know how to test the WebAreaRole mapping. This is not a a role that
is invoked explicitly by writing the test: <div role="foobar">foobar role</div>

> How can i test it different way? Should I test it?

Just get the accessible element for the <body> element and query the role for
it. That should be ATK_ROLE_DOCUMENT_WEB, therefore mapped to AXWebArea

> The second thing is that document role and article role both are mapped to
ATK_ROLE_DOCUMENT_FRAME and because of that they both return "AXDocument" in
test. Is this behavior correct?

According to the WAI ARIA mapping table, that is correct. The only difference
between those is that the article should also expose the object attribute
"xml-roles:article" (according to the very last revision from November the
5th), but I would not add that bit for now unless Joanie gives us green light
to do it so. And in any case that's something we can easily add later on, so
for now I'd say "forget about that xml-roles attribute and just expose
ATK_ROLE_DOCUMENT_FRAME" for article too.

> "alert" and "alertdialog" roles seem to behave similarly.

I'd say that for "alert" you should expose it as ATK_ROLE_ALERT and for
"alertdialog" as ATK_ROLE_DIALOG and ignore, at least for now, the fact that
you can't distinguish between a "dialog" and an "alertdialog" here.

> LayoutTests/accessibility/aria-mappings-expected.txt:21
> +role="alertdialog" is a dialog which contains an alert message.
> +Role for 'alertdialog' div is: AXRole: AXAlert

AlertDialogRole should be mapped to a dialog role, according to
http://www.w3.org/TR/wai-aria-implementation/#mapping_role_table

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:523
> +    case ApplicationAlertRole:
> +    case ApplicationAlertDialogRole:
> +	   return ATK_ROLE_ALERT;

AlertDialogRole should be mapped to a ATK_ROLE_DIALOG, according to
http://www.w3.org/TR/wai-aria-implementation/#mapping_role_table

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:194
> -	   return "AXWebArea";
> +	   return "AXDocument";

You forgot to map ATK_ROLE_DOCUMENT_WEB here