Bug 201775 – FindController::findString always updates foundStringMatchIndex even if match is the same as before

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

FindController::findString will always either increment or decrement the match index. This is incorrect when the highlighted match is the same as before.

For example, say the search string "Hello" highlights some text in the sentence "Hello, world.". Match count is (for example) 10 and match index is 0. All is well. The search is refined to "Hello, world" and the highlighted area is expanded according to the larger match range, but does not move to a different match on the page. The match index increments to 1 despite highlighting the same match as before.

[Radar WebKit Bug Importer]

<rdar://problem/55352425>

Created attachment 380265 [details]
Patch

[Tim Horton]

Comment on attachment 380265 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +            Add NoIndexChange find option flag

No indentation

> Source/WebKit/ChangeLog:22
> +            Do not change match index if NoIndexChange bit is set. Otherwise, no change in behavior.

This is indented wrong

> Source/WebKit/Shared/WebFindOptions.h:39
> +    FindOptionsNoIndexChange = 1 << 8,

It's not a problem in this case, but a bit odd to add to the middle of an enum.

> Source/WebKit/WebProcess/WebPage/FindController.cpp:70
> +    if (options & FindOptionsNoIndexChange)
> +        result.add(WebCore::NoIndexChange);

You can avoid the WebCore change and the core() change; nothing ever uses this option in WebCore, right?

[Tim Horton]

I bet you can write an API test for this; peek at TestWebKitAPI's WKWebViewFindString.mm (or maybe one of the others).

Created attachment 380341 [details]
Patch

[Tim Horton]

Looks like the API test fails

Created attachment 380345 [details]
Patch

(In reply to Tim Horton from comment #6)
> Looks like the API test fails
Added test to old API -- oops. This run should be good.

Created attachment 380349 [details]
Patch

[Tim Horton]

Comment on attachment 380349 [details]
Patch

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

> Source/WebKit/Shared/API/c/WKFindOptions.h:44
> +    kWKFindOptionsShowHighlight = 1 << 7,
> +    kWKFindOptionsNoIndexChange = 1 << 8

You shouldn't have to add this

> Source/WebKit/Shared/API/c/WKSharedAPICast.h:772
> +    if (wkFindOptions & kWKFindOptionsNoIndexChange)
> +        findOptions |= FindOptionsNoIndexChange;

Or this.

> Source/WebKit/Shared/WebFindOptions.h:40
> +    FindOptionsNoIndexChange = 1 << 8,
> +    FindOptionsDetermineMatchIndex = 1 << 9,

Technically fine in this case because it's internal only but still weird to add things in the middle of the enum.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewFindString.mm:192
> +    [firstWebView _findString:@"hello" options:findOptions maxCount:maxCount];

This is ObjC API so it should always take ObjC API enums (_WKFindOptions and _WKFindOptionsNoIndexChange). What went wrong that you have to use the C API constant?! Looking at the implementation of _findString:options:maxCount: in WKWebView, it uses the toFindOptions() function that's right above it, which goes from _WKFindOptions->WebKit::FindOptions, so I think this is just wrong.

Created attachment 380909 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 380909 [details]
Patch

Clearing flags on attachment: 380909

Committed r251111: <https://trac.webkit.org/changeset/251111>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.