| Summary: | AX: cellForColumnAndRow fails for tables with hidden table cells | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||||||||||||||
| Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | aboxhall, apinheiro, buildbot, dglazkov, dmazzoni, eric, esprehn+autocc, jdiggs, ojan.autocc, rniwa, thorton, webkit-ews, webkit.review.bot | ||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
chris fleizach
2013-02-16 23:50:06 PST
Created attachment 189561 [details]
patch
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 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 Comment on attachment 189561 [details] patch Attachment 189561 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16692212 Comment on attachment 189561 [details] patch Attachment 189561 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16699181 Created attachment 189564 [details]
patch
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.
Comment on attachment 189564 [details] patch Attachment 189564 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16701117 Created attachment 189567 [details]
patch
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) 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 Created attachment 189585 [details]
patch
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.
Created attachment 189586 [details]
patch
Comment on attachment 189586 [details] patch Attachment 189586 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16697275 Comment on attachment 189586 [details] patch Attachment 189586 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16695302 Created attachment 189589 [details]
patch
Comment on attachment 189589 [details] patch Attachment 189589 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16691300 Comment on attachment 189589 [details] patch Attachment 189589 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16701196 Created attachment 189592 [details]
patch
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 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 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. Created attachment 189774 [details]
patch
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? (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! (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. |