Bug 214946 - [macOS] Show picker for date and datetime-local input types
Summary: [macOS] Show picker for date and datetime-local input types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Safari Technology Preview
Hardware: Macintosh Other
: P2 Normal
Assignee: Aditya Keerthi
URL:
Keywords: InRadar
Depends on:
Blocks: 119175
  Show dependency treegraph
 
Reported: 2020-07-29 15:41 PDT by Aditya Keerthi
Modified: 2020-09-17 09:45 PDT (History)
17 users (show)

See Also:


Attachments
Patch (82.64 KB, patch)
2020-07-29 19:16 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (82.94 KB, patch)
2020-07-29 19:31 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Screenshot of picker (57.39 KB, image/png)
2020-07-30 10:41 PDT, Aditya Keerthi
no flags Details
Patch (84.94 KB, patch)
2020-08-05 10:44 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (84.94 KB, patch)
2020-08-05 16:55 PDT, Aditya Keerthi
wenson_hsieh: review+
Details | Formatted Diff | Diff
Patch for landing (85.02 KB, patch)
2020-08-06 17:15 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aditya Keerthi 2020-07-29 15:41:27 PDT
Both input types should display a calendar when the control is selected.
Comment 1 Jay 2020-07-29 15:43:44 PDT
I would love for this to be resolved.
Comment 2 Aditya Keerthi 2020-07-29 19:16:57 PDT
Created attachment 405540 [details]
Patch
Comment 3 Aditya Keerthi 2020-07-29 19:31:08 PDT
Created attachment 405541 [details]
Patch
Comment 4 m.kurz+webkitbugs 2020-07-30 05:49:59 PDT
Finally! Please make it happen!
Comment 5 Sam Weinig 2020-07-30 10:04:35 PDT
Aditya, can you add a screen shot of what this looks like to aid review?
Comment 6 Aditya Keerthi 2020-07-30 10:41:25 PDT
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 7 Darin Adler 2020-08-04 13:45:35 PDT
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.
Comment 8 Aditya Keerthi 2020-08-05 10:44:54 PDT
Created attachment 406011 [details]
Patch
Comment 9 Aditya Keerthi 2020-08-05 10:47:53 PDT
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.
Comment 10 Tim Horton 2020-08-05 11:53:41 PDT
(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?
Comment 11 Tim Horton 2020-08-05 11:54:19 PDT
Anyway, WeakObjCPtr<> is your friend.
Comment 12 Darin Adler 2020-08-05 12:16:19 PDT
(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.
Comment 13 Radar WebKit Bug Importer 2020-08-05 15:42:19 PDT
<rdar://problem/66597339>
Comment 14 Aditya Keerthi 2020-08-05 16:55:23 PDT
Created attachment 406057 [details]
Patch
Comment 15 Wenson Hsieh 2020-08-06 13:35:39 PDT
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 16 Darin Adler 2020-08-06 14:06:35 PDT
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 17 Sam Weinig 2020-08-06 15:34:26 PDT
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?
Comment 18 Aditya Keerthi 2020-08-06 17:15:45 PDT
Created attachment 406137 [details]
Patch for landing
Comment 19 Aditya Keerthi 2020-08-06 17:24:19 PDT
(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.
Comment 20 Sam Weinig 2020-08-06 18:12:39 PDT
(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.
Comment 21 Beth Dakin 2020-08-06 19:43:49 PDT
(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.
Comment 22 EWS 2020-08-24 09:11:07 PDT
Patch 406137 does not build
Comment 23 EWS 2020-08-24 09:25:15 PDT
Committed r266063: <https://trac.webkit.org/changeset/266063>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406137 [details].
Comment 24 Martin Häcker 2020-09-17 03:45:20 PDT
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?
Comment 25 Aditya Keerthi 2020-09-17 09:45:02 PDT
(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.