Bug 178789 – Make iOS Find UI reveal matches in scrollable elements

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

[Frédéric Wang (:fredw)]

Make iOS Find UI reveal matches in scrollable elements

[Frédéric Wang (:fredw)]

Created attachment 324816 [details]
Patch

[Frédéric Wang (:fredw)]

For some test cases, see attachment 308089 [details] and attachment 312006 [details]

[Alexey Proskuryakov]

Comment on attachment 324816 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        Make iOS Find UI reveal matches in scrollable elements

Can a test be made for this change?

[Frédéric Wang (:fredw)]

(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 324816 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324816&action=review
> 
> > Source/WebKit/ChangeLog:3
> > +        Make iOS Find UI reveal matches in scrollable elements
> 
> Can a test be made for this change?

It seems that old changes in this part of the code do not have tests...  I can test manually, but I'm not sure how to do it in the test runner. Does anyone know how to simulate opening the Find UI and searching some text?

Also, note that window.find does not follow the same code path (bug 163911 comment 18) so it does not make sense to use that API to test this change.

[Frédéric Wang (:fredw)]

Created attachment 324985 [details]
Patch (tentative test)

[Frédéric Wang (:fredw)]

Comment on attachment 324816 [details]
Patch

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

>>> Source/WebKit/ChangeLog:3
>>> +        Make iOS Find UI reveal matches in scrollable elements
>> 
>> Can a test be made for this change?
> 
> It seems that old changes in this part of the code do not have tests...  I can test manually, but I'm not sure how to do it in the test runner. Does anyone know how to simulate opening the Find UI and searching some text?
> 
> Also, note that window.find does not follow the same code path (bug 163911 comment 18) so it does not make sense to use that API to test this change.

I tried again today. It seems that the test runner webview does not have the same UI as Safari. Trying to send Cmd+F with eventSender fails (see attachment 324985 [details]). In Safari, we can also use the "Share" menu to execute "Find in Page" but again that UI is not visible in the test runner webview. Finally, I also tried the command document.execCommand("FindString", true, "match") but again, it follows a different code path and won't allow to test that change (see bug 163911 comment 19).

[Tim Horton]

(In reply to Frédéric Wang (:fredw) from comment #6)
> Comment on attachment 324816 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324816&action=review
> 
> >>> Source/WebKit/ChangeLog:3
> >>> +        Make iOS Find UI reveal matches in scrollable elements
> >> 
> >> Can a test be made for this change?
> > 
> > It seems that old changes in this part of the code do not have tests...  I can test manually, but I'm not sure how to do it in the test runner. Does anyone know how to simulate opening the Find UI and searching some text?
> > 
> > Also, note that window.find does not follow the same code path (bug 163911 comment 18) so it does not make sense to use that API to test this change.
> 
> I tried again today. It seems that the test runner webview does not have the
> same UI as Safari. Trying to send Cmd+F with eventSender fails (see
> attachment 324985 [details]). In Safari, we can also use the "Share" menu to
> execute "Find in Page" but again that UI is not visible in the test runner
> webview. Finally, I also tried the command
> document.execCommand("FindString", true, "match") but again, it follows a
> different code path and won't allow to test that change (see bug 163911
> comment 19).

Like smfr said in the other bug, you probably want a UIScriptController function that invokes WKWebView's _findString:options:maxCount:

[Frédéric Wang (:fredw)]

Created attachment 325159 [details]
Patch

[Frédéric Wang (:fredw)]

(In reply to Tim Horton from comment #7)
> Like smfr said in the other bug, you probably want a UIScriptController
> function that invokes WKWebView's _findString:options:maxCount:

Thanks Tim. I uploaded a new patch with a test to check the effect of the change.

[Tim Horton]

Comment on attachment 325159 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/FindController.cpp:254
> +#if PLATFORM(IOS)

Can we hide this in FindControllerIOS instead? Maybe in didFindString?

[Frédéric Wang (:fredw)]

Created attachment 325603 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 325159 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/FindController.cpp:254
>> +#if PLATFORM(IOS)
> 
> Can we hide this in FindControllerIOS instead? Maybe in didFindString?

Right, thanks. Done in the latest patch.

[Frédéric Wang (:fredw)]

Committed r224284: <https://trac.webkit.org/changeset/224284>

[Radar WebKit Bug Importer]

<rdar://problem/35568141>

[Frédéric Wang (:fredw)]

Created attachment 353082 [details]
Patch (try to implement findString in DumpRenderTree)

Simon had suggested to test scrolling on WK1. Just for the record, I'm attaching an attempt to implement UIScriptController::findString for DumpRenderTree's UIScriptControllerIOS. Unfortunately, WebKitLegacy's WebFindOptions does not have the ShowFindIndicator option required by find-text-in-overflow-node.html and WebView does not even expose findString (only the more limited searchFor function). So I believe we can not run this test on WK1.