[webkit-reviews] review denied: [Bug 136150] [ATK] Changing the mapping of aria rowheader and columnheader into respective ATK roles. : [Attachment 237453] proposed patch 5

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has denied Andrzej Badowski
<a.badowski at samsung.com>'s request for review:
Bug 136150: [ATK] Changing the mapping of aria rowheader and columnheader into
respective ATK roles.
https://bugs.webkit.org/show_bug.cgi?id=136150

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

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


> LayoutTests/ChangeLog:8
> +	   Minor changes in the test to check the mapping of rowheader and
columnheader.

Still, I miss tests using the <th> element. Perhaps it's better to leave those
for another patch, but I would be insteresting to try to add them now as well,
if it's not much of a trouble

> LayoutTests/accessibility/roles-exposed.html:210
> +	       <!-- [ATK] Wrong role (webkit.org/b/125493) --><div
data-platform="atk,mac" role="rowheader"    class="ex">X</div>
> +	       <!-- [ATK] Wrong role (webkit.org/b/125493) --><div
data-platform="atk,mac" role="columnheader" class="ex">X</div>

You should remove the prefixed comment and probably comment on bug 125493
together with this patch

> LayoutTests/accessibility/roles-exposed.html:274
> +	       <!-- [ATK] Wrong role (webkit.org/b/125493) --><div
role="rowheader"    data-platform="atk,mac" class="ex">X</div>
> +	       <!-- [ATK] Wrong role (webkit.org/b/125493) --><div
role="columnheader" data-platform="atk,mac" class="ex">X</div>

Same here

> Source/WebCore/ChangeLog:7
> +

You need to write a description of the patch here (between this 'Reviewed by'
line and the following one mentioning the tests).

> Source/WebCore/ChangeLog:12
> +	   (atkRoleCell):
> +	   (atkRole):

A brief sentence describing each change here wouldn't hurt either :)

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:597
> +    const AtomicString& roleCell =
coreObject->getAttribute(HTMLNames::roleAttr);
> +    if (equalIgnoringCase(roleCell.string(), "rowheader"))
> +	   return ATK_ROLE_ROW_HEADER;

This seems wrong. The value of coreObject->roleValue() or
coreObject->ariaRoleAttribute() should be RowHeaderRole and that's all you
should need to make this decision here. Mannually checking the value of the
role attribute like this is not a good idea (and in any case it should be done
in another way, such as in AXObject::nodeHasRole())

Maybe there's something wrong in determineAccessibilityRole() or
determineAriaRoleAttribute()?

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:696
> -	   return coreObject->inheritsPresentationalRole() ? ATK_ROLE_SECTION :
ATK_ROLE_TABLE_CELL;
> +	   return atkRoleCell(coreObject);

If you fix the root reason why you could not get RowHeaderRole or
ColumnHeaderRole to work (see comment above), you will probably not need this
new function here either.