Bug 216985

Summary: <input type="datetime-local"> not show calendar UI when it's inside ShadowDOM
Product: WebKit Reporter: sobue
Component: FormsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: akeerthi, ben, cafebab3, cdumez, changseok, darin, dino, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, hi, hta, japhet, jer.noble, joepeck, kangil.han, kondapallykalyan, pdr, philipj, pxlcoder, rniwa, sabouhallawa, sam, schenney, sergio, tommyw, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: Safari 14   
Hardware: iPhone / iPad   
OS: Other   
See Also: https://bugs.webkit.org/show_bug.cgi?id=222657
Bug Depends on:    
Bug Blocks: 148695    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Tests should fail
ews-feeder: commit-queue-
Patch
none
Patch none

Description sobue 2020-09-25 12:21:23 PDT
When `<input type="datetime-local">` is put inside ShadowDOM, iOS date time picker UI won't show up.
This bug is a regression from iOS 14. iOS 13.7 was showing date time picker.

```
<!DOCTYPE html>
<html>
  <head>
    <title>Safari datetime-local in ShadowDOM</title>
    <script>
      const template = document.createElement('template');
      template.innerHTML = `<input type="datetime-local">`;

      window.customElements.define('test-me', class TestMeElement extends HTMLElement {
        constructor() {
          super();
          const root = this.attachShadow({ mode: 'open'});
          root.appendChild(template.content.cloneNode(true));
        }
      });
    </script>
  </head>
  <body>
    Not in ShadowDOM : <input type="datetime-local"><br>
    Inside ShadowDOM : <test-me></test-me><br>
  </body>
</html>

```

Chrome v85 is showing calendar UI for both cases, either when `<input type="datetime-local">` is inside ShadowDOM or outside.
Mobile Safari in iOS13.7 shows calendar UI for both cases as well.

Mobile Safari in iOS 14.0 shows calendar UI when <input> is outside ShadowDOM but not when it's inside ShadowDOM.
Comment 1 Radar WebKit Bug Importer 2020-09-27 10:15:21 PDT
<rdar://problem/69660273>
Comment 2 Benjamin Nagel 2020-09-28 00:47:27 PDT
The same problem occures when using `<input type="date">` or `<input type="time">`. Other inputs are working fine. The same issue is when using `template.contens.cloneNode(true)`

Also when reading the type if the `<input type="datetime-local">` (or other date types) the result is always `text`. The input is styled like a date field but behaves like a text input.


JSFiddle - cloneNode: https://jsfiddle.net/g5xp3u76/1/
JSFiddle - webComponent: https://jsfiddle.net/znkpLuce/
Comment 3 Benjamin Nagel 2020-09-28 00:54:58 PDT
Also in the SO post I posted a few days ago a user experienced a crash when using the clear button of a date field. I didn't tested it myself.

Source: https://stackoverflow.com/questions/64042354/safari-on-ios-14-breaks-date-time-inputs-when-used-with-template-tags-or-custom
Comment 4 Aditya Keerthi 2020-10-05 08:33:51 PDT
There are a couple ways to reproduce this issue.

```
let body = document.getElementsByTagName("body")[0];
let template = document.createElement('template');
template.innerHTML = '<input type="date">';
body.appendChild(template.content.cloneNode(true));
```

and

```
let body = document.getElementsByTagName("body")[0];
let doc = document.implementation.createHTMLDocument("New Document");
doc.body = document.createElement("body");
doc.body.innerHTML = '<input type="date">';
body.appendChild(doc.body.childNodes[0]);
```

The common link between these two test cases is the creation of a new Document, without an associated Frame. This results in the Document’s Settings containing default values for all features.

```
m_settings(frame ? Ref<Settings>(frame->settings()) : Settings::create(nullptr))
```

Since the initial value of date input feature flags in Settings.yaml is false, creating an <input type=“date”> element in this kind of Document results in the creation of a TextInputType rather than a DateInputType.

```
Ref<InputType> InputType::create(HTMLInputElement& element, const AtomString& typeName)
{
    if (!typeName.isEmpty()) {
        static const auto factoryMap = makeNeverDestroyed(createInputTypeFactoryMap());
        auto&& [conditional, factory] = factoryMap.get().get(typeName);
        if (factory && (!conditional || std::invoke(conditional, element.document().settings())))
            return factory(element);
    }
    return adoptRef(*new TextInputType(element));
}
```

This regression was caused by r263977, which moved date input feature flags from RuntimeEnabledSettings into page Settings.
Comment 5 Sam Weinig 2020-10-05 09:31:43 PDT
We should fix the issue with bad Settings in Document by requiring all Documents to either be passed a Frame& or a Settings&. And then we should get rid of that Settings constructor.
Comment 6 Sam Weinig 2020-10-06 11:42:22 PDT Comment hidden (obsolete)
Comment 7 Sam Weinig 2020-10-06 11:49:54 PDT Comment hidden (obsolete)
Comment 8 Sam Weinig 2020-10-06 15:18:39 PDT Comment hidden (obsolete)
Comment 9 Sam Weinig 2020-10-06 17:02:33 PDT
Created attachment 410715 [details]
Patch
Comment 10 Sam Weinig 2020-10-06 17:36:44 PDT
Should be ready for review now.
Comment 11 Darin Adler 2020-10-06 18:39:17 PDT
Comment on attachment 410715 [details]
Patch

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

Seems the right way to do this. But we should not explicitly pass the Settings in any case where it can be avoided. I suggest removing it anywhere we can rather than passing it everywhere. I suspect that functions that do not allow a null frame don’t need the Settings argument although I might have missed something.

> Source/WebCore/Scripts/SettingsTemplates/Settings.h.erb:40
> +    WEBCORE_EXPORT static Ref<Settings> create(Page*);

Can this really work with a Page of null? Of not maybe change to a Page&?

> Source/WebCore/dom/DOMImplementation.h:46
> +    static Ref<Document> createDocument(const String& mimeType, Frame*, const Settings&, const URL&);

Another option is to use the name contentType.

> Source/WebCore/dom/Document.cpp:617
> +    auto document = adoptRef(*new Document(nullptr, contextDocument.m_settings.get(), URL()));

Doesn’t this work without the explicit call to get()?

> Source/WebCore/dom/Document.cpp:4010
> +            return XMLDocument::createXHTML(nullptr, m_settings.get(), url());

Same here.

> Source/WebCore/dom/Document.cpp:4011
> +        return XMLDocument::create(nullptr, m_settings.get(), url());

And here.

> Source/WebCore/dom/Document.cpp:4013
> +    return create(m_settings.get(), url());

Here.

> Source/WebCore/dom/Document.cpp:7142
> +        m_templateDocument = HTMLDocument::create(nullptr, m_settings.get(), aboutBlankURL());

Here.

> Source/WebCore/dom/Document.cpp:7144
> +        m_templateDocument = create(m_settings.get(), aboutBlankURL());

Here.

> Source/WebCore/dom/Document.h:343
> +    static Ref<Document> createNonRenderedPlaceholder(Frame&, const Settings&, const URL&);

Seems we do not need the Settings argument here.

> Source/WebCore/html/HTMLDocument.h:33
> +    static Ref<HTMLDocument> createSynthesizedDocument(Frame&, const Settings&, const URL&);

Seems we do not need the Settings argument here.

> Source/WebCore/html/ImageDocument.h:37
> +    static Ref<ImageDocument> create(Frame& frame, const Settings& settings, const URL& url)

Might not need Settings argument here.

> Source/WebCore/html/PluginDocument.h:37
> +    static Ref<PluginDocument> create(Frame& frame, const Settings& settings, const URL& url)

Might not need it here.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:114
> +static CachedResource* createResource(CachedResource::Type type, CachedResourceRequest&& request, const PAL::SessionID& sessionID, const CookieJar* cookieJar, const Settings& settings)

Change return type to smart pointer?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:153
> +static CachedResource* createResource(CachedResourceRequest&& request, CachedResource& resource)

Ditto?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:155
> +    switch (resource.type()) {

Is there a way to make this less repetitive?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:1081
> +    CachedResourceHandle<CachedResource> resource = createResource(type, WTFMove(request), sessionID, &cookieJar, settings);

Maybe auto if we fix the return type above

> Source/WebCore/loader/cache/CachedSVGDocument.cpp:39
> +    : CachedSVGDocument(WTFMove(request), resource.sessionID(), resource.cookieJar(), resource.m_settings.get())

Less get()?

> Source/WebCore/loader/cache/CachedSVGDocument.cpp:59
> +        m_document = SVGDocument::create(nullptr, m_settings.get(), response().url());

Likely don’t need the get() here

> Source/WebCore/page/ios/FrameIOS.mm:86
> +    auto document = HTMLDocument::createSynthesizedDocument(*this, settings(), url);

Maybe this is a case where we do not have to pass Settings.

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:63
> +    auto document = Document::createNonRenderedPlaceholder(mainFrame, mainFrame.settings(), data.scriptURL);

Maybe this function is a case where we do not need to pass Settings.

> Source/WebCore/xml/DOMParser.cpp:45
> +    auto document = DOMImplementation::createDocument(contentType, nullptr, m_settings.get(), URL { });

May not need get here.
Comment 12 Sam Weinig 2020-10-06 20:22:34 PDT
(In reply to Darin Adler from comment #11)
> Comment on attachment 410715 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410715&action=review
> 
> Seems the right way to do this. But we should not explicitly pass the
> Settings in any case where it can be avoided. I suggest removing it anywhere
> we can rather than passing it everywhere. I suspect that functions that do
> not allow a null frame don’t need the Settings argument although I might
> have missed something.
> 
> > Source/WebCore/Scripts/SettingsTemplates/Settings.h.erb:40
> > +    WEBCORE_EXPORT static Ref<Settings> create(Page*);
> 
> Can this really work with a Page of null? Of not maybe change to a Page&?

It can (really, Settings doesn't for the most part require a Page at all), and I'd like to make it take a Page& at some point, but that would require making it easier to make a Page so that the TestWebKitAPI test for DocumentOrder can still work.

> 
> > Source/WebCore/dom/DOMImplementation.h:46
> > +    static Ref<Document> createDocument(const String& mimeType, Frame*, const Settings&, const URL&);
> 
> Another option is to use the name contentType.

Done.

> 
> > Source/WebCore/dom/Document.cpp:617
> > +    auto document = adoptRef(*new Document(nullptr, contextDocument.m_settings.get(), URL()));
> 
> Doesn’t this work without the explicit call to get()?
> 
> > Source/WebCore/dom/Document.cpp:4010
> > +            return XMLDocument::createXHTML(nullptr, m_settings.get(), url());
> 
> Same here.
> 
> > Source/WebCore/dom/Document.cpp:4011
> > +        return XMLDocument::create(nullptr, m_settings.get(), url());
> 
> And here.
> 
> > Source/WebCore/dom/Document.cpp:4013
> > +    return create(m_settings.get(), url());
> 
> Here.
> 
> > Source/WebCore/dom/Document.cpp:7142
> > +        m_templateDocument = HTMLDocument::create(nullptr, m_settings.get(), aboutBlankURL());
> 
> Here.
> 
> > Source/WebCore/dom/Document.cpp:7144
> > +        m_templateDocument = create(m_settings.get(), aboutBlankURL());
> 
> Here.
> 

Done (and all above).

> > Source/WebCore/dom/Document.h:343
> > +    static Ref<Document> createNonRenderedPlaceholder(Frame&, const Settings&, const URL&);
> 
> Seems we do not need the Settings argument here.

Removed.

> 
> > Source/WebCore/html/HTMLDocument.h:33
> > +    static Ref<HTMLDocument> createSynthesizedDocument(Frame&, const Settings&, const URL&);
> 
> Seems we do not need the Settings argument here.

Removed.

> 
> > Source/WebCore/html/ImageDocument.h:37
> > +    static Ref<ImageDocument> create(Frame& frame, const Settings& settings, const URL& url)
> 
> Might not need Settings argument here.

Removed.

> 
> > Source/WebCore/html/PluginDocument.h:37
> > +    static Ref<PluginDocument> create(Frame& frame, const Settings& settings, const URL& url)
> 
> Might not need it here.


Removed.

> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:114
> > +static CachedResource* createResource(CachedResource::Type type, CachedResourceRequest&& request, const PAL::SessionID& sessionID, const CookieJar* cookieJar, const Settings& settings)
> 
> Change return type to smart pointer?

Done.

> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:153
> > +static CachedResource* createResource(CachedResourceRequest&& request, CachedResource& resource)
> 
> Ditto?

Done.

> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:155
> > +    switch (resource.type()) {
> 
> Is there a way to make this less repetitive?

I couldn't come up with one yet. Will keep thinking though.

> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:1081
> > +    CachedResourceHandle<CachedResource> resource = createResource(type, WTFMove(request), sessionID, &cookieJar, settings);
> 
> Maybe auto if we fix the return type above

Done.

> 
> > Source/WebCore/loader/cache/CachedSVGDocument.cpp:39
> > +    : CachedSVGDocument(WTFMove(request), resource.sessionID(), resource.cookieJar(), resource.m_settings.get())
> 
> Less get()?

Removed.

> 
> > Source/WebCore/loader/cache/CachedSVGDocument.cpp:59
> > +        m_document = SVGDocument::create(nullptr, m_settings.get(), response().url());
> 
> Likely don’t need the get() here

Removed.

> 
> > Source/WebCore/page/ios/FrameIOS.mm:86
> > +    auto document = HTMLDocument::createSynthesizedDocument(*this, settings(), url);
> 
> Maybe this is a case where we do not have to pass Settings.

Removed.

> 
> > Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:63
> > +    auto document = Document::createNonRenderedPlaceholder(mainFrame, mainFrame.settings(), data.scriptURL);
> 
> Maybe this function is a case where we do not need to pass Settings.

Removed.

> 
> > Source/WebCore/xml/DOMParser.cpp:45
> > +    auto document = DOMImplementation::createDocument(contentType, nullptr, m_settings.get(), URL { });
> 
> May not need get here.

Done.
Comment 13 Sam Weinig 2020-10-06 20:24:04 PDT
Created attachment 410727 [details]
Patch
Comment 14 EWS 2020-10-06 21:03:54 PDT
Committed r268114: <https://trac.webkit.org/changeset/268114>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410727 [details].
Comment 15 Aditya Keerthi 2020-10-29 17:27:13 PDT
*** Bug 218339 has been marked as a duplicate of this bug. ***