Bug 188878 – [iOS] Support the inputmode attribute on contenteditable elements

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

[Aditya Keerthi]

We should support the inputmode attribute on contenteditable elements, in addition to <input> and <textarea>.

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

[Aditya Keerthi]

Created attachment 347898 [details]
Patch

[EWS Watchlist]

Comment on attachment 347898 [details]
Patch

Attachment 347898 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8954065

New failing tests:
js/dom/dom-static-property-for-in-iteration.html
imported/w3c/web-platform-tests/html/dom/reflection-misc.html

[EWS Watchlist]

Created attachment 347906 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 347898 [details]
Patch

Attachment 347898 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8954082

New failing tests:
js/dom/dom-static-property-for-in-iteration.html
imported/w3c/web-platform-tests/html/dom/reflection-misc.html

[EWS Watchlist]

Created attachment 347909 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 347898 [details]
Patch

Attachment 347898 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8954095

New failing tests:
js/dom/dom-static-property-for-in-iteration.html

[EWS Watchlist]

Created attachment 347912 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 347898 [details]
Patch

Attachment 347898 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8954197

New failing tests:
imported/w3c/web-platform-tests/html/dom/reflection-misc.html

[EWS Watchlist]

Created attachment 347914 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Ryosuke Niwa]

Comment on attachment 347898 [details]
Patch

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

> Source/WebCore/html/HTMLElement.h:113
> +    const AtomicString& inputMode() const;
> +    void setInputMode(const AtomicString& value);

I don't think it makes much sense for these functions to take & return AtomicString
since setInputMode does a case-insensitive comparison. Just use String instead.

> LayoutTests/fast/forms/inputmode-attribute.html:29
> +shouldBe('textarea.inputMode = "tel"; textarea.inputMode', '"tel"');
> +shouldBe('textarea.getAttribute("inputmode")', '"tel"');
> +shouldBe('textarea.setAttribute("inputmode", "tel"); textarea.inputMode', '"tel"');
> +shouldBe('editor.inputMode = "url"; editor.inputMode', '"url"');

We need to test every type on every element type.
r- because of this reduces the test coverage and doesn't provide adequate amount of testing for content editable element support.

[Wenson Hsieh]

Comment on attachment 347898 [details]
Patch

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

>> Source/WebCore/html/HTMLElement.h:113
>> +    void setInputMode(const AtomicString& value);
> 
> I don't think it makes much sense for these functions to take & return AtomicString
> since setInputMode does a case-insensitive comparison. Just use String instead.

(See: <https://bugs.webkit.org/show_bug.cgi?id=183621#c13>)

[Daniel Bates]

(In reply to Ryosuke Niwa from comment #10)
> Comment on attachment 347898 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347898&action=review
> 
> > Source/WebCore/html/HTMLElement.h:113
> > +    const AtomicString& inputMode() const;
> > +    void setInputMode(const AtomicString& value);
> 
> I don't think it makes much sense for these functions to take & return
> AtomicString
> since setInputMode does a case-insensitive comparison. Just use String
> instead.
> 

I take it you meant to say that inputMode() does a case-insensitive comparison. Regardless, having either one of these functions (or both) take or return a String requires conversion (though it may be implicit and even optimized away). It is most natural for both of these functions to operate on- and return a- const lvalue reference to an AtomicString. The setter, setInputMode(), turns around and calls Element::setAttributeWithoutSynchronization() passing the new value and Element::setAttributeWithoutSynchronization() expects an AtomicString; => the setter needs to pay the conversion cost to AtomicString anyway; => the setter should take a const AtomicString&. For the getter, inputMode(), it is most natural (i.e. avoids conversion) to return a const AtomicString& because all of the WebCore::InputModeNames::*() (e.g. InputModeNames::text()) return a const AtomicString&. Therefore, it is most natural for both the setter and getter to take and return a const AtomicString&.

[Aditya Keerthi]

Created attachment 347943 [details]
Patch

[Aditya Keerthi]

(In reply to Ryosuke Niwa from comment #10)
> Comment on attachment 347898 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347898&action=review
>
> > LayoutTests/fast/forms/inputmode-attribute.html:29
> > +shouldBe('textarea.inputMode = "tel"; textarea.inputMode', '"tel"');
> > +shouldBe('textarea.getAttribute("inputmode")', '"tel"');
> > +shouldBe('textarea.setAttribute("inputmode", "tel"); textarea.inputMode', '"tel"');
> > +shouldBe('editor.inputMode = "url"; editor.inputMode', '"url"');
> 
> We need to test every type on every element type.
> r- because of this reduces the test coverage and doesn't provide adequate
> amount of testing for content editable element support.

Added additional tests to increase test coverage.

[WebKit Commit Bot]

Comment on attachment 347943 [details]
Patch

Clearing flags on attachment: 347943

Committed r235245: <https://trac.webkit.org/changeset/235245>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43658106>

[Ryan Haddad]

This change broke the Windows build:

c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\html\inputmode.cpp(73): error C2220: warning treated as error - no 'object' file generated [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\html\inputmode.cpp(73): warning C4715: 'WebCore::stringForInputMode': not all control paths return a value [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/11402

[Aditya Keerthi]

Committed r235263 as a fix.