Bug 213949 – Compile-time enable (but leave disabled at runtime by default) date/time input types on macOS to allow testing of cross platform (e.g. DOM) aspects of the controls

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Sam Weinig]

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

[Sam Weinig]

Adding folks to get

[Sam Weinig]

Adding folks to get feedback on the idea.

[Darin Adler]

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?

[Sam Weinig]

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

[Sam Weinig]

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

[Sam Weinig]

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

[Sam Weinig]

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

[Sam Weinig]

Created attachment 403533 [details]
WIP

[Sam Weinig]

Created attachment 403534 [details]
Patch

[Sam Weinig]

Created attachment 403536 [details]
Patch

[Sam Weinig]

Created attachment 403537 [details]
Patch

[Sam Weinig]

Created attachment 403539 [details]
Patch

[Sam Weinig]

Created attachment 403540 [details]
Patch

[Sam Weinig]

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

[Sam Weinig]

Created attachment 403541 [details]
Patch

[Darin Adler]

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

[Sam Weinig]

Created attachment 403544 [details]
Patch

[Darin Adler]

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!

[Darin Adler]

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

[Sam Weinig]

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

[Sam Weinig]

Created attachment 403569 [details]
Patch

[Sam Weinig]

Created attachment 403570 [details]
Patch

[Sam Weinig]

Created attachment 403572 [details]
Patch

[Darin Adler]

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

[EWS]

Committed r263977: <https://trac.webkit.org/changeset/263977>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403572 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65142171>

[Sam Weinig]

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

[Karl Rackler]

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

[Darin Adler]

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();

[Sam Weinig]

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