Accessibility text search and selection API enhancements.

• Component: New Bugs
• Status: RESOLVED FIXED

[Andres Gonzalez]

Accessibility text search and selection API enhancements.

[Andres Gonzalez]

Created attachment 367790 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 7 in 11 files


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

[EWS Watchlist]

Comment on attachment 367790 [details]
Patch

Attachment 367790 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11928675

New failing tests:
accessibility/mac/search-text/search-text.html

[EWS Watchlist]

Created attachment 367792 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

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

[EWS Watchlist]

Comment on attachment 367790 [details]
Patch

Attachment 367790 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11928843

New failing tests:
accessibility/mac/search-text/search-text.html

[EWS Watchlist]

Created attachment 367793 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 367790 [details]
Patch

Attachment 367790 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11930211

New failing tests:
accessibility/mac/search-text/search-text.html

[EWS Watchlist]

Created attachment 367795 [details]
Archive of layout-test-results from ews101 for mac-highsierra

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

[chris fleizach]

Comment on attachment 367790 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +	- Split the existing SelectTextWithCriteria API into SearchTextWithCriteria and TextOperation.

8 spaces indent for these lines

> Source/WebCore/accessibility/AccessibilityObject.cpp:769
> +    ASSERT(!afterRange || afterRange->compareBoundaryPoints(Range::START_TO_START, *referenceRange).releaseReturnValue() >= 0);

whats the benefit of doing this assert over the previous one?

> Source/WebCore/accessibility/AccessibilityObject.cpp:846
> +RefPtr<Range> AccessibilityObject::findTextRange(Vector<String> const& searchStrings, RefPtr<Range> const& start, AccessibilitySearchTextDirection dir) const

dir -> direction

> Source/WebCore/accessibility/AccessibilityObject.cpp:854
> +        RefPtr<Range> foundAfter = rangeOfStringClosestToRangeInDirection(start.get(), AccessibilitySearchDirection::Next, searchStrings);

auto foundAfter
auto foundBefore

> Source/WebCore/accessibility/AccessibilityObject.cpp:882
> +        /* Collapse the range to the start unless searching from the end of the

I would make this a one line comment with //

> Source/WebCore/accessibility/AccessibilityObject.cpp:899
> +        found = findTextRange(criteria.searchStrings, startRange, criteria.direction);

we can probably write this is as

if (auto found = findTextRange(criteria.searchStrings, startRange, criteria.direction))
    result.append(found)

> Source/WebCore/accessibility/AccessibilityObject.cpp:905
> +            found = findTextRange(criteria.searchStrings, startRange, dir);

auto found

> Source/WebCore/accessibility/AccessibilityObject.cpp:922
> +Vector<String> AccessibilityObject::performTextOperation(AccessibilityTextOperation const& op)

op -> operation

> Source/WebCore/accessibility/AccessibilityObject.cpp:926
> +    Frame* frame = this->frame();

auto* frame

> Source/WebCore/accessibility/AccessibilityObject.cpp:930
> +    if (!op.textRanges.isEmpty()) {

early return

if (!operation.textRanges.isEmpty()
   return result

> Source/WebCore/accessibility/AccessibilityObject.cpp:1129
>      Node* node = this->node();

auto* node

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:617
> +    AccessibilityTextOperation op;

op -> operation

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:658
> +static AccessibilitySearchTextCriteria accessibilitySearchTextCriteriaForParameterizedAttribute(const NSDictionary* params)

objective * character should be on the right side (which follows ObjC guidelines instead of C++ guidelines. confusing I know!)

NSDictionary *pararms

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:662
> +    NSArray* searchStrings = [params objectForKey:NSAccessibilitySearchTextSearchStrings];

*searchStrings
 *start
 *direction

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:669
> +        for (NSString* searchString in searchStrings) {

*searchString

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:694
> +static AccessibilityTextOperation accessibilityTextOperationForParameterizedAttribute(WebAccessibilityObjectWrapper* obj, const NSDictionary *parameterizedAttribute)

*obj
 *parameterizedAttrs

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:696
> +    AccessibilityTextOperation op;

op = operation

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:698
> +    NSArray* markerRanges = [parameterizedAttribute objectForKey:NSAccessibilityTextOperationMarkerRanges];

*markerRanges
 *operationType
 *replacementString

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4177
> +        return (NSString*)String();

I think this is by default, I don't think you need to do this cast

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4182
> +        Vector<RefPtr<Range>> ranges = m_object->findTextRanges(criteria);

auto ranges

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4186
> +                [markers addObject:[self textMarkerRangeFromRange:range]];

can textMarkerRangeFromRange return nil? if so we should validate before adding to object

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4194
> +        Vector<String> opResult = m_object->performTextOperation(textOperation);

operationResult

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4196
> +            NSMutableArray* result = [NSMutableArray arrayWithCapacity:opResult.size()];

*result

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4197
> +            for (String str : opResult)

for (auto str : operationResult)

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4198
> +                [result addObject:(NSString*)str];

cast should be unnecessary

> Tools/ChangeLog:8
> +	- WebCore-JavaScript glue for SearchText.

8 character indent

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:222
> +static Vector<T> convertNSArrayToVector(NSArray* a)

*array

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:288
>  

*searchTextParameterizedAttributeForCriteria

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

*parameterizedAttribute

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1262
> +    columnHeadersVector = convertNSArrayToVector<RefPtr<AccessibilityUIElement>>(columnHeadersArray);

auto columnHeaders

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1273
> +    Vector<RefPtr<AccessibilityUIElement>> rowHeadersVector = convertNSArrayToVector<RefPtr<AccessibilityUIElement>>(rowHeadersArray);

auto rowHeadersVector

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1284
> +    Vector<RefPtr<AccessibilityUIElement> > columnsVector = convertNSArrayToVector<RefPtr<AccessibilityUIElement>>(columnsArray);

auto columnVector

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1295
> +    Vector<RefPtr<AccessibilityUIElement>> rowsVector = convertNSArrayToVector<RefPtr<AccessibilityUIElement>>(rowsArray);

auto

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1306
> +    Vector<RefPtr<AccessibilityUIElement>> cellsVector = convertNSArrayToVector<RefPtr<AccessibilityUIElement>>(cellsArray);

auto

> LayoutTests/ChangeLog:8
> +	- Added LayoutTest for AccessibilitySearchText API.

8 character indent

[Andres Gonzalez]

Created attachment 367899 [details]
Patch

[chris fleizach]

Looks like build failures on non Mac builds

/Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/DerivedSources/WebKitTestRunner/JSAccessibilityUIElement.cpp:1157:18: error: no member named 'searchTextWithCriteria' in 'WTR::AccessibilityUIElement'
    return impl->searchTextWithCriteria(context, searchStrings, startFrom.get(), direction.get());
           ~~~~  ^
1 error

[chris fleizach]

Comment on attachment 367899 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +- Split the existing SelectTextWithCriteria API into two: search text API

bad indendation here

> Source/WebCore/accessibility/AccessibilityObject.cpp:914
> +        ASSERT(false);

you have all the cases covered? you probably you don't need the default case at all. compiler should take care of that for you

> Source/WebCore/accessibility/AccessibilityObject.cpp:927
> +    Frame* frame = this->frame();

auto* frame

> Source/WebCore/accessibility/AccessibilityObject.cpp:932
> +        if (frame->selection().setSelectedRange(textRange.get(), DOWNSTREAM, FrameSelection::ShouldCloseTyping::Yes)) {

should probably do early break here

if (!frame->selection().......)
     continue;

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:664
> +    NSString *dir = [params objectForKey:NSAccessibilitySearchTextDirection];

dir -> direction

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:699
> +    NSString *opType = [parameterizedAttribute objectForKey:NSAccessibilityTextOperationType];

opType -> operationType

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4183
> +        if (!ranges.isEmpty()) {

if (ranges.isEmpty()
   return nil;

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4197
> +        if (!operationResult.isEmpty()) {

if (operationResult.isEmpty())
    return nil;

> Tools/ChangeLog:8
> +- Added new API JS binding code for searchTextWithCriteria to both WTR and DRT.

bad indentation

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:289
> +    JSStringRef startFrom = nullptr;

we should do a RefPtr<JSStringRef> so that we don't have to worry about the JSStringRelease at the end

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:296
> +    auto result = toAXElement(thisObject)->searchTextWithCriteria(context, searchStrings, startFrom, direction);

then you can return immediately

return toAXElemen......

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:1924
> +#if PLATFORM(MAC) && !PLATFORM(IOS_FAMILY)

I think MAC means it is not IOS_FAMILY, so no need to do the !PLATFORM(IOS_FAM

> Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:317
> +    elementVector = convertNSArrayToVector<AccessibilityUIElement>(linkedElements);

is this going to work by setting elementVector? does it implicitly copy all elements into a vector by overloading = ?

> LayoutTests/ChangeLog:8
> +- Added new test for AccessibilitySearchTextWithCriteria API.

bad indentation

[Andres Gonzalez]

Created attachment 367978 [details]
Patch

[chris fleizach]

Comment on attachment 367978 [details]
Patch

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

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:291
> +        startFrom = JSValueToStringCopy(context, arguments[1], exception);

we should be able to make these RefPtr<> so we don't have to release ourselves

> Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:188
> +        JSObjectSetPropertyAtIndex(context, arrayObj, i, JSObjectMake(context, elements[i].getJSClass(), new T(elements[i])), 0);

are we going to leak new T() object here? I think so

> Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:215
> +    return v;

can we WTFMove this back so that we don't have copy the array on return?

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:202
>          JSObjectSetPropertyAtIndex(context, arrayObj, i, JSObjectMake(context, elements[i]->wrapperClass(), elements[i].get()), 0);

does JSObjectMake return an object that needs to be released?

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:229
> +    return v;

this will copy the whole array again on return right? 
can we WTFMove this back instead?

[Andres Gonzalez]

(In reply to chris fleizach from comment #14)
> Comment on attachment 367978 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367978&action=review
> 
> > Tools/DumpRenderTree/AccessibilityUIElement.cpp:291
> > +        startFrom = JSValueToStringCopy(context, arguments[1], exception);
> 
> we should be able to make these RefPtr<> so we don't have to release
> ourselves

JSStringRef is an opaque object that cannot be wrapped in a smart pointer like RefPtr since RefPtr doesn't have access to its constructor/destructor.

> 
> > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:188
> > +        JSObjectSetPropertyAtIndex(context, arrayObj, i, JSObjectMake(context, elements[i].getJSClass(), new T(elements[i])), 0);
> 
> are we going to leak new T() object here? I think so

I believe not because JSObjectMake is creating the object as follows:

    JSCallbackObject<JSDestructibleObject>* object = JSCallbackObject<JSDestructibleObject>::create(exec, exec->lexicalGlobalObject(), exec->lexicalGlobalObject()->callbackObjectStructure(), jsClass, data);

which should release the inner object (data) of class jsClass on deletion. The implementation in WTR is better since it passes in a RefPtr to the inner class instead of a raw pointer to the inner class. But it is probably beyond the scope of this change to convert all DRT to using RefPtr<AccessibilityXXXX>.

> 
> > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:215
> > +    return v;
> 
> can we WTFMove this back so that we don't have copy the array on return?

The compiler is doing that for us thanks to return value optimization (RVO) or copy elision.
> 
> > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:202
> >          JSObjectSetPropertyAtIndex(context, arrayObj, i, JSObjectMake(context, elements[i]->wrapperClass(), elements[i].get()), 0);
> 
> does JSObjectMake return an object that needs to be released?

I believe it should be released when the container arrayObj is released.

> 
> > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:229
> > +    return v;
> 
> this will copy the whole array again on return right? 
> can we WTFMove this back instead?

RVO takes care of it.

[chris fleizach]

Comment on attachment 367978 [details]
Patch

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

>>> Tools/DumpRenderTree/AccessibilityUIElement.cpp:291
>>> +        startFrom = JSValueToStringCopy(context, arguments[1], exception);
>> 
>> we should be able to make these RefPtr<> so we don't have to release ourselves
> 
> JSStringRef is an opaque object that cannot be wrapped in a smart pointer like RefPtr since RefPtr doesn't have access to its constructor/destructor.

I think we actually want RetainPtr<JSStringRef> which will take care of the releasing

[Andres Gonzalez]

Created attachment 368040 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #16)
> Comment on attachment 367978 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367978&action=review
> 
> >>> Tools/DumpRenderTree/AccessibilityUIElement.cpp:291
> >>> +        startFrom = JSValueToStringCopy(context, arguments[1], exception);
> >> 
> >> we should be able to make these RefPtr<> so we don't have to release ourselves
> > 
> > JSStringRef is an opaque object that cannot be wrapped in a smart pointer like RefPtr since RefPtr doesn't have access to its constructor/destructor.
> 
> I think we actually want RetainPtr<JSStringRef> which will take care of the
> releasing

Ah, got it... Fixed in patch 368040.

[EWS]

Comment on attachment 368040 [details]
Patch

Rejecting attachment 368040 [details] from commit-queue.

[e-mail] does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 368040 [details]
Patch

Clearing flags on attachment: 368040

Committed r244561: <https://trac.webkit.org/changeset/244561>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50141969>