Bug 124293 – AX: Calling NSAccessibilityColumnsAttribute and NSAccessibilityRowsAttribute simply to get column/row count can be very expensive.

• Component: Accessibility
• Status: RESOLVED FIXED

[Samuel White]

VoiceOver needs to query for column and row count often when navigating a table. When the table is large, this can be VERY expensive to do over and over. Given the dynamic nature of many tables (like Facebook, Twitter) we need a lightweight way to get the count out easily.

Such as:
NSAccessibilityColumnCountAttribute
NSAccessibilityRowCountAttribute

[Radar WebKit Bug Importer]

<rdar://problem/15460808>

[Samuel White]

To clarify, the dynamic nature of the tables mentioned above means it's not feasible for AT to just cache these values.

[Samuel White]

Created attachment 216840 [details]
Patch.

[Samuel White]

Created attachment 216842 [details]
Correct patch.

[chris fleizach]

Comment on attachment 216842 [details]
Correct patch.

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

did you run the full ax test suite? i think there a few tests that dump all the attributes, which would start to fail by adding these new attributes

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2412
> +            return @(static_cast<AccessibilityTable*>(m_object)->columnCount());

I think there are toAccessibilityTable methods now.
also i would stay away from boxing unless you see it being used elsewhere

[Build Bot]

Comment on attachment 216842 [details]
Correct patch.

Attachment 216842 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22889098

New failing tests:
accessibility/table-detection.html
accessibility/table-with-rules.html

[Build Bot]

Created attachment 216855 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 216842 [details]
Correct patch.

Attachment 216842 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22909081

New failing tests:
accessibility/table-detection.html
accessibility/table-with-rules.html

[Build Bot]

Created attachment 216857 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 216842 [details]
Correct patch.

Attachment 216842 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22590037

New failing tests:
accessibility/table-detection.html
accessibility/table-with-rules.html

[Build Bot]

Created attachment 216860 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Samuel White]

Created attachment 216864 [details]
Updated patch.

[Samuel White]

(In reply to comment #5)
> (From update of attachment 216842 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216842&action=review
> 
> did you run the full ax test suite? i think there a few tests that dump all the attributes, which would start to fail by adding these new attributes

Fixed.

> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2412
> > +            return @(static_cast<AccessibilityTable*>(m_object)->columnCount());
> 
> I think there are toAccessibilityTable methods now.

Ah. Updated.

> also i would stay away from boxing unless you see it being used elsewhere

Auto boxing can be found in a few places in WebKit and WebCore.

Thanks.

[chris fleizach]

Comment on attachment 216864 [details]
Updated patch.

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:111
> +#define NSAccessibilityColumnCountAttribute @"AXColumnCount"

these already exist so no need to define again

[Samuel White]

Created attachment 216877 [details]
Updated patch for review.

[Samuel White]

(In reply to comment #14)
> (From update of attachment 216864 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216864&action=review
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:111
> > +#define NSAccessibilityColumnCountAttribute @"AXColumnCount"
> 
> these already exist so no need to define again

Indeed. Sadly, I invented these before I checked. Then forgot to pull them out on this end once I found out they already existed. Doh. Thanks.

[chris fleizach]

Comment on attachment 216877 [details]
Updated patch for review.

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2403
> +        if ([attributeName isEqualToString:NSAccessibilityColumnCountAttribute]) {

single line if statements don't need brackets

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2407
> +        if ([attributeName isEqualToString:NSAccessibilityRowCountAttribute]) {

ditto

[Samuel White]

Created attachment 216878 [details]
Updated patch for review.

[EFL EWS Bot]

Comment on attachment 216878 [details]
Updated patch for review.

Attachment 216878 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/22909125

[Samuel White]

Since this API addition is specific to the Mac platform I think it's safe to move this test to platform/mac/accessibility.

[Samuel White]

Created attachment 216949 [details]
Patch.

[WebKit Commit Bot]

Comment on attachment 216949 [details]
Patch.

Clearing flags on attachment: 216949

Committed r159295: <http://trac.webkit.org/changeset/159295>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.