Bug 224924 - [GTK] Add picker UI for <input type=date> and <input type=datetime-local>
Summary: [GTK] Add picker UI for <input type=date> and <input type=datetime-local>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords:
Depends on:
Blocks: 98936
  Show dependency treegraph
 
Reported: 2021-04-22 02:32 PDT by Martin Robinson
Modified: 2021-05-10 05:17 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.75 KB, patch)
2021-05-06 05:19 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (10.81 KB, patch)
2021-05-07 02:27 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (10.99 KB, patch)
2021-05-08 03:48 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (10.99 KB, patch)
2021-05-08 04:01 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2021-04-22 02:32:13 PDT
This bug tracks adding the dialog-driven picker UI for these input types. Without them the entries can only be modified via the keyboard.
Comment 1 Martin Robinson 2021-05-06 05:19:15 PDT
Created attachment 427875 [details]
Patch
Comment 2 EWS Watchlist 2021-05-06 05:20:51 PDT
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
Comment 3 Carlos Garcia Campos 2021-05-06 06:31:10 PDT
Comment on attachment 427875 [details]
Patch

This is wip, let's remove the r? flag.
Comment 4 Carlos Garcia Campos 2021-05-07 02:27:29 PDT
Created attachment 427980 [details]
Patch
Comment 5 Martin Robinson 2021-05-07 06:30:54 PDT
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?
Comment 6 Carlos Garcia Campos 2021-05-07 07:56:10 PDT
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.
Comment 7 Carlos Garcia Campos 2021-05-08 03:48:30 PDT
Created attachment 428084 [details]
Patch
Comment 8 Carlos Garcia Campos 2021-05-08 04:01:16 PDT
Created attachment 428085 [details]
Patch
Comment 9 Adrian Perez 2021-05-10 03:12:41 PDT
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.
Comment 10 Carlos Garcia Campos 2021-05-10 03:21:29 PDT
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.
Comment 11 Adrian Perez 2021-05-10 04:58:46 PDT
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.
Comment 12 EWS 2021-05-10 05:17:55 PDT
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].