Bug 214946

I would love for this to be resolved.

Created attachment 405540 [details] Patch

Created attachment 405541 [details] Patch

Finally! Please make it happen!

Aditya, can you add a screen shot of what this looks like to aid review?

Created attachment 405589 [details] Screenshot of picker Note that the work to change the appearance of the control itself will be done in upcoming patches.

Comment on attachment 405541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405541&action=review > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:63 > + if (Chrome* chrome = this->chrome()) { I suggest auto. > Source/WebCore/loader/EmptyClients.cpp:498 > + return nullptr; Why is this OK? > Source/WebCore/page/Chrome.cpp:455 > +#if PLATFORM(IOS_FAMILY) > + UNUSED_PARAM(client); > + return nullptr; Why is this correct? > Source/WebCore/platform/DateTimeChooserParameters.h:31 > +#include "IntRect.h" > +#include <wtf/text/WTFString.h> I think we also need to include Vector.h. > Source/WebKit/UIProcess/WebPageProxy.h:64 > +#include "WebDateTimePicker.h" Would be nice to avoid having to add this include, and explore if we can do this with only forward declaration in the header. > Source/WebKit/UIProcess/WebPageProxy.h:1649 > + void didChooseDate(const String&); Same thought about StringView. > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.h:34 > +OBJC_CLASS WKDateTimePicker; Since this is an ObjC only header, only included in Objective-C++ files, we can and should write this: @class WKDateTimePicker; > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.h:43 > + void didChooseDate(const String&); Suggest StringView. > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.h:46 > + void endPicker() final; > + void showDateTimePicker(WebCore::DateTimeChooserParameters&&) final; Consider making these functions private. > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.h:51 > + NSView *m_view; What guarantees this reference counted object, WebDateTimePickerMac, will not outlive this other reference counted object, m_view? > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:38 > +static NSString * const kDateFormatString = @"yyyy-MM-dd"; > +static NSString * const kDateTimeFormatString = @"yyyy-MM-dd'T'HH:mm"; > +static NSString * const kDefaultLocaleIdentifier = @"en_US_POSIX"; Should consider constexpr for these too. > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:63 > + if (m_picker) { > + [m_picker invalidate]; > + m_picker = nil; > + } Could just write: [m_picker invalidate]; No need to null out a pointer when the object is being destroyed. And no need to check for null since Objective-C does nothing when passed null. > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:99 > +// TODO: Share this implementation with WKDataListSuggestionWindow. WebKit project uses FIXME, not TODO. > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:118 > + _backdropView = [[NSVisualEffectView alloc] initWithFrame:contentRect]; I believe this is going to leak because it’s missing adoptNS. > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:178 > + RetainPtr<NSLocale> englishLocale = adoptNS([[NSLocale alloc] initWithLocaleIdentifier:kDefaultLocaleIdentifier]); I suggest using auto here > Source/WebKit/WebProcess/WebCoreSupport/WebDateTimeChooser.h:45 > + void didChooseDate(const String&); Would be nicer if this took StringView, I think.

Created attachment 406011 [details] Patch

Thank you for the review! I would appreciate another look to verify my usage of StringView. (In reply to Darin Adler from comment #7) > Comment on attachment 405541 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405541&action=review > > > Source/WebCore/loader/EmptyClients.cpp:498 > > + return nullptr; > > Why is this OK? From my understanding, EmptyChromeClient is used for sanitizing web content. There is no need to present attached views in this context. Note that similar methods in this class either return nullptr or an empty implementation. It is safe to return nullptr here since a check is made in BaseChooserOnlyDateAndTimeInputType.cpp:65. > > Source/WebCore/page/Chrome.cpp:455 > > +#if PLATFORM(IOS_FAMILY) > > + UNUSED_PARAM(client); > > + return nullptr; > > Why is this correct? Attached views for form controls on iOS are presented through WKContentViewInteraction, using a different code path. Hence, it is safe to return nullptr here. > > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.h:51 > > + NSView *m_view; > > What guarantees this reference counted object, WebDateTimePickerMac, will > not outlive this other reference counted object, m_view? m_view is a WKWebView, which maintains a RefPtr to the WebPageProxy (_page). WebDateTimePickerMac is owned by the WebPageProxy (as m_dateTimePicker). Hence, WebDateTimePickerMac cannot outlive m_view.

(In reply to Aditya Keerthi from comment #9) > > > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.h:51 > > > + NSView *m_view; > > > > What guarantees this reference counted object, WebDateTimePickerMac, will > > not outlive this other reference counted object, m_view? > > m_view is a WKWebView, which maintains a RefPtr to the WebPageProxy (_page). > WebDateTimePickerMac is owned by the WebPageProxy (as m_dateTimePicker). > Hence, WebDateTimePickerMac cannot outlive m_view. I think this logic breaks down because WebDateTimePickerMac is refcounted, and thus could have its lifetime extended arbitrarily, right?

Anyway, WeakObjCPtr<> is your friend.

(In reply to Tim Horton from comment #10) > (In reply to Aditya Keerthi from comment #9) > > > > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.h:51 > > > > + NSView *m_view; > > > > > > What guarantees this reference counted object, WebDateTimePickerMac, will > > > not outlive this other reference counted object, m_view? > > > > m_view is a WKWebView, which maintains a RefPtr to the WebPageProxy (_page). > > WebDateTimePickerMac is owned by the WebPageProxy (as m_dateTimePicker). > > Hence, WebDateTimePickerMac cannot outlive m_view. > > I think this logic breaks down because WebDateTimePickerMac is refcounted, > and thus could have its lifetime extended arbitrarily, right? Yes. (In reply to Tim Horton from comment #11) > Anyway, WeakObjCPtr<> is your friend. Seems like a good, practical, solution.

<rdar://problem/66597339>

Created attachment 406057 [details] Patch

Comment on attachment 406057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406057&action=review > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:161 > + NSRect windowRect = [[_presentingView window] convertRectToScreen:[_presentingView convertRect:params.anchorRectInRootView toView:nil]]; The `params.anchorRectInRootView` here looks like a use-after-move! I think you meant to use _params instead.

Comment on attachment 406057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406057&action=review > Source/WebCore/page/Chrome.cpp:455 > + UNUSED_PARAM(client); > + return nullptr; I think we should add a "why" comment here at some point, not necessarily in this patch. Both why this will be called for IOS_FAMILY, and why it’s OK that we return nullptr when it is. Comment can be brief, but should say why. > Source/WebCore/platform/DateTimeChooserParameters.h:41 > + // Locale name for which the chooser should be localized. This > + // might be an invalid name because it comes from HTML lang > + // attributes. Could make this more beautiful but putting the line break at the sentence break, and using only two lines. > Source/WebCore/platform/DateTimeChooserParameters.h:50 > + double minimum; > + double maximum; > + double step; > + double stepBase; We may want to move from "double" to type like "WallTime" and "Seconds" for this in the future. Would make the code a little more clear about units. > Source/WebCore/platform/DateTimeChooserParameters.h:131 > + return {{ type, anchorRectInRootView, locale, currentValue, suggestionValues, localizedSuggestionValues, suggestionLabels, minimum, maximum, step, stepBase, required, isAnchorElementRTL }}; WTFMove will make more efficient code for the strings and especially the vectors. There are six of those here. > Source/WebKit/UIProcess/WebDateTimePicker.h:31 > +#include <wtf/RefPtr.h> Don’t need this include any more. > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:38 > +constexpr NSString * const kDateFormatString = @"yyyy-MM-dd"; > +constexpr NSString * const kDateTimeFormatString = @"yyyy-MM-dd'T'HH:mm"; > +constexpr NSString * const kDefaultLocaleIdentifier = @"en_US_POSIX"; Since we used constexpr here, we don’t also need the const after the *. > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:81 > + m_picker = adoptNS([[WKDateTimePicker alloc] initWithParams:WTFMove(params) inView:m_view.getAutoreleased()]); Instead of getAutoreleased() here, which is wasteful, we should use get().get(), which is correct for an argument we are passing to a method as opposed to one we are returning as a return value. > Tools/TestRunnerShared/UIScriptContext/UIScriptController.h:219 > + virtual bool isShowingDateTimePicker() const { notImplemented(); return false; } Not sure that notImplemented() is needed/helpful in this function (or in the other functions in this file).

Comment on attachment 406057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406057&action=review > Source/WebCore/html/MonthInputType.cpp:132 > +void MonthInputType::handleDOMActivateEvent(Event&) { } You should use the normal style here: void MonthInputType::handleDOMActivateEvent(Event&) { } >> Source/WebCore/page/Chrome.cpp:455 >> + return nullptr; > > I think we should add a "why" comment here at some point, not necessarily in this patch. Both why this will be called for IOS_FAMILY, and why it’s OK that we return nullptr when it is. Comment can be brief, but should say why. Seems like this would be a decision that would be made in the WebKit layer anyway. Also, is this right for catalyst?

Created attachment 406137 [details] Patch for landing

(In reply to Sam Weinig from comment #17) > Comment on attachment 406057 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406057&action=review > > >> Source/WebCore/page/Chrome.cpp:455 > >> + return nullptr; > > > > I think we should add a "why" comment here at some point, not necessarily in this patch. Both why this will be called for IOS_FAMILY, and why it’s OK that we return nullptr when it is. Comment can be brief, but should say why. > > Seems like this would be a decision that would be made in the WebKit layer > anyway. Also, is this right for catalyst? Catalyst uses iOS form controls, so the behavior should be the same as iOS.

(In reply to Aditya Keerthi from comment #19) > (In reply to Sam Weinig from comment #17) > > Comment on attachment 406057 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=406057&action=review > > > > >> Source/WebCore/page/Chrome.cpp:455 > > >> + return nullptr; > > > > > > I think we should add a "why" comment here at some point, not necessarily in this patch. Both why this will be called for IOS_FAMILY, and why it’s OK that we return nullptr when it is. Comment can be brief, but should say why. > > > > Seems like this would be a decision that would be made in the WebKit layer > > anyway. Also, is this right for catalyst? > > Catalyst uses iOS form controls, so the behavior should be the same as iOS. That doesn't seem like the right approach to me, but that is likely something we should discuss outside the context of this change.

(In reply to Sam Weinig from comment #20) > (In reply to Aditya Keerthi from comment #19) > > (In reply to Sam Weinig from comment #17) > > > Comment on attachment 406057 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=406057&action=review > > > > > > >> Source/WebCore/page/Chrome.cpp:455 > > > >> + return nullptr; > > > > > > > > I think we should add a "why" comment here at some point, not necessarily in this patch. Both why this will be called for IOS_FAMILY, and why it’s OK that we return nullptr when it is. Comment can be brief, but should say why. > > > > > > Seems like this would be a decision that would be made in the WebKit layer > > > anyway. Also, is this right for catalyst? > > > > Catalyst uses iOS form controls, so the behavior should be the same as iOS. > > That doesn't seem like the right approach to me, but that is likely > something we should discuss outside the context of this change. Ideally Catalyst controls would be like Mac controls. But this is its big own project.

Patch 406137 does not build

Committed r266063: <https://trac.webkit.org/changeset/266063> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406137 [details].

I was expecting <input type=date> to work on the technology preview builds by now, but I can't seem to get it working. Am I missing something?

(In reply to Martin Häcker from comment #24) > I was expecting <input type=date> to work on the technology preview builds > by now, but I can't seem to get it working. Am I missing something? This revision is not in the latest technology preview build.

Testing Safari TP 114, the date picker UI still isn't shown. Why? Or is the UI available starting with macOS 11.0 Big Sur only (testing was done with macOS 10.15.7 Catalina)?

Seeing the datepicker now in TP 115. YAY! Incidentally, I've noticed that this feature seems not yet available on <https://webkit.org/status/>. I had filed a bug about this here: https://bugs.webkit.org/show_bug.cgi?id=195666 Maybe this is the place to get that noticed?

(In reply to m.kurz+webkitbugs from comment #26) > Testing Safari TP 114, the date picker UI still isn't shown. Why? Or is the > UI available starting with macOS 11.0 Big Sur only (testing was done with > macOS 10.15.7 Catalina)? I think this was because it was behind the flag. And the flag is enabled in STP 115.