Bug 197095 - Accessibility text search and selection API enhancements.
Summary: Accessibility text search and selection API enhancements.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-18 23:40 PDT by Andres Gonzalez
Modified: 2019-04-23 13:32 PDT (History)
13 users (show)

See Also:


Attachments
Patch (56.98 KB, patch)
2019-04-19 00:29 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.76 MB, application/zip)
2019-04-19 01:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (2.90 MB, application/zip)
2019-04-19 02:19 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-highsierra (3.06 MB, application/zip)
2019-04-19 04:35 PDT, Build Bot
no flags Details
Patch (73.64 KB, patch)
2019-04-20 12:59 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (75.43 KB, patch)
2019-04-22 14:34 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (75.32 KB, patch)
2019-04-23 10:22 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2019-04-18 23:40:29 PDT
Accessibility text search and selection API enhancements.
Comment 1 Andres Gonzalez 2019-04-19 00:29:17 PDT
Created attachment 367790 [details]
Patch
Comment 2 Build Bot 2019-04-19 00:31:50 PDT
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.
Comment 3 Build Bot 2019-04-19 01:47:37 PDT
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
Comment 4 Build Bot 2019-04-19 01:47:38 PDT
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
Comment 5 Build Bot 2019-04-19 02:19:33 PDT
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
Comment 6 Build Bot 2019-04-19 02:19:35 PDT
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
Comment 7 Build Bot 2019-04-19 04:35:46 PDT
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
Comment 8 Build Bot 2019-04-19 04:35:47 PDT
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
Comment 9 chris fleizach 2019-04-19 09:34:58 PDT
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
Comment 10 Andres Gonzalez 2019-04-20 12:59:47 PDT
Created attachment 367899 [details]
Patch
Comment 11 chris fleizach 2019-04-20 21:56:03 PDT
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
Comment 12 chris fleizach 2019-04-20 22:12:12 PDT
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
Comment 13 Andres Gonzalez 2019-04-22 14:34:45 PDT
Created attachment 367978 [details]
Patch
Comment 14 chris fleizach 2019-04-22 15:45:53 PDT
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?
Comment 15 Andres Gonzalez 2019-04-23 06:28:17 PDT
(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.
Comment 16 chris fleizach 2019-04-23 09:12:44 PDT
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
Comment 17 Andres Gonzalez 2019-04-23 10:22:02 PDT
Created attachment 368040 [details]
Patch
Comment 18 Andres Gonzalez 2019-04-23 10:27:36 PDT
(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.
Comment 19 ews-feeder 2019-04-23 12:53:09 PDT
Comment on attachment 368040 [details]
Patch

Rejecting attachment 368040 [details] from commit-queue.

andresg_22@apple.com 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.
Comment 20 WebKit Commit Bot 2019-04-23 13:31:56 PDT
Comment on attachment 368040 [details]
Patch

Clearing flags on attachment: 368040

Committed r244561: <https://trac.webkit.org/changeset/244561>
Comment 21 WebKit Commit Bot 2019-04-23 13:31:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Radar WebKit Bug Importer 2019-04-23 13:32:18 PDT
<rdar://problem/50141969>