Bug 186714 – [Datalist][iOS] Add suggestions UI for TextFieldInputTypes

• Component: Forms
• Status: RESOLVED FIXED

[Aditya Keerthi]

This bug tracks the iOS user-interface work required to show suggestions for TextFieldInputTypes with a list attribute.

[Aditya Keerthi]

Created attachment 347585 [details]
Patch

[Tim Horton]

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.

[Tim Horton]

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.

[EWS Watchlist]

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.

[Tim Horton]

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?

[Aditya Keerthi]

Created attachment 347869 [details]
Patch

[Wenson Hsieh]

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?

[Aditya Keerthi]

Created attachment 348054 [details]
Patch

[Aditya Keerthi]

Created attachment 348056 [details]
Patch

[Wenson Hsieh]

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].

[Aditya Keerthi]

Created attachment 348221 [details]
Patch

[Aditya Keerthi]

(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.

[Tim Horton]

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?

[Wenson Hsieh]

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.

[Wenson Hsieh]

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?

[Wenson Hsieh]

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&

[Aditya Keerthi]

Created attachment 348577 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 348577 [details]
Patch

Clearing flags on attachment: 348577

Committed r235556: <https://trac.webkit.org/changeset/235556>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43943921>