Bug 216985

Summary:

<input type="datetime-local"> not show calendar UI when it's inside ShadowDOM

Product:

WebKit

Reporter:

sobue

Component:

Forms

Assignee:

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:

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

sobue

Reported 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.

Attachments
Patch (64.37 KB, patch) 2020-10-06 11:42 PDT, Sam Weinig	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (64.77 KB, patch) 2020-10-06 11:49 PDT, Sam Weinig	no flags	Details Formatted Diff Diff
Tests should fail (6.90 KB, patch) 2020-10-06 15:18 PDT, Sam Weinig	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (73.50 KB, patch) 2020-10-06 17:02 PDT, Sam Weinig	no flags	Details Formatted Diff Diff
Patch (68.76 KB, patch) 2020-10-06 20:24 PDT, Sam Weinig	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2020-09-27 10:15:21 PDT

<rdar://problem/69660273>

Benjamin Nagel

Comment 2 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/

Benjamin Nagel

Comment 3 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

Aditya Keerthi

Comment 4 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.

Sam Weinig

Comment 5 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.

Sam Weinig

Comment 6 2020-10-06 11:42:22 PDT Comment hidden (obsolete)

Created attachment 410672 [details] Patch

Sam Weinig

Comment 7 2020-10-06 11:49:54 PDT Comment hidden (obsolete)

Created attachment 410673 [details] Patch

Sam Weinig

Comment 8 2020-10-06 15:18:39 PDT Comment hidden (obsolete)

Created attachment 410701 [details] Tests should fail

Sam Weinig

Comment 9 2020-10-06 17:02:33 PDT

Created attachment 410715 [details] Patch

Sam Weinig

Comment 10 2020-10-06 17:36:44 PDT

Should be ready for review now.

Darin Adler

Comment 11 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.

Sam Weinig

Comment 12 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.

Sam Weinig

Comment 13 2020-10-06 20:24:04 PDT

Created attachment 410727 [details] Patch

EWS

Comment 14 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].

Aditya Keerthi

Comment 15 2020-10-29 17:27:13 PDT

*** Bug 218339 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.