[GTK] Add picker UI for <input type=date> and <input type=datetime-local>

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

This bug tracks adding the dialog-driven picker UI for these input types. Without them the entries can only be modified via the keyboard.

[Martin Robinson]

Created attachment 427875 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 427875 [details]
Patch

This is wip, let's remove the r? flag.

[Carlos Garcia Campos]

Created attachment 427980 [details]
Patch

[Martin Robinson]

Comment on attachment 427980 [details]
Patch

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

> Source/WebKit/UIProcess/gtk/WebDateTimePickerGtk.cpp:70
> +    webkitWebViewBaseSetShouldNotifyFocusEvents(WEBKIT_WEB_VIEW_BASE(webView), false);

Is it possible that you meant to write:

webkitWebViewBaseSetShouldNotifyFocusEvents(WEBKIT_WEB_VIEW_BASE(webView), true);

here?

[Carlos Garcia Campos]

Comment on attachment 427980 [details]
Patch

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

>> Source/WebKit/UIProcess/gtk/WebDateTimePickerGtk.cpp:70
>> +    webkitWebViewBaseSetShouldNotifyFocusEvents(WEBKIT_WEB_VIEW_BASE(webView), false);
> 
> Is it possible that you meant to write:
> 
> webkitWebViewBaseSetShouldNotifyFocusEvents(WEBKIT_WEB_VIEW_BASE(webView), true);
> 
> here?

Indeed! Good catch.

[Carlos Garcia Campos]

Created attachment 428084 [details]
Patch

[Carlos Garcia Campos]

Created attachment 428085 [details]
Patch

[Adrian Perez]

Comment on attachment 428085 [details]
Patch

Patch LGTM, with a small nit to take into account before landing. Thanks!

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

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:118
> +void webkitWebViewBasePageGrabbedTouch(WebKitWebViewBase*);

Why the change to add the name of the “webkitWebViewBase“ argument
to “webkitWebViewBasePageGrabbedTouch()”? That seems unneeded, please
make sure to only add the prototype for the new function here.

[Carlos Garcia Campos]

Comment on attachment 428085 [details]
Patch

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

>> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:118
>> +void webkitWebViewBasePageGrabbedTouch(WebKitWebViewBase*);
> 
> Why the change to add the name of the “webkitWebViewBase“ argument
> to “webkitWebViewBasePageGrabbedTouch()”? That seems unneeded, please
> make sure to only add the prototype for the new function here.

It's the opposite, it's removing the name. I missed it when reviewing the patch and noticed it when adding the new function.

[Adrian Perez]

Comment on attachment 428085 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:118
>>> +void webkitWebViewBasePageGrabbedTouch(WebKitWebViewBase*);
>> 
>> Why the change to add the name of the “webkitWebViewBase“ argument
>> to “webkitWebViewBasePageGrabbedTouch()”? That seems unneeded, please
>> make sure to only add the prototype for the new function here.
> 
> It's the opposite, it's removing the name. I missed it when reviewing the patch and noticed it when adding the new function.

Ouch, you're right. I've read the diff the other way around at first 🤦‍♂️️

Let's cq+ this.

[EWS]

Committed r277262 (237529@main): <https://commits.webkit.org/237529@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428085 [details].