[Webkit-unassigned] [Bug 136150] [ATK] Changing the mapping of aria rowheader and columnheader into respective ATK roles.

[bugzilla-daemon at webkit.org]

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


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

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #237453|review?                     |review-
               Flag|                            |




--- Comment #11 from Mario Sanchez Prada <mario at webkit.org>  2014-09-03 04:24:53 PST ---
(From update of attachment 237453)
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.

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