This bug tracks the iOS user-interface work required to show suggestions for TextFieldInputTypes with a list attribute.
Created attachment 347585 [details] Patch
Comment on attachment 347585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347585&action=review > Source/WebCore/css/html.css:634 > + z-index: 2147483645; What's this! Maybe talk to Simon.
Comment on attachment 347585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347585&action=review > Source/WebKit/UIProcess/WebDataListSuggestionsDropdownIOS.mm:369 > + [cell.textLabel setLineBreakMode:NSLineBreakByTruncatingTail]; Any reason you're not sticking with dot syntax past the first property? > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:390 > + && ![suggestions.firstObject isKindOfClass:NSClassFromString(@"WKDataListTextSuggestion")]) { isKindOfClass for our own class like this is not a great smell > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1005 > [_inputPeripheral endEditing]; > + if ([[_formInputSession customInputView] respondsToSelector:@selector(endEditing)]) > + [[_formInputSession customInputView] performSelector:@selector(endEditing)]; > + Wonder if we should put these somewhere together. There are already two callers, which might be enough.
Attachment 347585 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3115: The parameter name "]" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 347585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347585&action=review > Source/WebCore/html/TextFieldInputType.cpp:880 > +#if !PLATFORM(IOS) > if (!UserGestureIndicator::processingUserGesture()) > return; > +#endif Why is this OK? It seems really odd for a user gesture check to not apply to iOS. Possible that instead you want to fake a user gesture in some particular case where you know it’s OK? > Source/WebKit/UIProcess/WebDataListSuggestionsDropdownIOS.mm:45 > +@interface WKFormInputSession () > +- (void)setCustomInputView:(UIView *)customInputView; > +- (void)setSuggestions:(NSArray<UITextSuggestion *> *)suggestions; > +@end Why not... somewhere else? All of these things are internal, aren’t they? > Source/WebKit/UIProcess/WebDataListSuggestionsDropdownIOS.mm:49 > +@interface WKContentView () > +- (WKFormInputSession *)_formInputSession; > +@end Why not in WKContentView(Interaction).h?
Created attachment 347869 [details] Patch
Comment on attachment 347869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347869&action=review > Source/WebKit/ChangeLog:8 > + On iOS, we have less space display suggestions from a datalist element compared to Nit - less space to display > Source/WebKit/UIProcess/WebDataListSuggestionsDropdownIOS.mm:94 > +WebDataListSuggestionsDropdownIOS::~WebDataListSuggestionsDropdownIOS() { } Nit - do you need this destructor, or can you omit it/use = default?
Created attachment 348054 [details] Patch
Created attachment 348056 [details] Patch
Comment on attachment 348056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348056&action=review > Source/WebCore/html/TextFieldInputType.cpp:879 > + userActivated |= (type == DataListSuggestionActivationType::TextChanged); Not sure I'd name this "userActivated", since we poke a hole in that constraint on iOS if type == DataListSuggestionActivationType::TextChanged. Maybe "shouldDisplaySuggestions"? It also seems weird that we relax this constraint only for iOS. Is there a reason we need to gate this on user gesture more strictly for macOS, or can we remove this platform check? > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:396 > + if (_contentView.get() Nit - you don't need this null check because [_contentView assistedNodeInformation].hasSuggestions will evaluate to NO anyways if ! _contentView. > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3233 > + if ([textSuggestion isKindOfClass:[WKDataListTextSuggestion class]]) { We chatted over IRC about several different ways to accomplish this. I think this is okay, but another way (that avoids a class check and possibly the need for a WKDataListTextSuggestion subclass altogether) is check if this text suggestion is one of the suggestions in -[WKDataListSuggestionsControl textSuggestions].
Created attachment 348221 [details] Patch
(In reply to Wenson Hsieh from comment #10) > Comment on attachment 348056 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348056&action=review > > > Source/WebCore/html/TextFieldInputType.cpp:879 > > + userActivated |= (type == DataListSuggestionActivationType::TextChanged); > > Not sure I'd name this "userActivated", since we poke a hole in that > constraint on iOS if type == DataListSuggestionActivationType::TextChanged. > Maybe "shouldDisplaySuggestions"? > > It also seems weird that we relax this constraint only for iOS. Is there a > reason we need to gate this on user gesture more strictly for macOS, or can > we remove this platform check? > UserGestureIndicator::processingUserGesture() has a different behavior on iOS and macOS when typing. By the time this method is called, processingUserGesture returns false on iOS. However, I can remove the platform check, since TextChanged is only used through TextFieldInputType::didSetValueByUserEdit. This means that the "user activated" behavior of displaying the suggestions is preserved.
Comment on attachment 348221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348221&action=review > Source/WebCore/html/TextFieldInputType.cpp:267 > + m_dataListDropdownIndicator->setInlineStyleProperty(CSSPropertyDisplay, suggestions().size() ? CSSValueBlock : CSSValueNone, true); Why all this inline style instead of selectors in the UA stylesheet?
Comment on attachment 348221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348221&action=review >> Source/WebCore/html/TextFieldInputType.cpp:267 >> + m_dataListDropdownIndicator->setInlineStyleProperty(CSSPropertyDisplay, suggestions().size() ? CSSValueBlock : CSSValueNone, true); > > Why all this inline style instead of selectors in the UA stylesheet? I thought the same thing, but didn't see a way to accomplish this without also showing the dropdown indicator when suggestions() is empty.
Comment on attachment 348221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348221&action=review > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:384 > + [_customInputView performSelector:@selector(endEditing)]; I think we generally prefer to cast to the relevant ObjC protocol or type and call the selector (-endEditing) directly, instead of using -performSelector:. > Source/WebKit/UIProcess/ios/WebDataListSuggestionsDropdownIOS.mm:108 > + [m_suggestionsControl showSuggestionsDropdown:this activationType:information.activationType]; Hm...but we already WTFMove'd our information above! Perhaps you meant to save the value of information.activationType in a temporary before creating m_suggestionsControl?
Comment on attachment 348221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348221&action=review Other than the `information.activationType` above, this looks okay to me. > Source/WebKit/UIProcess/ios/WebDataListSuggestionsDropdownIOS.mm:122 > +void WebDataListSuggestionsDropdownIOS::didSelectOption(String& selectedOption) Nit - this should be a const String&
Created attachment 348577 [details] Patch
Comment on attachment 348577 [details] Patch Clearing flags on attachment: 348577 Committed r235556: <https://trac.webkit.org/changeset/235556>
All reviewed patches have been landed. Closing bug.
<rdar://problem/43943921>