Bug 124430 - AX: Add ability to fetch only visible table rows.
Summary: AX: Add ability to fetch only visible table rows.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.9
: P2 Normal
Assignee: Samuel White
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-11-15 12:43 PST by Samuel White
Modified: 2013-11-18 16:30 PST (History)
11 users (show)

See Also:


Attachments
Initial patch for feedback only. (16.87 KB, patch)
2013-11-15 12:53 PST, Samuel White
no flags Details | Formatted Diff | Diff
Rebased feedback patch. (16.00 KB, patch)
2013-11-15 13:51 PST, Samuel White
no flags Details | Formatted Diff | Diff
Fixed patch for feedback. (16.00 KB, patch)
2013-11-15 13:57 PST, Samuel White
no flags Details | Formatted Diff | Diff
Repeat WITH fix. (16.00 KB, patch)
2013-11-15 13:58 PST, Samuel White
buildbot: commit-queue-
Details | Formatted Diff | Diff
Updated patch. (19.97 KB, patch)
2013-11-15 17:41 PST, Samuel White
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch for review. (20.99 KB, patch)
2013-11-18 10:26 PST, Samuel White
no flags Details | Formatted Diff | Diff
Update patch for review. (20.89 KB, patch)
2013-11-18 14:41 PST, Samuel White
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Samuel White 2013-11-15 12:43:01 PST
Currently AXRows and AXVisibleRows return the same thing. We have a FIXME line that should be fixed.
Comment 1 Radar WebKit Bug Importer 2013-11-15 12:43:27 PST
<rdar://problem/15483239>
Comment 2 Samuel White 2013-11-15 12:46:40 PST
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.
Comment 3 Samuel White 2013-11-15 12:53:09 PST
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.
Comment 4 Samuel White 2013-11-15 13:51:08 PST
Created attachment 217079 [details]
Rebased feedback patch.

Rebased patch for feedback to get EWS results.
Comment 5 WebKit Commit Bot 2013-11-15 13:53:09 PST
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.
Comment 6 Samuel White 2013-11-15 13:57:12 PST
Created attachment 217081 [details]
Fixed patch for feedback.

Removed semicolon that snuck in.
Comment 7 Samuel White 2013-11-15 13:58:59 PST
Created attachment 217082 [details]
Repeat WITH fix.

Oops. REAL fixed patch.
Comment 8 Build Bot 2013-11-15 14:40:33 PST
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 9 chris fleizach 2013-11-15 15:02:13 PST
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 10 EFL EWS Bot 2013-11-15 17:27:26 PST
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
Comment 11 Samuel White 2013-11-15 17:41:32 PST
Created attachment 217109 [details]
Updated patch.
Comment 12 EFL EWS Bot 2013-11-15 17:47:43 PST
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
Comment 13 Samuel White 2013-11-15 17:53:17 PST
(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!
Comment 14 Samuel White 2013-11-15 17:54:08 PST
Will fix the elf and win failures in next patch.
Comment 15 Build Bot 2013-11-15 18:24:48 PST
Comment on attachment 217109 [details]
Updated patch.

Attachment 217109 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/24488020
Comment 16 chris fleizach 2013-11-18 09:44:56 PST
(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!
Comment 17 Samuel White 2013-11-18 10:26:28 PST
Created attachment 217207 [details]
Updated patch for review.
Comment 18 Samuel White 2013-11-18 10:27:58 PST
(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 19 chris fleizach 2013-11-18 11:38:23 PST
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
Comment 20 Samuel White 2013-11-18 14:41:08 PST
Created attachment 217236 [details]
Update patch for review.

Updates per review.
Comment 21 WebKit Commit Bot 2013-11-18 16:30:25 PST
Comment on attachment 217236 [details]
Update patch for review.

Clearing flags on attachment: 217236

Committed r159468: <http://trac.webkit.org/changeset/159468>
Comment 22 WebKit Commit Bot 2013-11-18 16:30:28 PST
All reviewed patches have been landed.  Closing bug.