Bug 124561 - AX: Add AXUIElementCountForSearchPredicate parameterized attribute.
Summary: AX: Add AXUIElementCountForSearchPredicate parameterized attribute.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.9
: P2 Normal
Assignee: Samuel White
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-11-18 22:01 PST by Samuel White
Modified: 2013-12-03 15:20 PST (History)
15 users (show)

See Also:


Attachments
Patch for EWS. (35.59 KB, patch)
2013-11-20 10:57 PST, Samuel White
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch for EWS. (37.39 KB, patch)
2013-11-20 11:28 PST, Samuel White
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Last patch for EWS (I hope). (39.14 KB, patch)
2013-11-20 12:23 PST, Samuel White
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (486.39 KB, application/zip)
2013-11-20 13:25 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (613.75 KB, application/zip)
2013-11-20 15:31 PST, Build Bot
no flags Details
Patch for feedback. (42.52 KB, patch)
2013-11-20 17:31 PST, Samuel White
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (448.87 KB, application/zip)
2013-11-20 18:26 PST, Build Bot
no flags Details
Patch for review. (47.14 KB, patch)
2013-11-20 19:41 PST, Samuel White
cfleizach: review+
Details | Formatted Diff | Diff
Updated patch. (47.06 KB, patch)
2013-12-02 10:54 PST, Samuel White
cfleizach: review+
Details | Formatted Diff | Diff
Final patch. (47.06 KB, patch)
2013-12-02 11:29 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-18 22:01:45 PST
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.
Comment 1 Samuel White 2013-11-18 22:09:35 PST
<rdar://problem/15500340>
Comment 2 Samuel White 2013-11-20 10:57:57 PST
Created attachment 217457 [details]
Patch for EWS.

No layout test added yet. Just looking for non Mac platform failures via EWS.
Comment 3 EFL EWS Bot 2013-11-20 11:14:06 PST
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
Comment 4 kov's GTK+ EWS bot 2013-11-20 11:16:34 PST
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
Comment 5 Samuel White 2013-11-20 11:28:11 PST
Created attachment 217461 [details]
Updated patch for EWS.

Should fix non Mac platform issues. Patch not for review. Still needs layout test.
Comment 6 EFL EWS Bot 2013-11-20 11:44:54 PST
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
Comment 7 kov's GTK+ EWS bot 2013-11-20 12:00:14 PST
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
Comment 8 Build Bot 2013-11-20 12:13:32 PST
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
Comment 9 Samuel White 2013-11-20 12:23:36 PST
Created attachment 217465 [details]
Last patch for EWS (I hope).

Missed some DRT fixes. THIS should do it.
Comment 10 Build Bot 2013-11-20 13:25:49 PST
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
Comment 11 Build Bot 2013-11-20 13:25:51 PST
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
Comment 12 Build Bot 2013-11-20 15:30:58 PST
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
Comment 13 Build Bot 2013-11-20 15:31:02 PST
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
Comment 14 chris fleizach 2013-11-20 15:36:41 PST
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
Comment 15 Samuel White 2013-11-20 17:31:41 PST
Created attachment 217504 [details]
Patch for feedback.

Added layout test to complete patch. Making sure it passes and will update Changelogs.
Comment 16 Build Bot 2013-11-20 18:25:58 PST
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
Comment 17 Build Bot 2013-11-20 18:26:00 PST
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
Comment 18 Samuel White 2013-11-20 19:41:49 PST
Created attachment 217514 [details]
Patch for review.
Comment 19 Samuel White 2013-11-20 19:48:31 PST
(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.
Comment 20 Samuel White 2013-11-20 19:51:35 PST
(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.
Comment 21 Samuel White 2013-11-24 15:16:15 PST
Any additional feedback on this one Chris?
Comment 22 chris fleizach 2013-11-30 15:23:04 PST
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
Comment 23 Samuel White 2013-12-02 10:54:57 PST
Created attachment 218189 [details]
Updated patch.
Comment 24 Samuel White 2013-12-02 10:59:40 PST
(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.
Comment 25 chris fleizach 2013-12-02 11:08:43 PST
(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]
---------------------
Comment 26 Samuel White 2013-12-02 11:29:40 PST
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.
Comment 27 Samuel White 2013-12-02 11:37:21 PST
Comment on attachment 218198 [details]
Final patch.

Will commit manually if all tests pass.
Comment 28 Samuel White 2013-12-02 16:37:12 PST
Committed in http://trac.webkit.org/changeset/159980

Closing.
Comment 29 Alexey Proskuryakov 2013-12-03 14:21:10 PST
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.
Comment 30 Samuel White 2013-12-03 14:28:16 PST
(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.
Comment 31 Alexey Proskuryakov 2013-12-03 15:18:57 PST
OK, marked as flaky, and filed bug 125195 to track this.
Comment 32 Samuel White 2013-12-03 15:20:26 PST
(In reply to comment #31)
> OK, marked as flaky, and filed bug 125195 to track this.

Great, thank you.