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

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Andrzej Badowski]

Created attachment 236974 [details]
proposed patch

It is my suggestion as http://www.w3.org/TR/wai-aria-implementation/ 5.4.1.

[Radar WebKit Bug Importer]

<rdar://problem/18099822>

[WebKit Commit Bot]

Attachment 236974 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:682:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:684:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 4 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrzej Badowski]

Created attachment 236975 [details]
proposed patch 2

[Andrzej Badowski]

Created attachment 236977 [details]
proposed patch 3

[chris fleizach]

Comment on attachment 236977 [details]
proposed patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=236977&action=review

> Source/WebCore/ChangeLog:10
> +        No new tests because existing tests, especially lauout accessibility tests associated with the roles aria cover it.

lauout spelled incorrect
aria => ARIA

[Andrzej Badowski]

Created attachment 237066 [details]
proposed patch 4

according to the commentary of Chris Fleizach

[Andrzej Badowski]

Mario, any more comments?

[Mario Sanchez Prada]

Comment on attachment 237066 [details]
proposed patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=237066&action=review

> Source/WebCore/ChangeLog:10
> +        No new tests because existing tests, especially accessibility layout tests associated with the ARIA roles, cover it.

Could you mention existing tests are those ones covering this change? Asking that because it's not clear to me that this change is being tested at all (even though if the mapping seems correct)

[Andrzej Badowski]

Created attachment 237453 [details]
proposed patch 5

[Andrzej Badowski]

Mario, I introduced an amendment to the test to be able to verify the change. Moreover I completed the code. Please comment this proposal of mapping.

[Mario Sanchez Prada]

Comment on attachment 237453 [details]
proposed patch 5

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.

[Andrzej Badowski]

Created attachment 237694 [details]
proposed patch 6

This is a proposal for discussion. 
I am currently working on several tests that fail following these changes in the code.

[Mario Sanchez Prada]

Comment on attachment 237694 [details]
proposed patch 6

View in context: https://bugs.webkit.org/attachment.cgi?id=237694&action=review

> 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>

Please remove the prefixed comments (and post a comment in bug 125493 too, if possible)

> 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>

Ditto

> Source/WebCore/ChangeLog:10
> +        No new tests because the existing test with minor amendments and other tests cover it.
> +
> +        Proposals for changes in the code that allow using listed ARIA roles.

I know it's a proposal for discussion, but just in case, we normally put first the line describing the patch and last the one mentioning the tests (new or not) covering the changes. Would be nice to consider that for future patches

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2564
> +        for ( ; node; node = node->parentNode()) {

I think a 'while (node)' statement would be better in this case

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:684
> +    case RowHeaderRole:
> +        return ATK_ROLE_ROW_HEADER;
> +    case ColumnHeaderRole:
> +        return ATK_ROLE_COLUMN_HEADER;

I wonder if this change will break other a11y tests or functionality if that some AccessibilityObjects that used to report a CellRole role will now report a ColumnHeaderRole or RowHeaderRole. I have the feeling that some more changes would be needed such as, for instance, to update the roleIsTextType() static function in the ATK wrapper.

Also, it would be great if you could share here a plain text representation of the ATK hierarchy that would be exposed for some common cases of HTML tables with column/row headers, so we are sure that whatever we will be exposing is what ATs (e.g. Orca) do expect.

[Andrzej Badowski]

Comment on attachment 237694 [details]
proposed patch 6

View in context: https://bugs.webkit.org/attachment.cgi?id=237694&action=review

Thank you for your review.

>> LayoutTests/accessibility/roles-exposed.html:210
>> +            <!-- [ATK] Wrong role (webkit.org/b/125493) --><div data-platform="atk,mac" role="columnheader" class="ex">X</div>
> 
> Please remove the prefixed comments (and post a comment in bug 125493 too, if possible)

OK

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

OK

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2564
>> +        for ( ; node; node = node->parentNode()) {
> 
> I think a 'while (node)' statement would be better in this case

I agree.

[Andrzej Badowski]

Created attachment 239110 [details]
ptoposed patch 7

[Build Bot]

Comment on attachment 239110 [details]
ptoposed patch 7

Attachment 239110 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4576392100446208

New failing tests:
accessibility/aria-tables.html

[Build Bot]

Created attachment 239112 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 239110 [details]
ptoposed patch 7

Attachment 239110 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4691469575127040

New failing tests:
accessibility/aria-tables.html

[Build Bot]

Created attachment 239113 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Andrzej Badowski]

Created attachment 239114 [details]
proposed patch 8

[chris fleizach]

Comment on attachment 239114 [details]
proposed patch 8

View in context: https://bugs.webkit.org/attachment.cgi?id=239114&action=review

> LayoutTests/accessibility/table-roles-hierarchy.html:13
> +    {

brace goes on previous line

> LayoutTests/accessibility/table-roles-hierarchy.html:19
> +    {

brace goes on previous line

> LayoutTests/accessibility/table-roles-hierarchy.html:25
> +    {

brace goes on previous line

> LayoutTests/accessibility/table-roles-hierarchy.html:36
> +          if ( childAXObject.role != "AXRole: AXStaticText")

extra space after (

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2573
> +        while (node) {

this can be written as a for loop
for (node = node->parentNode(); node; node = node->parentNode())

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2578
> +            if (node->hasTagName(tableTag))

are you sure you want to make a <Table> into a CellRole?

[Andrzej Badowski]

Comment on attachment 239114 [details]
proposed patch 8

View in context: https://bugs.webkit.org/attachment.cgi?id=239114&action=review

>> LayoutTests/accessibility/table-roles-hierarchy.html:13
>> +    {
> 
> brace goes on previous line

OK

>> LayoutTests/accessibility/table-roles-hierarchy.html:19
>> +    {
> 
> brace goes on previous line

OK

>> LayoutTests/accessibility/table-roles-hierarchy.html:25
>> +    {
> 
> brace goes on previous line

OK

>> LayoutTests/accessibility/table-roles-hierarchy.html:36
>> +          if ( childAXObject.role != "AXRole: AXStaticText")
> 
> extra space after (

OK

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2573
>> +        while (node) {
> 
> this can be written as a for loop
> for (node = node->parentNode(); node; node = node->parentNode())

The for loop is very good...

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2578
>> +            if (node->hasTagName(tableTag))
> 
> are you sure you want to make a <Table> into a CellRole?

Please note that tableTag at this point means that th element has no parent or the parent's parent and so on, which would be theadTag, tbodyTag or tfootTag item. In this situation, the decision for the original thTag is CellRole. Use tableTag lets stop checking on tableTag and not for example on body tag or html tag.

[Andrzej Badowski]

Created attachment 239337 [details]
proposed patch 9

According to the comments by Chris.

[Andrzej Badowski]

If the current patch for bug 136818 (proposed patch 4) would be positively reviewed, this bug will depend on bug 136818. Accurate determination of role in the function AccessibilityTableCell::determineAccessibilityRole() will be based on the new functions: isRowHeaderCell and isColumnHeaderCell.

[chris fleizach]

Comment on attachment 239337 [details]
proposed patch 9

View in context: https://bugs.webkit.org/attachment.cgi?id=239337&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2568
> +        for (node = node->parentNode(); node; node = node->parentNode()) {

I think you should use a different variable for node, since you're using it it everywhere else to reference the current node, but
then you're in this for loop, changing it to parent nodes.
I see that you don't leave this block, so there's probably not a logic problem, but for readability it might be good to use

for (Node* parentNode = node->parentNode(); parentNode; parentNode = parentNode->parentNode())

[Darin Adler]

Comment on attachment 239337 [details]
proposed patch 9

View in context: https://bugs.webkit.org/attachment.cgi?id=239337&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2567
> +    if (node && (node->hasTagName(thTag))) {

Instead of hasTagName(thTag) we should consider:

    if (is<HTMLTableHeaderElement>(node))

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2568
>> +        for (node = node->parentNode(); node; node = node->parentNode()) {
> 
> I think you should use a different variable for node, since you're using it it everywhere else to reference the current node, but
> then you're in this for loop, changing it to parent nodes.
> I see that you don't leave this block, so there's probably not a logic problem, but for readability it might be good to use
> 
> for (Node* parentNode = node->parentNode(); parentNode; parentNode = parentNode->parentNode())

This is much better:

    for (auto& ancestor : ancestorsOfType<HTMLElement>(downcast<HTMLTableHeaderElement>(*node))

All the hasTagName checks below will be more efficient if we already know it’s an HTML element.

[Chris Dumez]

Comment on attachment 239337 [details]
proposed patch 9

View in context: https://bugs.webkit.org/attachment.cgi?id=239337&action=review

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2567
>> +    if (node && (node->hasTagName(thTag))) {
> 
> Instead of hasTagName(thTag) we should consider:
> 
>     if (is<HTMLTableHeaderElement>(node))

I don't think we have such class (yet). Sadly, both tdTag and thTag map to the same HTMLTableCellElement class. So currently, if you're only interested in thTag but not tdTag, using hasTagName() is correct. This is something we should likely improve.

[Andrzej Badowski]

Created attachment 239525 [details]
proposed patch 10

according to a recent commentary by Chris

[WebKit Commit Bot]

Comment on attachment 239525 [details]
proposed patch 10

Clearing flags on attachment: 239525

Committed r174567: <http://trac.webkit.org/changeset/174567>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.