Bug 183621 – Add support for the `inputmode` attribute

• Component: Forms
• Status: RESOLVED FIXED

[Dayton Lowell]

https://html.spec.whatwg.org/multipage/interaction.html#input-modalities%3A-the-inputmode-attribute

This will ship in Blink/Chrome 66: https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/inputmode/blink-dev/MAHQkH4vvUQ/i0yCobrMCQAJ

[Aditya Keerthi]

Created attachment 347340 [details]
Patch

[Aditya Keerthi]

Created attachment 347396 [details]
Patch

[Aditya Keerthi]

Created attachment 347413 [details]
Patch

[Daniel Bates]

Comment on attachment 347413 [details]
Patch

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

> Source/WebCore/html/InputModeNames.cpp:77
> +} // namespace WebCore::InputModeNames

// namespace InputModeNames

> Source/WebCore/html/InputModeNames.h:42
> +}

Ditto.

> Source/WebKit/Shared/AssistedNodeInformation.h:61
> +enum class InputMode {

We should specify a size fo this enum to get better packing. Otherwise, this enum will default to the width of int.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3554
> +        default:

I suggest we remove this "default" case and list any other enumerators. This will allow the compiler to ensure that this switch handles all enumerators.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2302
> +static InputMode inputModeForElement(const Node& node)

Can we come up with a better name for this?

[Wenson Hsieh]

> duplicate symbol __ZN7WebCore14InputModeNames7numericEv in:
>     /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release-iphonesimulator/WebCore.build/Objects-normal/x86_64/InputModeNames.o
>     /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release-iphonesimulator/WebCore.build/Objects-normal/x86_64/UnifiedSource269.o
> ld: 7 duplicate symbols for architecture x86_64
> clang: error: linker command failed with exit code 1 (use -v to see invocation)

Is InputModeNames.cpp being included in the WebCore target, as well appearing in one of the unified source files?

[Aditya Keerthi]

Created attachment 347504 [details]
Patch

[Aditya Keerthi]

Created attachment 347720 [details]
Patch

[Aditya Keerthi]

Added an API test to verify we show the correct keyboard for a variety of input configurations.

[Tim Horton]

Comment on attachment 347720 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:192
> +    NSArray *inputModes = @[ @"", @"text", @"tel", @"url", @"email", @"numeric", @"decimal", @"search" ];

Could create this from the dictionary with -allKeys. Except "" I guess... I dunno.

> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:203
> +    NSArray *inputTypesWithKeyboard = @[ @"text", @"password", @"search", @"email", @"tel", @"number", @"url" ];

Could create this from the dictionary with -allKeys

[Aditya Keerthi]

Created attachment 347812 [details]
Patch

[Aditya Keerthi]

Created attachment 347820 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 347820 [details]
Patch for landing

Clearing flags on attachment: 347820

Committed r235201: <https://trac.webkit.org/changeset/235201>

[Daniel Bates]

Comment on attachment 347820 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:8
> +        Added the inputmode attribute to the HTMLInputElement and HTMLTextAreaElement IDL

It is an unwritten convention that we reference the spec. and version/date of the spec. in the ChangeLog when adding new web exposed functionality. One example of such a ChangeLog (though it does not include a date) is <https://trac.webkit.org/changeset/234680/>. This records the spec you referenced when writing this patch.

> Source/WebCore/html/HTMLTextAreaElement.idl:62
> +    attribute DOMString inputMode;

As per <https://html.spec.whatwg.org/multipage/interaction.html#dom-inputmode> this should be a reflected attribute. Please add "[Reflect]" before "attribute".

> Source/WebCore/html/HTMLTextFormControlElement.cpp:678
> +String HTMLTextFormControlElement::inputMode() const

How did you come to the decision to return a String instead of returning an const AtomicString&? I mean, returning a String causes unnecessary ref-count churn.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:702
> +void HTMLTextFormControlElement::setInputMode(const String& value)
> +{
> +    setAttributeWithoutSynchronization(inputmodeAttr, value);
> +}

This function is unnecessary when the attribute is marked "[Reflect]" in the IDL.

[Radar WebKit Bug Importer]

<rdar://problem/43623657>

[Daniel Bates]

(In reply to Daniel Bates from comment #13)
> > Source/WebCore/html/HTMLTextAreaElement.idl:62
> > +    attribute DOMString inputMode;
> 
> As per
> <https://html.spec.whatwg.org/multipage/interaction.html#dom-inputmode> this
> should be a reflected attribute. Please add "[Reflect]" before "attribute".
> 

Disregard this remark. Our binding generator code does not support reflections with a custom getter.

> [...]
> 
> > Source/WebCore/html/HTMLTextFormControlElement.cpp:702
> > +void HTMLTextFormControlElement::setInputMode(const String& value)
> > +{
> > +    setAttributeWithoutSynchronization(inputmodeAttr, value);
> > +}
> 
> This function is unnecessary when the attribute is marked "[Reflect]" in the
> IDL.


Disregard this remark.

[David Tapuska]

So looking at the patch set it looks like you might have only implemented half of it. There doesn't seem to be any "none" support and this is missing the attribute on the HTML element which is useful for content editable. 

Is this really resolved fixed? Or are more patches coming? If not can you share justification for diverging from the specification?

[Aditya Keerthi]

(In reply to David Tapuska from comment #16)
> So looking at the patch set it looks like you might have only implemented
> half of it. There doesn't seem to be any "none" support and this is missing
> the attribute on the HTML element which is useful for content editable. 
> 
> Is this really resolved fixed? Or are more patches coming? If not can you
> share justification for diverging from the specification?

I have a patch forthcoming for the contenteditable case.

[David Tapuska]

None support is used by some of Google's productivity apps that have a content editable item but don't wish a keyboard to be present. For example presentation mode in Google Slides.

[Darryl Pogue]

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

[Emanuele Feliziani]

Hello

This has been shipped in Chrome and I see it has been marked as fixed here for months. I am writing this hoping it gets noticed by someone who can verify it so it can make its way into Safari soon enough.

Many thanks everyone.

[Dayton Lowell]

Emanuele,

This will ship in iOS 12.2, which is currently in beta.

[Emanuele Feliziani]

Best news of the week. Thanks!