Bug 110050 – AX: cellForColumnAndRow fails for tables with hidden table cells

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

[chris fleizach]

If a table has cells hidden in a row, then the cellForColumnAndRow method fails because of discrepancies between how table children are created and how cellForRowAndColumn is implemented.

This causes VoiceOver to be unable to navigate certain tables.

Here's an example

<table id="table" border=1>
  <thead><th hidden>header 1</th><th hidden>header 2</th></thead>
  <tr><td>foo</td><td>bar</td></tr>
</table>

[chris fleizach]

Created attachment 189561 [details]
patch

[chris fleizach]

rdar://13207279

[chris fleizach]

Comment on attachment 189561 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:-135
> -AccessibilityTableCell* AccessibilityARIAGrid::cellForColumnAndRow(unsigned column, unsigned row)

this code was moved to AccessibilityTable::cellForColAndRow and cleaned up a tad

> Source/WebCore/accessibility/AccessibilityTable.cpp:349
> +    RenderTableSection* tableSection = table->topSection();

use topSection instead of header as suggested in the FIXME

[WebKit Review Bot]

Comment on attachment 189561 [details]
patch

Attachment 189561 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16700151

[WebKit Review Bot]

Comment on attachment 189561 [details]
patch

Attachment 189561 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16692212

[EFL EWS Bot]

Comment on attachment 189561 [details]
patch

Attachment 189561 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16699181

[chris fleizach]

Created attachment 189564 [details]
patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/accessibility/table-with-hidden-head-section-expected.txt', u'LayoutTests/accessibility/table-with-hidden-head-section.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/AccessibilityARIAGrid.cpp', u'Source/WebCore/accessibility/AccessibilityARIAGrid.h', u'Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp', u'Source/WebCore/accessibility/AccessibilityARIAGridCell.h', u'Source/WebCore/accessibility/AccessibilityTable.cpp', u'Source/WebCore/accessibility/AccessibilityTable.h', u'Source/WebCore/accessibility/AccessibilityTableCell.cpp', u'Source/WebCore/accessibility/AccessibilityTableCell.h', u'Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceTable.cpp', u'Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm', u'Source/WebCore/rendering/RenderTableSection.h', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/WebAccessibilityObject.cpp']" exit_code: 1
Source/WebCore/ChangeLog:45:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 15 files


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

[EFL EWS Bot]

Comment on attachment 189564 [details]
patch

Attachment 189564 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16701117

[chris fleizach]

Created attachment 189567 [details]
patch

[Dominic Mazzoni]

Comment on attachment 189561 [details]
patch

Looks like a good refactoring, much cleaner

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

> Source/WebCore/accessibility/AccessibilityTable.cpp:513
> +        for (unsigned colIndexCounter = children.size(); colIndexCounter > 0; --colIndexCounter) {

This loop could start at min(children.size(), column)

[WebKit Review Bot]

Comment on attachment 189567 [details]
patch

Attachment 189567 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16697229

New failing tests:
accessibility/table-with-hidden-head-section.html

[chris fleizach]

Created attachment 189585 [details]
patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/accessibility/table-with-hidden-head-section-expected.txt', u'LayoutTests/accessibility/table-with-hidden-head-section.html', u'LayoutTests/platform/chromium/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/AccessibilityARIAGrid.cpp', u'Source/WebCore/accessibility/AccessibilityARIAGrid.h', u'Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp', u'Source/WebCore/accessibility/AccessibilityARIAGridCell.h', u'Source/WebCore/accessibility/AccessibilityTable.cpp', u'Source/WebCore/accessibility/AccessibilityTable.h', u'Source/WebCore/accessibility/AccessibilityTableCell.cpp', u'Source/WebCore/accessibility/AccessibilityTableCell.h', u'Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceTable.cpp', u'Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm', u'Source/WebCore/rendering/RenderTableSection.h', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/WebAccessibilityObject.cpp']" exit_code: 1
LayoutTests/platform/chromium/TestExpectations:1460:  expecting "[", "#", or end of line instead of "[Skip]"  [test/expectations] [5]
Total errors found: 1 in 16 files


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

[chris fleizach]

Created attachment 189586 [details]
patch

[Early Warning System Bot]

Comment on attachment 189586 [details]
patch

Attachment 189586 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16697275

[Early Warning System Bot]

Comment on attachment 189586 [details]
patch

Attachment 189586 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16695302

[chris fleizach]

Created attachment 189589 [details]
patch

[Early Warning System Bot]

Comment on attachment 189589 [details]
patch

Attachment 189589 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16691300

[Early Warning System Bot]

Comment on attachment 189589 [details]
patch

Attachment 189589 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16701196

[chris fleizach]

Created attachment 189592 [details]
patch

[Build Bot]

Comment on attachment 189592 [details]
patch

Attachment 189592 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16693519

New failing tests:
accessibility/table-attributes.html
accessibility/aria-tables.html
accessibility/table-sections.html
platform/mac/accessibility/aria-table-with-colspan-cells.html
accessibility/table-cell-for-column-and-row-crash.html
accessibility/table-cells.html
platform/mac/accessibility/table-multi-bodies.html
accessibility/table-cell-spans.html
accessibility/table-with-hidden-head-section.html

[Build Bot]

Comment on attachment 189592 [details]
patch

Attachment 189592 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16695745

New failing tests:
accessibility/table-attributes.html
accessibility/aria-tables.html
accessibility/table-sections.html
platform/mac/accessibility/aria-table-with-colspan-cells.html
accessibility/table-cell-for-column-and-row-crash.html
accessibility/table-cells.html
platform/mac/accessibility/table-multi-bodies.html
accessibility/table-cell-spans.html
accessibility/table-with-hidden-head-section.html

[Mario Sanchez Prada]

Comment on attachment 189592 [details]
patch

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

> Source/WebCore/ChangeLog:216
> +>>>>>>> .r143633

This line seems to have slipped by in the patch

> Source/WebCore/accessibility/AccessibilityTable.cpp:513
> +        for (unsigned colIndexCounter = std::min(static_cast<unsigned>(children.size()), column); colIndexCounter > 0; --colIndexCounter) {

I think this should actually be std::min(static_cast<unsigned>(children.size()), column + 1). Otherwise you won't ever enter the loop when you ask for column 0, even if children.size() > 0

This might be the reason why those tests are failing with the current patch, I guess.

[chris fleizach]

Created attachment 189774 [details]
patch

[Tim Horton]

Comment on attachment 189774 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityTable.cpp:348
>      // go through all the available sections to pull out the rows
>      // and add them as children

Any interest in capitalization/punctuation-izing this nearby comment that you didn't change?

> Source/WebCore/accessibility/AccessibilityTable.cpp:370
> +            // we need to check every cell for a new row, because cell spans
> +            // can cause us to mess rows if we just check the first column

s/mess/miss/? Capitals, punctuation, etc. (I realize you just moved this, but might as well fix it)

> Source/WebCore/accessibility/AccessibilityTable.cpp:374
> +            row->setRowIndex((int)m_rows.size());

You're static_casting elsewhere.

> Source/WebCore/accessibility/AccessibilityTable.cpp:-512
> -    if (!m_renderer || !m_renderer->isTable())

Where'd this go/why don't we need it anymore?

> Source/WebCore/accessibility/AccessibilityTable.cpp:509
> +    for (unsigned rowIndexCounter = row + 1; rowIndexCounter > 0; --rowIndexCounter) {
> +        unsigned rowIndex = rowIndexCounter - 1;

Why not just make rowIndexCounter rowIndex and make the values match up? I don't see it being used elsewhere. What's going on here?

> Source/WebCore/accessibility/AccessibilityTable.cpp:514
> +        for (unsigned colIndexCounter = std::min(static_cast<unsigned>(children.size()), column + 1); colIndexCounter > 0; --colIndexCounter) {
> +            unsigned colIndex = colIndexCounter - 1;

Ditto.

> Source/WebCore/rendering/RenderTableSection.h:156
> +    // Used by accessibility.

Do we usually add comments like this?

[chris fleizach]

(In reply to comment #26)
> (From update of attachment 189774 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189774&action=review
> 
> > Source/WebCore/accessibility/AccessibilityTable.cpp:348
> >      // go through all the available sections to pull out the rows
> >      // and add them as children
> 
> Any interest in capitalization/punctuation-izing this nearby comment that you didn't change?
> 

Will do

> > Source/WebCore/accessibility/AccessibilityTable.cpp:370
> > +            // we need to check every cell for a new row, because cell spans
> > +            // can cause us to mess rows if we just check the first column
> 
> s/mess/miss/? Capitals, punctuation, etc. (I realize you just moved this, but might as well fix it)

Will do

> 
> > Source/WebCore/accessibility/AccessibilityTable.cpp:374
> > +            row->setRowIndex((int)m_rows.size());
> 
> You're static_casting elsewhere.
> 

Will do


> > Source/WebCore/accessibility/AccessibilityTable.cpp:-512
> > -    if (!m_renderer || !m_renderer->isTable())
> 
> Where'd this go/why don't we need it anymore?
> 

We're no longer accessing the render table directly in this method so we don't need to verify that it still has a renderer or a table, we'll be relying solely on the internal structure we built up for accessibility.

> > Source/WebCore/accessibility/AccessibilityTable.cpp:509
> > +    for (unsigned rowIndexCounter = row + 1; rowIndexCounter > 0; --rowIndexCounter) {
> > +        unsigned rowIndex = rowIndexCounter - 1;
> 
> Why not just make rowIndexCounter rowIndex and make the values match up? I don't see it being used elsewhere. What's going on here?

We have to go backwards in this counter, but I want to use an unsigned int since i'll be comparing this number against other unsigned ints. 
Hence, I wouldn't be able to reach the 0th index without doing this trickery.

> 
> > Source/WebCore/accessibility/AccessibilityTable.cpp:514
> > +        for (unsigned colIndexCounter = std::min(static_cast<unsigned>(children.size()), column + 1); colIndexCounter > 0; --colIndexCounter) {
> > +            unsigned colIndex = colIndexCounter - 1;
> 
> Ditto.
> 
> > Source/WebCore/rendering/RenderTableSection.h:156
> > +    // Used by accessibility.
> 
> Do we usually add comments like this?

In some places, if you don't think it's necessary, I will remove

Thanks!

[Tim Horton]

(In reply to comment #27)
> > > Source/WebCore/accessibility/AccessibilityTable.cpp:509
> > > +    for (unsigned rowIndexCounter = row + 1; rowIndexCounter > 0; --rowIndexCounter) {
> > > +        unsigned rowIndex = rowIndexCounter - 1;
> > 
> > Why not just make rowIndexCounter rowIndex and make the values match up? I don't see it being used elsewhere. What's going on here?
> 
> We have to go backwards in this counter, but I want to use an unsigned int since i'll be comparing this number against other unsigned ints. 
> Hence, I wouldn't be able to reach the 0th index without doing this trickery.

A very good reason :)

> > 
> > > Source/WebCore/accessibility/AccessibilityTable.cpp:514
> > > +        for (unsigned colIndexCounter = std::min(static_cast<unsigned>(children.size()), column + 1); colIndexCounter > 0; --colIndexCounter) {
> > > +            unsigned colIndex = colIndexCounter - 1;
> > 
> > Ditto.
> > 
> > > Source/WebCore/rendering/RenderTableSection.h:156
> > > +    // Used by accessibility.
> > 
> > Do we usually add comments like this?
> 
> In some places, if you don't think it's necessary, I will remove

I don't think it's necessary. It's up to you.

[chris fleizach]

http://trac.webkit.org/changeset/144727