Bug 190621 – [iOS] [Datalist] Can't pick datalist suggestions in a stock WKWebView

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Wenson Hsieh]

CRASHING TEST: fast/forms/datalist/datalist-textinput-keydown.html
*** Terminating app due to uncaught exception 'WebKitTestRunnerTestProblem', reason: 'The test completed before all synthesized events had been handled. Perhaps you're calling notifyDone() too early?'
terminating with uncaught exception of type NSException
abort() called
CoreSimulator 576 - Device: iPhone SE - Runtime: iOS 12.0 (16A405) - DeviceType: iPhone SE

Application Specific Backtrace 1:
0   CoreFoundation                      0x000000011717c29b __exceptionPreprocess + 331
1   libobjc.A.dylib                     0x0000000111691735 objc_exception_throw + 48
2   CoreFoundation                      0x000000011717c0f5 +[NSException raise:format:] + 197
3   WebKitTestRunnerApp                 0x000000010b988f52 _ZN3WTR18UIScriptController28checkForOutstandingCallbacksEv + 98
4   WebKitTestRunnerApp                 0x000000010b980edb _ZN3WTR15UIScriptContextD2Ev + 43
5   WebKitTestRunnerApp                 0x000000010b980fd5 _ZN3WTR15UIScriptContextD1Ev + 21
6   WebKitTestRunnerApp                 0x000000010b97268e _ZN3WTR14TestInvocationD2Ev + 238
7   WebKitTestRunnerApp                 0x000000010b9727a5 _ZN3WTR14TestInvocationD1Ev + 21
8   WebKitTestRunnerApp                 0x000000010b94205a _ZN3WTR14TestController7runTestEPKc + 2314
9   WebKitTestRunnerApp                 0x000000010b945d69 _ZN3WTR14TestController20runTestingServerLoopEv + 265
10  WebKitTestRunnerApp                 0x000000010b932786 _ZN3WTR14TestController3runEv + 54
11  WebKitTestRunnerApp                 0x000000010b932109 _ZN3WTR14TestControllerC2EiPPKc + 1497
12  WebKitTestRunnerApp                 0x000000010b932963 _ZN3WTR14TestControllerC1EiPPKc + 35
13  WebKitTestRunnerApp                 0x000000010b91e66f -[WebKitTestRunnerApp _runTestController] + 47
14  Foundation                          0x0000000110f44e7b __NSThreadPerformPerform + 330
15  CoreFoundation                      0x00000001170dfb31 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
16  CoreFoundation                      0x00000001170df464 __CFRunLoopDoSources0 + 436
17  CoreFoundation                      0x00000001170d9a4f __CFRunLoopRun + 1263
18  CoreFoundation                      0x00000001170d9221 CFRunLoopRunSpecific + 625
19  GraphicsServices                    0x00000001121131dd GSEventRunModal + 62
20  UIKitCore                           0x000000012fbd5431 UIApplicationMain + 140
21  WebKitTestRunnerApp                 0x000000010b91e81c main + 140
22  libdyld.dylib                       0x0000000112527551 start + 1
23  ???                                 0x0000000000000002 0x0 + 2

Looks like this test needs to be adjusted to wait for HIDEventGenerator to finish dispatchhing HID events.

[Radar WebKit Bug Importer]

<rdar://problem/45310649>

[Wenson Hsieh]

As it turns out, the datalist suggestions dropdown doesn't quite work in a plain WKWebView unless the WKWebView has a _WKInputDelegate that responds to -_webView:(did|will)StartInputSession:, since it depends on WebKit creating a _WKFormInputSession that we can specify a customInputView with text suggestions.

We should refactor this slightly to decouple datalists from _WKFormInputSession.

[Wenson Hsieh]

Renaming to track the above issue (with a test fix for fast/forms/datalist/datalist-textinput-suggestions-order.html en route to fixing the bug).

[Wenson Hsieh]

Created attachment 352762 [details]
Patch

[Tim Horton]

Comment on attachment 352762 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:253
> +    RetainPtr<UIView<WKFormControl>> _dataListTextSuggestionsInputView;
> +    RetainPtr<NSArray<UITextSuggestion *>> _dataListTextSuggestions;

Sooooooo I don't know if WebKit style has a preference, but I usually go with "Class <ConformsToProtocol>" but "Class<GenericType>". AppKit public headers seem to agree.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:398
> +    if (suggestions == _suggestions || [suggestions isEqualToArray:_suggestions.get()])

This reads humorously but I think it's right since you want nil to compare equal.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4805
> +    // Text suggestions vended from internal clients take precedence over text suggestions from a focused form control with a datalist.

Why "internal" clients? What does that even mean? Aren't clients external by definition?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4808
> +    if ([_formInputSession suggestions].count) {
> +        [inputDelegate setSuggestions:[_formInputSession suggestions]];

If I'm reading this right, `suggestions` calls straight to the client, so you should definitely only call it once and store the result here; no need to be so chatty with the client!

> Tools/WebKitTestRunner/UIScriptControllerCocoa.mm:48
> +    [TestController::singleton().mainWebView()->platformView() resignFirstResponder];

Do we ... fix this ... when the next test starts?

[Wenson Hsieh]

Comment on attachment 352762 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:253
>> +    RetainPtr<NSArray<UITextSuggestion *>> _dataListTextSuggestions;
> 
> Sooooooo I don't know if WebKit style has a preference, but I usually go with "Class <ConformsToProtocol>" but "Class<GenericType>". AppKit public headers seem to agree.

Ah, ok. (If I understand correctly, you mean this should be `RetainPtr<UIView <WKFormControl>>` and `RetainPtr<NSArray<UITextSuggestion *>>`?)

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:398
>> +    if (suggestions == _suggestions || [suggestions isEqualToArray:_suggestions.get()])
> 
> This reads humorously but I think it's right since you want nil to compare equal.

Yep, that was the intention!

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4805
>> +    // Text suggestions vended from internal clients take precedence over text suggestions from a focused form control with a datalist.
> 
> Why "internal" clients? What does that even mean? Aren't clients external by definition?

Oops, I really meant clients of our SPI (here at Apple). Changed "internal clients" to just "clients".

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4808
>> +        [inputDelegate setSuggestions:[_formInputSession suggestions]];
> 
> If I'm reading this right, `suggestions` calls straight to the client, so you should definitely only call it once and store the result here; no need to be so chatty with the client!

Hm...do you mean the call to `[_formInputSession suggestions]`? The concrete class (WKFormInputSession) implements -suggestions as a simple RetainPtr::get(), so I don't think we'll be double-calling our client here.

But it's probably still a good idea to stash the result in a temporary though, so I changed this to only call -suggestions once.

>> Tools/WebKitTestRunner/UIScriptControllerCocoa.mm:48
>> +    [TestController::singleton().mainWebView()->platformView() resignFirstResponder];
> 
> Do we ... fix this ... when the next test starts?

🤔 good point. We should probably ensure that we're first responder when we reset the web view to a consistent state in WKTR. My understanding is that layout tests shouldn't actually care whether the web view is FR prior to running the test. The relatively few tests that do care (e.g. tests that insert text in a text field) would all do something to ensure that the web view is FR, if necessary (e.g. by tapping something on the web view).

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #6)
> (…)
> >> Tools/WebKitTestRunner/UIScriptControllerCocoa.mm:48
> >> +    [TestController::singleton().mainWebView()->platformView() resignFirstResponder];
> > 
> > Do we ... fix this ... when the next test starts?
> 
> 🤔 good point. We should probably ensure that we're first responder when we
> reset the web view to a consistent state in WKTR (…)

So we do (in fact) try to become first responder on macOS when resetting to a consistent state, inside of TestController::resetStateToConsistentValues (see the call to `m_mainWebView->focus()`). However, this turns around and calls `PlatformWebView::makeWebViewFirstResponder()` which, on iOS, is just a stub.

I think we just need to implement this stub (and fix one other edge case on iOS where we might continue onto the next test without being FR).

[Wenson Hsieh]

Created attachment 352802 [details]
EWS run

[WebKit Commit Bot]

Comment on attachment 352802 [details]
EWS run

Clearing flags on attachment: 352802

Committed r237305: <https://trac.webkit.org/changeset/237305>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.