Bug 178789

Summary: Make iOS Find UI reveal matches in scrollable elements
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: New BugsAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: dvoytenko, malteubl, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=163911
Attachments:
Description Flags
Patch
none
Patch (tentative test)
none
Patch
none
Patch thorton: review+

Description Frédéric Wang (:fredw) 2017-10-25 06:54:53 PDT
Make iOS Find UI reveal matches in scrollable elements
Comment 1 Frédéric Wang (:fredw) 2017-10-25 06:57:16 PDT
Created attachment 324816 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2017-10-25 07:00:35 PDT
For some test cases, see attachment 308089 [details] and attachment 312006 [details]
Comment 3 Alexey Proskuryakov 2017-10-25 10:28:16 PDT
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?
Comment 4 Frédéric Wang (:fredw) 2017-10-25 10:40:37 PDT
(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.
Comment 5 Frédéric Wang (:fredw) 2017-10-26 04:12:39 PDT
Created attachment 324985 [details]
Patch (tentative test)
Comment 6 Frédéric Wang (:fredw) 2017-10-26 04:18:32 PDT
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).
Comment 7 Tim Horton 2017-10-26 09:22:52 PDT
(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:
Comment 8 Frédéric Wang (:fredw) 2017-10-27 07:31:12 PDT
Created attachment 325159 [details]
Patch
Comment 9 Frédéric Wang (:fredw) 2017-10-27 07:33:17 PDT
(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.
Comment 10 Tim Horton 2017-11-01 10:37:03 PDT
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?
Comment 11 Frédéric Wang (:fredw) 2017-11-01 11:41:24 PDT
Created attachment 325603 [details]
Patch
Comment 12 Frédéric Wang (:fredw) 2017-11-01 11:44:13 PDT
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.
Comment 13 Frédéric Wang (:fredw) 2017-11-01 11:50:45 PDT
Committed r224284: <https://trac.webkit.org/changeset/224284>
Comment 14 Radar WebKit Bug Importer 2017-11-15 12:47:49 PST
<rdar://problem/35568141>