[AX] Improve AccessibilityTableCell columnHeaders and rowHeaders functions.

• Component: Accessibility
• Status: RESOLVED FIXED

[Andrzej Badowski]

Take into account that <th> elements can be both the column headers and row headers improved the operation of two functions: columnHeaders and rowHeaders.

[Radar WebKit Bug Importer]

<rdar://problem/18335899>

[Andrzej Badowski]

Created attachment 238116 [details]
proposed patch

[Andrzej Badowski]

Created attachment 238124 [details]
proposed patch 2

[Build Bot]

Comment on attachment 238124 [details]
proposed patch 2

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

New failing tests:
accessibility/table-headers.html
accessibility/table-cells.html
accessibility/table-attributes.html
accessibility/table-sections.html

[Build Bot]

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

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

[chris fleizach]

Comment on attachment 238124 [details]
proposed patch 2

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

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:140
> +    if (scope == "row" || scope == "rowgroup")

It seems like this will invalidate the existing logic
if ("rowgroup" && isTableCellInSameRowGroup(tableCell)

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:145
> +    while (parentNode) {

this should be written as a for loop. 
also, you're getting the parentNode at a bad time, because when the parentNode() returns 0, you'll then crash on the next line
   if (parentNode->hasTagName(...

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:146
> +        parentNode = parentNode->parentNode();

what is the provenance of this logic? Is there something in the spec that says a cell in a tfooter or body is a row header?

[Build Bot]

Comment on attachment 238124 [details]
proposed patch 2

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

New failing tests:
accessibility/table-headers.html
accessibility/table-cells.html
accessibility/table-attributes.html
accessibility/table-sections.html

[Build Bot]

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

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

[Andrzej Badowski]

Comment on attachment 238124 [details]
proposed patch 2

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

>> Source/WebCore/accessibility/AccessibilityTableCell.cpp:140
>> +    if (scope == "row" || scope == "rowgroup")
> 
> It seems like this will invalidate the existing logic
> if ("rowgroup" && isTableCellInSameRowGroup(tableCell)

I don't think so, because the existing logic: if ("rowgroup" && isTableCellInSameRowGroup(tableCell)) is checked at the beginning of the function rowHeaders and only then is invoked function isRowHeaderCell. This last function is a more general purpose.

>> Source/WebCore/accessibility/AccessibilityTableCell.cpp:145
>> +    while (parentNode) {
> 
> this should be written as a for loop. 
> also, you're getting the parentNode at a bad time, because when the parentNode() returns 0, you'll then crash on the next line
>    if (parentNode->hasTagName(...

You're right with "at a bad time".

>> Source/WebCore/accessibility/AccessibilityTableCell.cpp:146
>> +        parentNode = parentNode->parentNode();
> 
> what is the provenance of this logic? Is there something in the spec that says a cell in a tfooter or body is a row header?

Please note that this decision takes place at a time when it is already known that this cell had no another markings on rowheader.
I think it is worth at this point exclude the scope of two types: "col" and "colgroup".
It remains a situation in which the cell must be resolved th type with no support in the specification.
It seems logical that such a cell in the thead (header across the table) is column header. 
Also for me it was logical that since in the table was separated tbody area, this is an area on rows of data. th element will probably be at the beginning of a row as the first cell or the first cells.
I imagine the situation that in the summary of table, for which a good place seems to be tfoot, the first th cell will be logically rowheader. 
I think that to have columnheader in tfoot / tbody, it must be specially marked. 
Likewise, I think that if the author put in the code html th tags inside rows within tbody or tfoot and he did not identify them extra, it has the right to expect to qualify them in any way.

As a simple illustration for the above sentences I quote the code and the appearance of a table taken from the attached test, supplemented by tfoot:
<table id="table1">
  <thead>
  <tr>
    <th>No</th>
    <th>Country</th>
    <th>Capital</th>
  </tr>
  </thead>
  <tbody>
  <tr>
    <th>1.</th>
    <td>Poland</td>
    <td>Warsaw</td>
  </tr>
  <tr>
    <th>2.</th>
    <td>Russia</td>
    <td>Moscow</td>
  </tr>
   <tr>
    <th>3.</th>
    <td>Ukraine</td>
    <td>Kiev</td>
  </tr>
  </tbody>
  <tfoot>
   <th>All</th><td>3 countries</td><td>3 capitals</td>
  </tfoot>
</table>

No	Country	Capital
1.	Poland	Warsaw
2.	Russia	Moscow
3.	Ukraine	Kiev
All	3 countries	3 capitals

[chris fleizach]

Comment on attachment 238124 [details]
proposed patch 2

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

still need to figure out the layout test failures. otherwise the concept seems ok

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:127
> +    while (parentNode) {

we should add some comments here to explain why we want to do this

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:147
> +        if (parentNode->hasTagName(tbodyTag) || parentNode->hasTagName(tfootTag))

we should add some comments here to explain why we want to do this

[Andrzej Badowski]

Created attachment 239405 [details]
proposed patch 3

[chris fleizach]

Comment on attachment 239405 [details]
proposed patch 3

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

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:133
> +    for ( ; parentNode; parentNode = parentNode->parentNode()) {

I think you can put
Node* parentNode = node(); 
inside the for loop here
I don't see you using it again outside the for loop

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:156
> +    for ( ; parentNode; parentNode = parentNode->parentNode()) {

ditto about parentNode

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:157
> +        if (parentNode->hasTagName(tbodyTag) || parentNode->hasTagName(tfootTag))

Why are row headers == to being inside tfoot? while column headers are == to being inside thead? 
it doesn't seem like those things have to be equal. 

I can see thead elements would probably be column headers (unless it's the first cell in the thead, in which case it might be a row header)
But for tfoot contained elements, I don't see why those would be row headers

[Andrzej Badowski]

Comment on attachment 239405 [details]
proposed patch 3

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

>> Source/WebCore/accessibility/AccessibilityTableCell.cpp:133
>> +    for ( ; parentNode; parentNode = parentNode->parentNode()) {
> 
> I think you can put
> Node* parentNode = node(); 
> inside the for loop here
> I don't see you using it again outside the for loop

You're right. The code will be more compact.

>> Source/WebCore/accessibility/AccessibilityTableCell.cpp:156
>> +    for ( ; parentNode; parentNode = parentNode->parentNode()) {
> 
> ditto about parentNode

OK

>> Source/WebCore/accessibility/AccessibilityTableCell.cpp:157
>> +        if (parentNode->hasTagName(tbodyTag) || parentNode->hasTagName(tfootTag))
> 
> Why are row headers == to being inside tfoot? while column headers are == to being inside thead? 
> it doesn't seem like those things have to be equal. 
> 
> I can see thead elements would probably be column headers (unless it's the first cell in the thead, in which case it might be a row header)
> But for tfoot contained elements, I don't see why those would be row headers

While I well understand your intention. I think that it is worth at this point further clarify the meaning of the th element within the tfoot. The situation is such that the author of the html code did not use a chance to resolve the meaning of the th element with the scope. It seems to me that deciding instead of someone (beyond the first cell in a row), it would be overinterpretation.

[Andrzej Badowski]

Created attachment 239470 [details]
proposed patch 4

[chris fleizach]

Comment on attachment 239470 [details]
proposed patch 4

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

almost there i think

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:160
> +        if (parentNode->hasTagName(tfootTag)) {

this looks good for tfoot, but doesn't the same thing apply to body?

For example, this is from w3c

<tbody>
  <tr>
   <td>Ms Danus
   <td>Doughnuts
   <td><img src="http://example.com/mydoughnuts.png" title="Doughnuts from Ms Danus">
   <td>$45
  <tr>
   <td><input id=e1 type=text name=who required form=f>
   <td><input id=e2 type=text name=what required form=f>
   <td><input id=e3 type=url name=pic form=f>
   <td><input id=e4 type=number step=0.01 min=0 value=0 required form=f>
</table>
-------------
I wouldn't expect each of these cells to be a row header

[Andrzej Badowski]

Comment on attachment 239470 [details]
proposed patch 4

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

>> Source/WebCore/accessibility/AccessibilityTableCell.cpp:160
>> +        if (parentNode->hasTagName(tfootTag)) {
> 
> this looks good for tfoot, but doesn't the same thing apply to body?
> 
> For example, this is from w3c
> 
> <tbody>
>   <tr>
>    <td>Ms Danus
>    <td>Doughnuts
>    <td><img src="http://example.com/mydoughnuts.png" title="Doughnuts from Ms Danus">
>    <td>$45
>   <tr>
>    <td><input id=e1 type=text name=who required form=f>
>    <td><input id=e2 type=text name=what required form=f>
>    <td><input id=e3 type=url name=pic form=f>
>    <td><input id=e4 type=number step=0.01 min=0 value=0 required form=f>
> </table>
> -------------
> I wouldn't expect each of these cells to be a row header

I agree with you. Although in this particular case I am not afraid that any td element will be qualified as rowheader or columnheader because they do not have a scope attribute. All of the code of classification based on the location inside the table only applies to th element. This is ensured by a prior call to isTableHeaderCell. Of course, I will repeat the same thing for tbody.

[Andrzej Badowski]

Created attachment 239535 [details]
proposed patch 5

[Build Bot]

Comment on attachment 239535 [details]
proposed patch 5

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

New failing tests:
accessibility/table-cells.html
accessibility/table-attributes.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 239535 [details]
proposed patch 5

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

New failing tests:
accessibility/table-cells.html
accessibility/table-attributes.html

[Build Bot]

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

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

[Andrzej Badowski]

Created attachment 239620 [details]
proposed patch 6

[chris fleizach]

Comment on attachment 239620 [details]
proposed patch 6

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

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:138
> +    for (Node* parentNode = node(); parentNode; parentNode = parentNode->parentNode()) {

this patch looks good. I think after Darin's comments in another bug we should change this for loop to

 for (const auto& ancestor : ancestorsOfType<Node>(node()))

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:160
> +    for (Node* parentNode = node(); parentNode; parentNode = parentNode->parentNode()) {

ditto
 for (const auto& ancestor : ancestorsOfType<Node>(node()))

> Source/WebCore/accessibility/AccessibilityTableCell.h:49
> +    bool isRowHeaderCell();

these two methods should be const

bool isRowHeaderCell() const;

[Andrzej Badowski]

Comment on attachment 239620 [details]
proposed patch 6

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

>> Source/WebCore/accessibility/AccessibilityTableCell.cpp:138
>> +    for (Node* parentNode = node(); parentNode; parentNode = parentNode->parentNode()) {
> 
> this patch looks good. I think after Darin's comments in another bug we should change this for loop to
> 
>  for (const auto& ancestor : ancestorsOfType<Node>(node()))

Direct use of this function leads to errors in the build. Some examples of the use of a similar iterator exist in webkit. My attempts to minor changes have not led to success.

>> Source/WebCore/accessibility/AccessibilityTableCell.cpp:160
>> +    for (Node* parentNode = node(); parentNode; parentNode = parentNode->parentNode()) {
> 
> ditto
>  for (const auto& ancestor : ancestorsOfType<Node>(node()))

Direct use of this function leads to errors in the build. Some examples of the use of a similar iterator exist in webkit. My attempts to minor changes in the use did not lead to success.

>> Source/WebCore/accessibility/AccessibilityTableCell.h:49
>> +    bool isRowHeaderCell();
> 
> these two methods should be const
> 
> bool isRowHeaderCell() const;

OK

[Andrzej Badowski]

Created attachment 239728 [details]
proposed patch 7

[WebKit Commit Bot]

Comment on attachment 239728 [details]
proposed patch 7

Clearing flags on attachment: 239728

Committed r174675: <http://trac.webkit.org/changeset/174675>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.