Bug 124561 – AX: Add AXUIElementCountForSearchPredicate parameterized attribute.

• Component: Accessibility
• Status: RESOLVED FIXED

[Samuel White]

WebKit currently supports the AXUIElementsForSearchPredicate parameterized attribute. This lets VoiceOver fetch specific groups of elements very efficiently. However, VoiceOver also needs to support AXUIElementCountForSearchPredicate so it can inform users about the total number of elements that meet their search criteria without actually fetching everything.

[Samuel White]

<rdar://problem/15500340>

[Samuel White]

Created attachment 217457 [details]
Patch for EWS.

No layout test added yet. Just looking for non Mac platform failures via EWS.

[EFL EWS Bot]

Comment on attachment 217457 [details]
Patch for EWS.

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

[kov's GTK+ EWS bot]

Comment on attachment 217457 [details]
Patch for EWS.

Attachment 217457 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/29798026

[Samuel White]

Created attachment 217461 [details]
Updated patch for EWS.

Should fix non Mac platform issues. Patch not for review. Still needs layout test.

[EFL EWS Bot]

Comment on attachment 217461 [details]
Updated patch for EWS.

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

[kov's GTK+ EWS bot]

Comment on attachment 217461 [details]
Updated patch for EWS.

Attachment 217461 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/30798002

[Build Bot]

Comment on attachment 217461 [details]
Updated patch for EWS.

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

[Samuel White]

Created attachment 217465 [details]
Last patch for EWS (I hope).

Missed some DRT fixes. THIS should do it.

[Build Bot]

Comment on attachment 217465 [details]
Last patch for EWS (I hope).

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

New failing tests:
platform/mac/accessibility/search-predicate-element-count.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 217465 [details]
Last patch for EWS (I hope).

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

New failing tests:
platform/mac/accessibility/search-predicate-element-count.html

[Build Bot]

Created attachment 217485 [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

[chris fleizach]

Comment on attachment 217465 [details]
Last patch for EWS (I hope).

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:550
> +    id searchKeyParameter = [parameterizedAttribute objectForKey:@"AXSearchKey"];

you should start these objects out as the type that they're expected, so we don't have to cast (to CFStringRef), and won't run into any other weird namespace collisions

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:580
> +            id searchKey = [searchKeyParameter objectAtIndex:i];

ditto for type

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:582
> +                criteria.searchKeys.uncheckedAppend(accessibilitySearchKeyForString((CFStringRef)searchKey));

probably CFStringRef not needed

[Samuel White]

Created attachment 217504 [details]
Patch for feedback.

Added layout test to complete patch. Making sure it passes and will update Changelogs.

[Build Bot]

Comment on attachment 217504 [details]
Patch for feedback.

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

New failing tests:
platform/mac/accessibility/search-predicate-element-count.html

[Build Bot]

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

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

[Samuel White]

Created attachment 217514 [details]
Patch for review.

[Samuel White]

(In reply to comment #14)
> (From update of attachment 217465 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217465&action=review
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:550
> > +    id searchKeyParameter = [parameterizedAttribute objectForKey:@"AXSearchKey"];
> 
> you should start these objects out as the type that they're expected, so we don't have to cast (to CFStringRef), and won't run into any other weird namespace collisions

The object referenced by each pointer is validated with isKindOfClass: before being messaged so collisions aren't an issue for us here. However, your suggestion helps with readability so I've included it.

> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:580
> > +            id searchKey = [searchKeyParameter objectAtIndex:i];
> 
> ditto for type
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:582
> > +                criteria.searchKeys.uncheckedAppend(accessibilitySearchKeyForString((CFStringRef)searchKey));
> 
> probably CFStringRef not needed

I thought these casts made the somewhat tricky CFStringRef/String backing clearer. Since I've followed your above advice, this became redundant. Removed.

Thanks.

[Samuel White]

(In reply to comment #19)
> (In reply to comment #14)
> > (From update of attachment 217465 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=217465&action=review
> > 
> > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:550
> > > +    id searchKeyParameter = [parameterizedAttribute objectForKey:@"AXSearchKey"];
> > 
> > you should start these objects out as the type that they're expected, so we don't have to cast (to CFStringRef), and won't run into any other weird namespace collisions
> 
> The object referenced by each pointer is validated with isKindOfClass: before being messaged so collisions aren't an issue for us here. However, your suggestion helps with readability so I've included it.
> 
> > 
> > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:580
> > > +            id searchKey = [searchKeyParameter objectAtIndex:i];
> > 
> > ditto for type
> > 
> > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:582
> > > +                criteria.searchKeys.uncheckedAppend(accessibilitySearchKeyForString((CFStringRef)searchKey));
> > 
> > probably CFStringRef not needed
> 
> I thought these casts made the somewhat tricky CFStringRef/String backing clearer. Since I've followed your above advice, this became redundant. Removed.

Oops, to clarify, I meant this cast:

searchText = (CFStringRef)searchTextParameter;

As the relationship between String type and CFStringRef type is non obvious (imo).

the others were legacy.

> 
> Thanks.

[Samuel White]

Any additional feedback on this one Chris?

[chris fleizach]

Comment on attachment 217514 [details]
Patch for review.

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

> Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:200
> +    NSMutableDictionary* parameterizedAttribute = [NSMutableDictionary dictionary];

this * should be on the other side since it's an ObjC object

> Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:213
> +            if (searchKeyString) {

braces not necessary since it's a single line

> Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:1017
> +    NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, UINT_MAX, searchKey, searchText, visibleOnly);

* on wrong side

> Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:1029
> +    NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, 1, searchKey, searchText, visibleOnly);

* on wrong side

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:215
> +    NSMutableDictionary* parameterizedAttribute = [NSMutableDictionary dictionary];

* on wrong side

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:228
> +            if (searchKeyString) {

braces not necessary

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1046
> +    NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, UINT_MAX, searchKey, searchText, visibleOnly);

* on wrong side

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1058
> +    NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, 1, searchKey, searchText, visibleOnly);

* on wrong side

[Samuel White]

Created attachment 218189 [details]
Updated patch.

[Samuel White]

(In reply to comment #22)
> (From update of attachment 217514 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217514&action=review
> 
> > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:200
> > +    NSMutableDictionary* parameterizedAttribute = [NSMutableDictionary dictionary];
> 
> this * should be on the other side since it's an ObjC object
> 
> > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:213
> > +            if (searchKeyString) {
> 
> braces not necessary since it's a single line

Fixed.

> 
> > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:1017
> > +    NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, UINT_MAX, searchKey, searchText, visibleOnly);
> 
> * on wrong side
> 
> > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:1029
> > +    NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, 1, searchKey, searchText, visibleOnly);
> 
> * on wrong side
> 
> > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:215
> > +    NSMutableDictionary* parameterizedAttribute = [NSMutableDictionary dictionary];
> 
> * on wrong side
> 
> > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:228
> > +            if (searchKeyString) {
> 
> braces not necessary

Fixed.

> 
> > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1046
> > +    NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, UINT_MAX, searchKey, searchText, visibleOnly);
> 
> * on wrong side
> 
> > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1058
> > +    NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, 1, searchKey, searchText, visibleOnly);
> 
> * on wrong side

Why are we considering "NSString* str" wrong and "NSString *str" correct? If I look at the files you mention, the vast majority of our obj-c code is of the style "NSString* str". Is this something you hope to change moving forward? If so, I can adopt this style per your request. Thanks.

[chris fleizach]

(In reply to comment #24)
> (In reply to comment #22)
> > (From update of attachment 217514 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=217514&action=review
> > 
> > > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:200
> > > +    NSMutableDictionary* parameterizedAttribute = [NSMutableDictionary dictionary];
> > 
> > this * should be on the other side since it's an ObjC object
> > 
> > > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:213
> > > +            if (searchKeyString) {
> > 
> > braces not necessary since it's a single line
> 
> Fixed.
> 
> > 
> > > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:1017
> > > +    NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, UINT_MAX, searchKey, searchText, visibleOnly);
> > 
> > * on wrong side
> > 
> > > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:1029
> > > +    NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, 1, searchKey, searchText, visibleOnly);
> > 
> > * on wrong side
> > 
> > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:215
> > > +    NSMutableDictionary* parameterizedAttribute = [NSMutableDictionary dictionary];
> > 
> > * on wrong side
> > 
> > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:228
> > > +            if (searchKeyString) {
> > 
> > braces not necessary
> 
> Fixed.
> 
> > 
> > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1046
> > > +    NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, UINT_MAX, searchKey, searchText, visibleOnly);
> > 
> > * on wrong side
> > 
> > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1058
> > > +    NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, 1, searchKey, searchText, visibleOnly);
> > 
> > * on wrong side
> 
> Why are we considering "NSString* str" wrong and "NSString *str" correct? If I look at the files you mention, the vast majority of our obj-c code is of the style "NSString* str". Is this something you hope to change moving forward? If so, I can adopt this style per your request. Thanks.

For objectiveC objects it should be like "NSString *a" -- a lot of code doesn't conform to this standard, but i guess for code we touch we should slowly start to move them into correct form
---------------
Under Pointer & Referneces
http://www.webkit.org/coding/coding-style.html
Pointer types in non-C++ code — Pointer types should be written with a space between the type and the * (so the * is adjacent to the following identifier if any). [pointers-non-cpp]
---------------------

[Samuel White]

Created attachment 218198 [details]
Final patch.

Moved * in every file being compiled as obj-c++ to meet spec. Chris mentioned.

Should be ready to go. Thanks.

[Samuel White]

Comment on attachment 218198 [details]
Final patch.

Will commit manually if all tests pass.

[Samuel White]

Committed in http://trac.webkit.org/changeset/159980

Closing.

[Alexey Proskuryakov]

platform/mac/accessibility/search-predicate-element-count.html is failing on all OS X versions. I'm going to roll out soon, unless someone speaks up, because this affects EWS reliability.

[Samuel White]

(In reply to comment #29)
> platform/mac/accessibility/search-predicate-element-count.html is failing on all OS X versions. I'm going to roll out soon, unless someone speaks up, because this affects EWS reliability.

The test only fails when run in a group. I suggest we mark it as flaky until I can get to the bottom o fit.

[Alexey Proskuryakov]

OK, marked as flaky, and filed bug 125195 to track this.

[Samuel White]

(In reply to comment #31)
> OK, marked as flaky, and filed bug 125195 to track this.

Great, thank you.