Bug 112276 – Allow multiple searchKeys to be passed to AXUIElementCopyParameterizedAttributeValue

• Component: Accessibility
• Status: RESOLVED FIXED

[Greg Hughes]

Accessibility clients need to be able to request multiple different element types when using AXUIElementCopyParameterizedAttributeValue. Currently to get a list of links and buttons clients need to make multiple requests to AXUIElementCopyParameterizedAttributeValue and then compare the positions of returned elements to sort them accordingly. 

This has a negative impact on performance for webKit and clients.

Clients should be allowed to pass an NSArray of searchKeys to AXUIElementCopyParameterizedAttributeValue

[Greg Hughes]

Created attachment 192964 [details]
patch

[Greg Hughes]

<rdar://13412692>

[WebKit Review Bot]

Attachment 192964 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/accessibility/AccessibilityObject.cpp', u'Source/WebCore/accessibility/AccessibilityObject.h', u'Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm']" exit_code: 1
Source/WebCore/accessibility/AccessibilityObject.cpp:126:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebCore/accessibility/AccessibilityObject.cpp:125:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 192964 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:121
> +    unsigned length = criteria->searchKeys->size();

my guess is that size() is a size_t object

> Source/WebCore/accessibility/AccessibilityObject.cpp:124
> +        switch (criteria->searchKeys->at(i)) {

all these changes here are unnecessary and they'll fail the style guidelines

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3187
> +        if ([searchKeyEntry isKindOfClass:[NSString self]])

[NSString class]

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3189
> +        else if ([searchKeyEntry isKindOfClass:[NSArray self]]) {

[NSArray class]

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3190
> +            unsigned length = [(NSArray *)searchKeyEntry count];

i believe count is a NSUInteger

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3192
> +            for (unsigned i = 0; i < length; ++i) {

size length is NSUInteger, so should i

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3194
> +                if ([searchKey isKindOfClass:[NSString self]])

[NSString class]

[Greg Hughes]

Comment on attachment 192964 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityObject.cpp:121
>> +    unsigned length = criteria->searchKeys->size();
> 
> my guess is that size() is a size_t object

fixed in next patch

>> Source/WebCore/accessibility/AccessibilityObject.cpp:124
>> +        switch (criteria->searchKeys->at(i)) {
> 
> all these changes here are unnecessary and they'll fail the style guidelines

fixed in next patch (also moved switch statement to is own function to make the loop easier to read)

>> Source/WebCore/accessibility/AccessibilityObject.cpp:125
>> +                // The AnyTypeSearchKey matches any non-null AccessibilityObject.
> 
> When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]

fixed in next patch

>> Source/WebCore/accessibility/AccessibilityObject.cpp:126
>> +            case AnyTypeSearchKey:
> 
> A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]

fixed in next patch

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3187
>> +        if ([searchKeyEntry isKindOfClass:[NSString self]])
> 
> [NSString class]

fixed in next patch

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3189
>> +        else if ([searchKeyEntry isKindOfClass:[NSArray self]]) {
> 
> [NSArray class]

fixed in next patch

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3190
>> +            unsigned length = [(NSArray *)searchKeyEntry count];
> 
> i believe count is a NSUInteger

fixed in next patch (downcast to a size_t because reserveInitialCapacity takes a size_t)

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3192
>> +            for (unsigned i = 0; i < length; ++i) {
> 
> size length is NSUInteger, so should i

fixed in next patch (downcast to a size_t because reserveInitialCapacity takes a size_t)

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3194
>> +                if ([searchKey isKindOfClass:[NSString self]])
> 
> [NSString class]

fixed in next patch

[Greg Hughes]

Created attachment 193031 [details]
patch v3

[chris fleizach]

Comment on attachment 193031 [details]
patch v3

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

you also need a layout test. see LayoutTests/platform/mac/accessibility/search-predicate something or other for an example of using this functionality

> Source/WebCore/ChangeLog:5
> +        https://bugs.webkit.org/show_bug.cgi?id=112276

no new line between title and link

> Source/WebCore/ChangeLog:6
> +

add description of what this patch does

> Source/WebCore/ChangeLog:7
> +        Reviewed by Chris Fleizach

don't put my name here, leave it as blank

> Source/WebCore/accessibility/AccessibilityObject.cpp:115
> +bool AccessibilityObject::isAccessibilityObjectSearchKeyMatch(AccessibilityObject* axObject, AccessibilitySearchKey searchKey, AccessibilityObject *startObject)

* is in wrong place for startObject

> Source/WebCore/accessibility/AccessibilityObject.cpp:256
> +        if (isAccessibilityObjectSearchKeyMatch(axObject, criteria->searchKeys->at(i), criteria->startObject))

you should pass in the index of the search key criteria instead of breaking up the criteria like this. that will be less churn overall.
so i imagine the method would take, axObject, criteria, searchKeyIndex

using the at() method is not necessary, you should be able to do searchKeys[i]

> Source/WebCore/accessibility/AccessibilityObject.h:315
> +    Vector<AccessibilitySearchKey> *searchKeys;

i don't think you need to make a pointer out of this. it can just be a Vector<>
then you can make a constructor that takes in the other objects and just use the Vector<> that you get out of the criteria

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3190
> +            size_t length = (size_t)[(NSArray *)searchKeyEntry count];

i think you'll do less casting if you leave this as a NSUINteger and then just do one static_cast<size_t> in reserveInitialCapacity

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3192
> +            for (size_t i = 0; i < length; ++i) {

i should be a NUINteger because it's used to access obejctAtIndex: which expects that type

[Greg Hughes]

Created attachment 193169 [details]
patch v4

[chris fleizach]

Comment on attachment 193169 [details]
patch v4

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

overall looks good, but change log isn't correct and you need a layout test

> Source/WebCore/ChangeLog:3
> +        Allow multiple searchKeys to be passed to AXUIElementCopyParameterizedAttributeValue

please use prepare-Changelog --bug=X to make the appropriate change log
then check other change log entries

[Greg Hughes]

Created attachment 193594 [details]
Patch v5, now with layout tests

[Greg Hughes]

The style fails for having a variable name in the header, but that entire header has variable names, so instead of changing the entire header I left it as is.

./Tools/Scripts/run-webkit-tests --debug accessibility
"All 295 tests ran as expected."

[WebKit Review Bot]

Attachment 193594 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/mac/accessibility/search-predicate-expected.txt', u'LayoutTests/platform/mac/accessibility/search-predicate.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/AccessibilityObject.cpp', u'Source/WebCore/accessibility/AccessibilityObject.h', u'Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm', u'Tools/ChangeLog', u'Tools/DumpRenderTree/AccessibilityUIElement.cpp', u'Tools/DumpRenderTree/AccessibilityUIElement.h', u'Tools/DumpRenderTree/ios/AccessibilityUIElementIOS.mm', 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.h:205:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/DumpRenderTree/AccessibilityUIElement.h:202:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 193594 [details]
Patch v5, now with layout tests

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

> Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:945
> +                    if ( searchKeyParameter == nil )

this should just me if (searchKeyParameter)

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:939
> +            JSObjectRef array = const_cast<JSObjectRef>(searchKey);

i don't think you want a const_cast. that's for removing const-ness. i think you want a static case. however if there's a toJSObjectRef() method that would be better

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:956
> +                    if ( searchKeyParameter == nil )

same issue here

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:958
> +                    [(NSMutableArray *)searchKeyParameter addObject:[NSString stringWithJSStringRef:singleSearchKey]];

i think you should define the searchKeyParameter as the type your using it as in each code block, so you don't have to cast like this

> LayoutTests/platform/mac/accessibility/search-predicate.html:266
> +        // Heading OR link search

Comments should be a full sentence

> LayoutTests/platform/mac/accessibility/search-predicate.html:268
> +        resultElement = containerElement.uiElementForSearchPredicate(startElement, true, ["AXHeadingLevel2SearchKey","AXLinkSearchKey"], "");

space after the comma between the items in the array

> LayoutTests/platform/mac/accessibility/search-predicate.html:272
> +        // continue from result element

ditto about comment

> LayoutTests/platform/mac/accessibility/search-predicate.html:273
> +        resultElement = containerElement.uiElementForSearchPredicate(resultElement, true, ["AXHeadingLevel2SearchKey","AXLinkSearchKey"], "");

ditto about comma spacing

> LayoutTests/platform/mac/accessibility/search-predicate.html:277
> +        // going back should bring us back to the heading

ditto about comment

> LayoutTests/platform/mac/accessibility/search-predicate.html:278
> +        resultElement = containerElement.uiElementForSearchPredicate(resultElement, false, ["AXHeadingLevel2SearchKey","AXLinkSearchKey"], "");

ditto about comma spacing

[Greg Hughes]

const_cast has to be used and it is what is used in other places in WebKit to do the same thing, i.e.
EventSendingController.cpp:86
    JSObjectRef array = const_cast<JSObjectRef>(arrayValue);

[Greg Hughes]

Created attachment 193618 [details]
Patch v6

[Greg Hughes]

Created attachment 193619 [details]
patch v6 (second try)

[WebKit Review Bot]

Attachment 193619 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/mac/accessibility/search-predicate-expected.txt', u'LayoutTests/platform/mac/accessibility/search-predicate.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/AccessibilityObject.cpp', u'Source/WebCore/accessibility/AccessibilityObject.h', u'Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm', u'Tools/ChangeLog', u'Tools/DumpRenderTree/AccessibilityUIElement.cpp', u'Tools/DumpRenderTree/AccessibilityUIElement.h', u'Tools/DumpRenderTree/ios/AccessibilityUIElementIOS.mm', 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.h:205:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/DumpRenderTree/AccessibilityUIElement.h:202:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 193619 [details]
patch v6 (second try)

Attachment 193619 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17202351

[EFL EWS Bot]

Comment on attachment 193619 [details]
patch v6 (second try)

Attachment 193619 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17205571

[Build Bot]

Comment on attachment 193619 [details]
patch v6 (second try)

Attachment 193619 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17234324

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

[Build Bot]

Comment on attachment 193619 [details]
patch v6 (second try)

Attachment 193619 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17220304

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

[Greg Hughes]

Created attachment 195377 [details]
Patch v7

[Greg Hughes]

Created attachment 195378 [details]
Patch v7 (attempt 2)

[EFL EWS Bot]

Comment on attachment 195378 [details]
Patch v7 (attempt 2)

Attachment 195378 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17227718

[Build Bot]

Comment on attachment 195378 [details]
Patch v7 (attempt 2)

Attachment 195378 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17298356

[Build Bot]

Comment on attachment 195378 [details]
Patch v7 (attempt 2)

Attachment 195378 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17331079

New failing tests:
media/video-zoom.html
platform/mac/accessibility/search-predicate.html

[Build Bot]

Created attachment 195592 [details]
Archive of layout-test-results from webkit-ews-04 for mac-future

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

[Greg Hughes]

Created attachment 195642 [details]
Patch v8

[kov's GTK+ EWS bot]

Comment on attachment 195642 [details]
Patch v8

Attachment 195642 [details] did not pass gtk-ews (gtk):
Output: http://webkit-commit-queue.appspot.com/results/17355092

[Greg Hughes]

Created attachment 195649 [details]
Patch v9

[chris fleizach]

Comment on attachment 195649 [details]
Patch v9

there are no change logs in this patch

[Greg Hughes]

Created attachment 195762 [details]
patch v10

[WebKit Review Bot]

Comment on attachment 195762 [details]
patch v10

Clearing flags on attachment: 195762

Committed r147236: <http://trac.webkit.org/changeset/147236>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.