Bug 124430 – AX: Add ability to fetch only visible table rows.

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

[Samuel White]

Currently AXRows and AXVisibleRows return the same thing. We have a FIXME line that should be fixed.

[Radar WebKit Bug Importer]

<rdar://problem/15483239>

[Samuel White]

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.

[Samuel White]

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.

[Samuel White]

Created attachment 217079 [details]
Rebased feedback patch.

Rebased patch for feedback to get EWS results.

[WebKit Commit Bot]

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.

[Samuel White]

Created attachment 217081 [details]
Fixed patch for feedback.

Removed semicolon that snuck in.

[Samuel White]

Created attachment 217082 [details]
Repeat WITH fix.

Oops. REAL fixed patch.

[Build Bot]

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

[chris fleizach]

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

[EFL EWS Bot]

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

[Samuel White]

Created attachment 217109 [details]
Updated patch.

[EFL EWS Bot]

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

[Samuel White]

(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!

[Samuel White]

Will fix the elf and win failures in next patch.

[Build Bot]

Comment on attachment 217109 [details]
Updated patch.

Attachment 217109 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/24488020

[chris fleizach]

(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!

[Samuel White]

Created attachment 217207 [details]
Updated patch for review.

[Samuel White]

(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!

[chris fleizach]

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

[Samuel White]

Created attachment 217236 [details]
Update patch for review.

Updates per review.

[WebKit Commit Bot]

Comment on attachment 217236 [details]
Update patch for review.

Clearing flags on attachment: 217236

Committed r159468: <http://trac.webkit.org/changeset/159468>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.