We currently have a few of the date/time input types disabled completely on macOS. While we can't enable them completely (they still lack any UI as far as I know), it would be useful to at least have them compiled in and tested via run-webkit-tests for the parts that do work (e.g. the DOM aspects), but runtime disabled everywhere else. Curious if anyone has objections to this idea (the actual amount of extra code compiled is quite small).
Adding folks to get
Adding folks to get feedback on the idea.
Seems like an obviously good idea. I seem to recall that we made the decision to compile them out long ago when we couldn’t thoroughly and completely disable such feature at runtime. Whether the features should show up in the Experimental Features menu seems a separate question. Might be unhelpful if they did at this stage?
(In reply to Darin Adler from comment #3) > Seems like an obviously good idea. > > I seem to recall that we made the decision to compile them out long ago when > we couldn’t thoroughly and completely disable such feature at runtime. Hm, I am not sure if that is still a problem. It seems like the runtime checks when creating the InputType subclass should be enough. But I will keep this in mind. > > Whether the features should show up in the Experimental Features menu seems > a separate question. Might be unhelpful if they did at this stage? Yeah, for now, I was going to keep them off the Experimental Features menu.
(In reply to Sam Weinig from comment #4) > (In reply to Darin Adler from comment #3) > > Seems like an obviously good idea. > > > > I seem to recall that we made the decision to compile them out long ago when > > we couldn’t thoroughly and completely disable such feature at runtime. > > Hm, I am not sure if that is still a problem. It seems like the runtime > checks when creating the InputType subclass should be enough. But I will > keep this in mind. > Hm, I wonder if the issue was with default style sheet. Indeed, enabling the macro, but disabling the runtime feature for, for instance, ENABLE_INPUT_TYPE_DATE, will still have this added to html.css: input[type="date"] { align-items: center; -webkit-appearance: menulist-button; display: -webkit-inline-flex; overflow: hidden; #if !(defined(WTF_PLATFORM_IOS_FAMILY) && WTF_PLATFORM_IOS_FAMILY) width: 10em; #endif } which is likely observable. I am not sure how big a deal this would be in practice.
(In reply to Sam Weinig from comment #5) > (In reply to Sam Weinig from comment #4) > > (In reply to Darin Adler from comment #3) > > > Seems like an obviously good idea. > > > > > > I seem to recall that we made the decision to compile them out long ago when > > > we couldn’t thoroughly and completely disable such feature at runtime. > > > > Hm, I am not sure if that is still a problem. It seems like the runtime > > checks when creating the InputType subclass should be enough. But I will > > keep this in mind. > > > > Hm, I wonder if the issue was with default style sheet. Indeed, enabling the > macro, but disabling the runtime feature for, for instance, > ENABLE_INPUT_TYPE_DATE, will still have this added to html.css: > > input[type="date"] { > align-items: center; > -webkit-appearance: menulist-button; > display: -webkit-inline-flex; > overflow: hidden; > #if !(defined(WTF_PLATFORM_IOS_FAMILY) && WTF_PLATFORM_IOS_FAMILY) > width: 10em; > #endif > } > > which is likely observable. I am not sure how big a deal this would be in > practice. If it is an issue, seems like a good time to make see what it would take to allow runtime settings to control what is in the user agent style sheet.
(In reply to Sam Weinig from comment #6) > (In reply to Sam Weinig from comment #5) > > (In reply to Sam Weinig from comment #4) > > > (In reply to Darin Adler from comment #3) > > > > Seems like an obviously good idea. > > > > > > > > I seem to recall that we made the decision to compile them out long ago when > > > > we couldn’t thoroughly and completely disable such feature at runtime. > > > > > > Hm, I am not sure if that is still a problem. It seems like the runtime > > > checks when creating the InputType subclass should be enough. But I will > > > keep this in mind. > > > > > > > Hm, I wonder if the issue was with default style sheet. Indeed, enabling the > > macro, but disabling the runtime feature for, for instance, > > ENABLE_INPUT_TYPE_DATE, will still have this added to html.css: > > > > input[type="date"] { > > align-items: center; > > -webkit-appearance: menulist-button; > > display: -webkit-inline-flex; > > overflow: hidden; > > #if !(defined(WTF_PLATFORM_IOS_FAMILY) && WTF_PLATFORM_IOS_FAMILY) > > width: 10em; > > #endif > > } > > > > which is likely observable. I am not sure how big a deal this would be in > > practice. > > If it is an issue, seems like a good time to make see what it would take to > allow runtime settings to control what is in the user agent style sheet. Now I am just talking to myself, but it seems like UserAgentStyle::ensureDefaultStyleSheetsForElement() allows for this already.
Created attachment 403533 [details] WIP
Created attachment 403534 [details] Patch
Created attachment 403536 [details] Patch
Created attachment 403537 [details] Patch
Created attachment 403539 [details] Patch
Created attachment 403540 [details] Patch
Going to wait for https://bugs.webkit.org/show_bug.cgi?id=213964 to land, as that will conflict, and allow this to be a bit smaller (won't have to change all those FeatureDefines.xcconfigs).
Created attachment 403541 [details] Patch
You will want to wait until the patch from bug 212542 lands; after that one, you can definitely flip these flags without touching FeatureDefines.xcconfig. Before that one, PlatformEnableCocoa.h and FeatureDefines.xcconfig both define the same thing, and it would likely be confusing to only edit PlatformEnableCocoa.h, even though it would likely work as long as it’s enabling something *more*.
Created attachment 403544 [details] Patch
Comment on attachment 403544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403544&action=review > Source/WebKit/Shared/WebPreferencesDefaultValues.h:241 > +#if !PLATFORM(COCOA) || PLATFORM(IOS) || PLATFORM(WATCHOS) I would write this the opposite way. #if !PLATFORM(MAC) && !PLATFORM(MACCATALYST) && !PLATFORM(APPLETV) That should be the entire list!
Comment on attachment 403544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403544&action=review > Source/WebCore/ChangeLog:29 > + Switch from using RuntimeEnabledFeatures to Settings for checking for enabling (Settings allows Why we do we even have RuntimeEnableFeatures? In what cases does it outshine Settings? > Source/WebCore/ChangeLog:30 > + us to write less boilerplait code and is more versitile for testing) and switch to checking the misspellings: boilerplate and versatile
(In reply to Darin Adler from comment #19) > Comment on attachment 403544 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403544&action=review > > > Source/WebCore/ChangeLog:29 > > + Switch from using RuntimeEnabledFeatures to Settings for checking for enabling (Settings allows > > Why we do we even have RuntimeEnableFeatures? In what cases does it outshine > Settings? The only thing RuntimeEnabledFeatures offers that Settings doesn't, is that they are global, whereas as Settings requires access to a Document or Page. Like most other mutable globals in WebKit, I don't think we should have RuntimeEnabledFeatures (and argued at length when they were added). Any runtime mutation of them is likely wrong with respect to at least Legacy WebKit, as you might have multiple WebViews in a process with different needs. > > > Source/WebCore/ChangeLog:30 > > + us to write less boilerplait code and is more versitile for testing) and switch to checking the > > misspellings: boilerplate and versatile I really need to get a text editor with spellcheck one of these days (or learn to spell, though I don't see that happening anytime soon). Maybe I should make the style checker spell check sentences in ChangeLog for me.
Created attachment 403569 [details] Patch
Created attachment 403570 [details] Patch
Created attachment 403572 [details] Patch
Comment on attachment 403572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403572&action=review > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:-544 > - _type = WKInputTypeDateTime; Is there deprecation or removal needed for "WKInputTypeDateTime".
Committed r263977: <https://trac.webkit.org/changeset/263977> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403572 [details].
<rdar://problem/65142171>
(In reply to Darin Adler from comment #24) > Comment on attachment 403572 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403572&action=review > > > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:-544 > > - _type = WKInputTypeDateTime; > > Is there deprecation or removal needed for "WKInputTypeDateTime". There likely should be. I need find out what the current process for removing this kind of SPI is.
It looks like commit r263977 caused 36 fast form and web-platform test consistently crashing. Tracking the issue here: https://bugs.webkit.org/show_bug.cgi?id=214009
Comment on attachment 403572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403572&action=review > Source/WebCore/rendering/RenderTheme.cpp:1089 > + return ""_s; IOS_FAMILY builds are crashing here. That’s because this needs to be: return emptyString();
(In reply to Darin Adler from comment #29) > Comment on attachment 403572 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403572&action=review > > > Source/WebCore/rendering/RenderTheme.cpp:1089 > > + return ""_s; > > IOS_FAMILY builds are crashing here. That’s because this needs to be: > > return emptyString(); Thanks, landed r263993 with the fix <https://trac.webkit.org/changeset/263993>.