Bug 186151 – [Datalist] Allow TextFieldInputType to show and hide suggestions

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

[Aditya Keerthi]

This bug tracks the work required to allow TextFieldInputType to show and hide suggestions from a datalist.

The UI work will come in a later patch.

[Aditya Keerthi]

Created attachment 341690 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/40781913>

[Wenson Hsieh]

Comment on attachment 341690 [details]
Patch

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

> Source/WebCore/html/DataListSuggestionInformation.h:31
> +enum DataListSuggestionActivationType {

Nit - we generally prefer to use `enum class` for these, instead of just plain enums.

> Source/WebCore/html/TextFieldInputType.cpp:168
> +void TextFieldInputType::handleClickEvent(__unused MouseEvent& event)

Nit - instead of __unused, we prefer omitting the argument name (in this case, just MouseEvent&).

> Source/WebCore/html/TextFieldInputType.cpp:544
> +#if ENABLE(DATALIST_ELEMENT)

Hm...I find this change a bit odd. It seems like whether we respect the list attribute should depend on whether we support rendering data list UI, rather than always returning true iff the compile-time flag is enabled.

Could we tweak the implementation of RenderTheme::supportsDataListUI() instead? I think it would make the code a bit easier to follow, though it would entail checking against a palette of input type names. But this doesn't seem so costly, especially since AtomicString comparisons are cheap.

> Source/WebCore/html/TextFieldInputType.cpp:768
> +        for (unsigned i = 0; RefPtr<HTMLOptionElement> option = downcast<HTMLOptionElement>(options->item(i)); ++i) {

Nit - We generally prefer to use `auto` for types that can be easily deduced from the RHS. For instance, this could be `auto option = downcast<HTMLOptionElement>(options->item(i))`.

> Source/WebCore/html/TextFieldInputType.h:57
> +    void handleClickEvent(MouseEvent&) override;

Nit - if subclasses don't need to override this, we generally prefer `final` instead of `override`.

> Source/WebCore/html/TextFieldInputType.h:127
> +    IntRect elementRectRelativeToRootView() const final;

Nit - It's great that you specified the coordinate space in the function name! But in most of the places where we have a function like this, it's usually (…)InRootViewCoordinates().

> Source/WebCore/platform/DataListSuggestionPicker.h:42
> +    virtual void handleKeydownWithIdentifier(const WTF::String&) { }

Nit - we probably don't need an explicit WTF:: here.

[Aditya Keerthi]

(In reply to Wenson Hsieh from comment #3)
> Comment on attachment 341690 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341690&action=review
> 
> > Source/WebCore/html/TextFieldInputType.cpp:544
> > +#if ENABLE(DATALIST_ELEMENT)
> 
> Hm...I find this change a bit odd. It seems like whether we respect the list
> attribute should depend on whether we support rendering data list UI, rather
> than always returning true iff the compile-time flag is enabled.
> 
> Could we tweak the implementation of RenderTheme::supportsDataListUI()
> instead? I think it would make the code a bit easier to follow, though it
> would entail checking against a palette of input type names. But this
> doesn't seem so costly, especially since AtomicString comparisons are cheap.

I made the change this way as support for the list attribute depends on the input type (https://www.w3.org/TR/html50/forms.html#do-not-apply).

All TextFieldInputTypes support the attribute. Consequently we can always return true for this method in TextFieldInputType. The UI for these input types is also the same.

For input types that don't support the list attribute, we can return false in shouldRespectListAttribute.

[Aditya Keerthi]

Created attachment 341977 [details]
Patch

[Wenson Hsieh]

Comment on attachment 341977 [details]
Patch

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

> Source/WebCore/html/TextFieldInputType.cpp:802
> +        for (unsigned i = 0; auto option = downcast<HTMLOptionElement>(options->item(i)); ++i) {

Ah, I think in general we prefer auto* instead of auto for pointers like this.

> Source/WebCore/html/TextFieldInputType.cpp:822
> +    m_suggestionPicker = nullptr;

This seems to be the only place we null out m_suggestionPicker, and it looks like the plan is for this to be invoked after an IPC message from the UI process? Should we be clearing this out from more places, e.g. when the element loses focus? Though, it looks like the next patch in the sequence will answer my questions :)

Just keep this in mind!

> Source/WebKit/WebProcess/WebCoreSupport/WebDataListSuggestionPicker.cpp:42
> +    , m_page(page) { }

Nit - in most constructors like this, we'd put the braces on new lines like:

{
}

> Source/WebKit/WebProcess/WebCoreSupport/WebDataListSuggestionPicker.h:47
> +    void didSelectOption(const WTF::String&);

Nit - I don't think the WTF:: in WTF::String should be needed here.

> Source/WebKit/WebProcess/WebCoreSupport/WebDataListSuggestionPicker.h:52
> +    __unused WebCore::DataListSuggestionsClient* m_dataListSuggestionsClient;

I assume these __unused are going to be short-lived? (i.e. the next patch will use m_page and m_dataListSuggestionsClient).

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3566
> +    m_activeDataListSuggestionPicker = dataListSuggestionPicker;

Also here — make sure we clean up m_activeDataListSuggestionPicker in the various ways that might cause the picker to disappear! (e.g. element is blurred programmatically).

[Aditya Keerthi]

(In reply to Wenson Hsieh from comment #6)
> Comment on attachment 341977 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341977&action=review
> 
> > Source/WebCore/html/TextFieldInputType.cpp:822
> > +    m_suggestionPicker = nullptr;
> 
> This seems to be the only place we null out m_suggestionPicker, and it looks
> like the plan is for this to be invoked after an IPC message from the UI
> process? Should we be clearing this out from more places, e.g. when the
> element loses focus? Though, it looks like the next patch in the sequence
> will answer my questions :)
> 
> Just keep this in mind!

The next patch handles all of the clean up after the picker disappears :)

> > Source/WebKit/WebProcess/WebCoreSupport/WebDataListSuggestionPicker.h:52
> > +    __unused WebCore::DataListSuggestionsClient* m_dataListSuggestionsClient;
> 
> I assume these __unused are going to be short-lived? (i.e. the next patch
> will use m_page and m_dataListSuggestionsClient).

These will be used in the next patch.

[Aditya Keerthi]

Created attachment 342186 [details]
Patch

[Tim Horton]

Comment on attachment 342186 [details]
Patch

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

> Source/WebCore/html/TextFieldInputType.cpp:568
> +#if ENABLE(DATALIST_ELEMENT)
> +    return true;
> +#else
>      return InputType::themeSupportsDataListUI(this);
> +#endif

How does this work? If the control theme doesn’t support the UI for suggestion lists, how does this patch make it work?

[Aditya Keerthi]

(In reply to Tim Horton from comment #9)
> Comment on attachment 342186 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342186&action=review
> 
> > Source/WebCore/html/TextFieldInputType.cpp:568
> > +#if ENABLE(DATALIST_ELEMENT)
> > +    return true;
> > +#else
> >      return InputType::themeSupportsDataListUI(this);
> > +#endif
> 
> How does this work? If the control theme doesn’t support the UI for
> suggestion lists, how does this patch make it work?

The UI for suggestions will be the same for the TextFieldInputTypes, and all TextFieldInputTypes support showing suggestions. The UI itself will be added in the next patch. 

Range and color inputs will require different UI, as we can't simply show a dropdown list of values. For that reason, no changes have been made to their methods at this time.

[Tim Horton]

Comment on attachment 342186 [details]
Patch

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

>>> Source/WebCore/html/TextFieldInputType.cpp:568
>>> +#endif
>> 
>> How does this work? If the control theme doesn’t support the UI for suggestion lists, how does this patch make it work?
> 
> The UI for suggestions will be the same for the TextFieldInputTypes, and all TextFieldInputTypes support showing suggestions. The UI itself will be added in the next patch. 
> 
> Range and color inputs will require different UI, as we can't simply show a dropdown list of values. For that reason, no changes have been made to their methods at this time.

But themeSupportsDataListUI returns false when handed a TextFieldInputType, despite "all TextFieldInputTypes support showing suggestions”? That seems odd. And if it’s not true, you wouldn’t need this change.

[Aditya Keerthi]

(In reply to Tim Horton from comment #11)
> Comment on attachment 342186 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342186&action=review
> 
> >>> Source/WebCore/html/TextFieldInputType.cpp:568
> >>> +#endif
> >> 
> >> How does this work? If the control theme doesn’t support the UI for suggestion lists, how does this patch make it work?
> > 
> > The UI for suggestions will be the same for the TextFieldInputTypes, and all TextFieldInputTypes support showing suggestions. The UI itself will be added in the next patch. 
> > 
> > Range and color inputs will require different UI, as we can't simply show a dropdown list of values. For that reason, no changes have been made to their methods at this time.
> 
> But themeSupportsDataListUI returns false when handed a TextFieldInputType,
> despite "all TextFieldInputTypes support showing suggestions”? That seems
> odd. And if it’s not true, you wouldn’t need this change.

I would eventually like to remove themeSupportsDataListUI. It currently returns false as we have no datalist support, but once finished, support will be determined by the specification.

Each input type can override shouldRespectListAttribute on it's own as necessary, since respecting the attribute is dependent on the type of input. The method needs to return true for us to be able to obtain the datalist element associated with the input.

A list of input types that support the list attribute can be found here: https://www.w3.org/TR/html50/forms.html#the-input-element. Once full support has been added, all of those types should have shouldRespectListAttribute return true.

[EWS Watchlist]

Comment on attachment 342186 [details]
Patch

Rejecting attachment 342186 [details] from commit-queue.

[e-mail] does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 342186 [details]
Patch

Clearing flags on attachment: 342186

Committed r232640: <https://trac.webkit.org/changeset/232640>