Bug 136150 - [ATK] Changing the mapping of aria rowheader and columnheader into respective ATK roles.
Summary: [ATK] Changing the mapping of aria rowheader and columnheader into respective...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-08-22 01:30 PDT by Andrzej Badowski
Modified: 2014-10-10 01:33 PDT (History)
12 users (show)

See Also:


Attachments
proposed patch (1.50 KB, patch)
2014-08-22 01:30 PDT, Andrzej Badowski
no flags Details | Formatted Diff | Diff
proposed patch 2 (2.06 KB, patch)
2014-08-22 02:05 PDT, Andrzej Badowski
no flags Details | Formatted Diff | Diff
proposed patch 3 (1.72 KB, patch)
2014-08-22 02:18 PDT, Andrzej Badowski
cfleizach: review-
Details | Formatted Diff | Diff
proposed patch 4 (1.72 KB, patch)
2014-08-25 02:27 PDT, Andrzej Badowski
no flags Details | Formatted Diff | Diff
proposed patch 5 (6.11 KB, patch)
2014-09-01 09:50 PDT, Andrzej Badowski
mario: review-
Details | Formatted Diff | Diff
proposed patch 6 (8.24 KB, patch)
2014-09-05 09:18 PDT, Andrzej Badowski
no flags Details | Formatted Diff | Diff
ptoposed patch 7 (49.65 KB, patch)
2014-10-02 08:24 PDT, Andrzej Badowski
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (476.81 KB, application/zip)
2014-10-02 09:19 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (499.62 KB, application/zip)
2014-10-02 09:52 PDT, Build Bot
no flags Details
proposed patch 8 (50.28 KB, patch)
2014-10-02 10:01 PDT, Andrzej Badowski
no flags Details | Formatted Diff | Diff
proposed patch 9 (50.24 KB, patch)
2014-10-06 08:29 PDT, Andrzej Badowski
cfleizach: review+
Details | Formatted Diff | Diff
proposed patch 10 (50.28 KB, patch)
2014-10-09 03:35 PDT, Andrzej Badowski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrzej Badowski 2014-08-22 01:30:49 PDT
Created attachment 236974 [details]
proposed patch

It is my suggestion as http://www.w3.org/TR/wai-aria-implementation/ 5.4.1.
Comment 1 Radar WebKit Bug Importer 2014-08-22 01:31:17 PDT
<rdar://problem/18099822>
Comment 2 WebKit Commit Bot 2014-08-22 01:33:08 PDT
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.
Comment 3 Andrzej Badowski 2014-08-22 02:05:29 PDT
Created attachment 236975 [details]
proposed patch 2
Comment 4 Andrzej Badowski 2014-08-22 02:18:37 PDT
Created attachment 236977 [details]
proposed patch 3
Comment 5 chris fleizach 2014-08-22 09:02:08 PDT
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
Comment 6 Andrzej Badowski 2014-08-25 02:27:13 PDT
Created attachment 237066 [details]
proposed patch 4

according to the commentary of Chris Fleizach
Comment 7 Andrzej Badowski 2014-08-25 05:53:22 PDT
Mario, any more comments?
Comment 8 Mario Sanchez Prada 2014-08-25 06:01:12 PDT
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)
Comment 9 Andrzej Badowski 2014-09-01 09:50:51 PDT
Created attachment 237453 [details]
proposed patch 5
Comment 10 Andrzej Badowski 2014-09-02 05:11:54 PDT
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.
Comment 11 Mario Sanchez Prada 2014-09-03 04:24:53 PDT
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.
Comment 12 Andrzej Badowski 2014-09-05 09:18:52 PDT
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.
Comment 13 Mario Sanchez Prada 2014-09-08 02:09:02 PDT
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.
Comment 14 Andrzej Badowski 2014-09-08 03:59:50 PDT
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.
Comment 15 Andrzej Badowski 2014-10-02 08:24:49 PDT
Created attachment 239110 [details]
ptoposed patch 7
Comment 16 Build Bot 2014-10-02 09:19:29 PDT
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
Comment 17 Build Bot 2014-10-02 09:19:33 PDT
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
Comment 18 Build Bot 2014-10-02 09:52:27 PDT
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
Comment 19 Build Bot 2014-10-02 09:52:33 PDT
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
Comment 20 Andrzej Badowski 2014-10-02 10:01:12 PDT
Created attachment 239114 [details]
proposed patch 8
Comment 21 chris fleizach 2014-10-04 00:15:25 PDT
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?
Comment 22 Andrzej Badowski 2014-10-06 08:24:29 PDT
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.
Comment 23 Andrzej Badowski 2014-10-06 08:29:26 PDT
Created attachment 239337 [details]
proposed patch 9

According to the comments by Chris.
Comment 24 Andrzej Badowski 2014-10-08 07:56:25 PDT
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.
Comment 25 chris fleizach 2014-10-08 09:16:27 PDT
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())
Comment 26 Darin Adler 2014-10-08 11:47:07 PDT
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.
Comment 27 Chris Dumez 2014-10-08 11:58:17 PDT
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.
Comment 28 Andrzej Badowski 2014-10-09 03:35:57 PDT
Created attachment 239525 [details]
proposed patch 10

according to a recent commentary by Chris
Comment 29 WebKit Commit Bot 2014-10-10 01:33:00 PDT
Comment on attachment 239525 [details]
proposed patch 10

Clearing flags on attachment: 239525

Committed r174567: <http://trac.webkit.org/changeset/174567>
Comment 30 WebKit Commit Bot 2014-10-10 01:33:08 PDT
All reviewed patches have been landed.  Closing bug.