RESOLVED FIXED 224924
[GTK] Add picker UI for <input type=date> and <input type=datetime-local> 
https://bugs.webkit.org/show_bug.cgi?id=224924
Summary [GTK] Add picker UI for <input type=date> and <input type=datetime-local>
Martin Robinson
Reported 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.
Attachments
Patch (3.75 KB, patch)
2021-05-06 05:19 PDT, Martin Robinson 		no flags
DetailsFormatted DiffDiff
Patch (10.81 KB, patch)
2021-05-07 02:27 PDT, Carlos Garcia Campos 		no flags
DetailsFormatted DiffDiff
Patch (10.99 KB, patch)
2021-05-08 03:48 PDT, Carlos Garcia Campos 		no flags
DetailsFormatted DiffDiff
Patch (10.99 KB, patch)
2021-05-08 04:01 PDT, Carlos Garcia Campos 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Martin Robinson
Comment 1 2021-05-06 05:19:15 PDT
Created attachment 427875 [details] Patch
EWS Watchlist
Comment 2 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
Carlos Garcia Campos
Comment 3 2021-05-06 06:31:10 PDT
Comment on attachment 427875 [details] Patch This is wip, let's remove the r? flag.
Carlos Garcia Campos
Comment 4 2021-05-07 02:27:29 PDT
Created attachment 427980 [details] Patch
Martin Robinson
Comment 5 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?
Carlos Garcia Campos
Comment 6 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.
Carlos Garcia Campos
Comment 7 2021-05-08 03:48:30 PDT
Created attachment 428084 [details] Patch
Carlos Garcia Campos
Comment 8 2021-05-08 04:01:16 PDT
Created attachment 428085 [details] Patch
Adrian Perez
Comment 9 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.
Carlos Garcia Campos
Comment 10 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.
Adrian Perez
Comment 11 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.
EWS
Comment 12 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].
Note You need to log in before you can comment on or make changes to this bug.