Currently AXRows and AXVisibleRows return the same thing. We have a FIXME line that should be fixed.
<rdar://problem/15483239>
Should also fixup the AccessibilityTable rowHeaders and colHeaders methods while we are in there. Specifically, they both have static_casts we can convert to toAccessibilityTable calls and they both have unnecessary continue statements.
Created attachment 217072 [details] Initial patch for feedback only. Patch added visibleRows method to AccessibilityTable. I selected to gather these rows each time the method was called (rather than cacheing like m_rows and m_columns) because they will be different after each scroll. Would add too much churn. Also, added ability to get arrays of elements with a specific attribute to DRT. Should help with test brevity moving forward. i.e. less need to dump all attrs as text to see a single array based val.
Created attachment 217079 [details] Rebased feedback patch. Rebased patch for feedback to get EWS results.
Attachment 217079 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/platform/mac/accessibility/table-visible-rows-expected.txt', u'LayoutTests/platform/mac/accessibility/table-visible-rows.html', u'Source/WebCore/accessibility/AccessibilityTable.cpp', u'Source/WebCore/accessibility/AccessibilityTable.h', u'Source/WebCore/accessibility/AccessibilityTableRow.h', u'Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm', u'Tools/DumpRenderTree/AccessibilityUIElement.cpp', u'Tools/DumpRenderTree/AccessibilityUIElement.h', u'Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm', u'Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp', u'Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h', u'Tools/WebKitTestRunner/InjectedBundle/Bindings/AccessibilityUIElement.idl', u'Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm']" exit_code: 1 Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:83: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 217081 [details] Fixed patch for feedback. Removed semicolon that snuck in.
Created attachment 217082 [details] Repeat WITH fix. Oops. REAL fixed patch.
Comment on attachment 217082 [details] Repeat WITH fix. Attachment 217082 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/24558002
Comment on attachment 217082 [details] Repeat WITH fix. View in context: https://bugs.webkit.org/attachment.cgi?id=217082&action=review > LayoutTests/platform/mac/accessibility/table-visible-rows.html:38 > + shouldBe("visibleRows", "3"); can you hide the content of this test when the test is done so that the ABCDE don't end up in the text expectations? > Source/WebCore/accessibility/AccessibilityTable.cpp:455 > +void AccessibilityTable::visibleRows(AccessibilityChildrenVector& rows) instead of replacing columnHeaders will visibleRows, can you just make a new visibleRows method -- that should cut down the on the churn in the patch and make it easier to read > Source/WebCore/accessibility/AccessibilityTable.cpp:462 > + unsigned rowCount = m_rows.size(); i think .size() returns a size_t so we should use that
Comment on attachment 217082 [details] Repeat WITH fix. Attachment 217082 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/24538018
Created attachment 217109 [details] Updated patch.
Comment on attachment 217109 [details] Updated patch. Attachment 217109 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/23748362
(In reply to comment #9) > (From update of attachment 217082 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217082&action=review > > > LayoutTests/platform/mac/accessibility/table-visible-rows.html:38 > > + shouldBe("visibleRows", "3"); > > can you hide the content of this test when the test is done so that the ABCDE don't end up in the text expectations? Hmm. I don't see this being done anyplace else and it seems that it would be somewhat confusing as this text dump is often relied on as part of the test. For example, if the test involved removing a node, we would expect to NOT see that node in the dump. Also, if something changes that is causing nodes to be removed or lost, removing them in our tests would hide that. Right? > > > Source/WebCore/accessibility/AccessibilityTable.cpp:455 > > +void AccessibilityTable::visibleRows(AccessibilityChildrenVector& rows) > > instead of replacing columnHeaders will visibleRows, can you just make a new visibleRows method -- that should cut down the on the churn in the patch and make it easier to read I flipped these around to match their declaration order and make the group easier to find and parse. The group being: void columnHeaders(AccessibilityChildrenVector&); void rowHeaders(AccessibilityChildrenVector&); void visibleRows(AccessibilityChildrenVector&); > > > Source/WebCore/accessibility/AccessibilityTable.cpp:462 > > + unsigned rowCount = m_rows.size(); > > i think .size() returns a size_t so we should use that Indeed. I prefer this as well. I was trying to maintain parity with the surrounding methods but I updated them all to size_t. Which, since I was already touching them to remove the unnecessary continue statements as stated above, seems ok. Thanks as usual for the helpful feedback!
Will fix the elf and win failures in next patch.
Comment on attachment 217109 [details] Updated patch. Attachment 217109 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/24488020
(In reply to comment #13) > (In reply to comment #9) > > (From update of attachment 217082 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=217082&action=review > > > > > LayoutTests/platform/mac/accessibility/table-visible-rows.html:38 > > > + shouldBe("visibleRows", "3"); > > > > can you hide the content of this test when the test is done so that the ABCDE don't end up in the text expectations? > > Hmm. I don't see this being done anyplace else and it seems that it would be somewhat confusing as this text dump is often relied on as part of the test. > > For example, if the test involved removing a node, we would expect to NOT see that node in the dump. Also, if something changes that is causing nodes to be removed or lost, removing them in our tests would hide that. Right? A bunch of AX tests do this. Ryosuke has been recommending doing this to me. If media tests had done this then we wouldn't have had to modify 500 files when we made that recent <video> tag change. I think it makes the test output easier to read, because we're not testing the dump text routines > > > > > > Source/WebCore/accessibility/AccessibilityTable.cpp:455 > > > +void AccessibilityTable::visibleRows(AccessibilityChildrenVector& rows) > > > > instead of replacing columnHeaders will visibleRows, can you just make a new visibleRows method -- that should cut down the on the churn in the patch and make it easier to read > > I flipped these around to match their declaration order and make the group easier to find and parse. The group being: > > void columnHeaders(AccessibilityChildrenVector&); > void rowHeaders(AccessibilityChildrenVector&); > void visibleRows(AccessibilityChildrenVector&); > > > > > > Source/WebCore/accessibility/AccessibilityTable.cpp:462 > > > + unsigned rowCount = m_rows.size(); > > > > i think .size() returns a size_t so we should use that > > Indeed. I prefer this as well. I was trying to maintain parity with the surrounding methods but I updated them all to size_t. Which, since I was already touching them to remove the unnecessary continue statements as stated above, seems ok. > > Thanks as usual for the helpful feedback!
Created attachment 217207 [details] Updated patch for review.
(In reply to comment #16) > (In reply to comment #13) > > (In reply to comment #9) > > > (From update of attachment 217082 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=217082&action=review > > > > > > > LayoutTests/platform/mac/accessibility/table-visible-rows.html:38 > > > > + shouldBe("visibleRows", "3"); > > > > > > can you hide the content of this test when the test is done so that the ABCDE don't end up in the text expectations? > > > > Hmm. I don't see this being done anyplace else and it seems that it would be somewhat confusing as this text dump is often relied on as part of the test. > > > > For example, if the test involved removing a node, we would expect to NOT see that node in the dump. Also, if something changes that is causing nodes to be removed or lost, removing them in our tests would hide that. Right? > > A bunch of AX tests do this. Ryosuke has been recommending doing this to me. > > If media tests had done this then we wouldn't have had to modify 500 files when we made that recent <video> tag change. I think it makes the test output easier to read, because we're not testing the dump text routines Great point. Thanks for clarifying. Fixed. > > > > > > > > > > Source/WebCore/accessibility/AccessibilityTable.cpp:455 > > > > +void AccessibilityTable::visibleRows(AccessibilityChildrenVector& rows) > > > > > > instead of replacing columnHeaders will visibleRows, can you just make a new visibleRows method -- that should cut down the on the churn in the patch and make it easier to read > > > > I flipped these around to match their declaration order and make the group easier to find and parse. The group being: > > > > void columnHeaders(AccessibilityChildrenVector&); > > void rowHeaders(AccessibilityChildrenVector&); > > void visibleRows(AccessibilityChildrenVector&); > > > > > > > > > Source/WebCore/accessibility/AccessibilityTable.cpp:462 > > > > + unsigned rowCount = m_rows.size(); > > > > > > i think .size() returns a size_t so we should use that > > > > Indeed. I prefer this as well. I was trying to maintain parity with the surrounding methods but I updated them all to size_t. Which, since I was already touching them to remove the unnecessary continue statements as stated above, seems ok. > > > > Thanks as usual for the helpful feedback!
Comment on attachment 217207 [details] Updated patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=217207&action=review > Source/WebCore/accessibility/AccessibilityTable.cpp:435 > + if (header) the above line can be put inside the if if (AccessibilityObj *header = ) > Source/WebCore/accessibility/AccessibilityTable.cpp:451 > + headers.append(header); this seems like an unnecessary change
Created attachment 217236 [details] Update patch for review. Updates per review.
Comment on attachment 217236 [details] Update patch for review. Clearing flags on attachment: 217236 Committed r159468: <http://trac.webkit.org/changeset/159468>
All reviewed patches have been landed. Closing bug.