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: | 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
sobue
2020-09-25 12:21:23 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/ 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 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. 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. Created attachment 410672 [details]
Patch
Created attachment 410673 [details]
Patch
Created attachment 410701 [details]
Tests should fail
Created attachment 410715 [details]
Patch
Should be ready for review now. 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. (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. Created attachment 410727 [details]
Patch
Committed r268114: <https://trac.webkit.org/changeset/268114> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410727 [details]. *** Bug 218339 has been marked as a duplicate of this bug. *** |