Bug 188878 - [iOS] Support the inputmode attribute on contenteditable elements
Summary: [iOS] Support the inputmode attribute on contenteditable elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-22 21:25 PDT by Aditya Keerthi
Modified: 2018-08-23 17:18 PDT (History)
8 users (show)

See Also:


Attachments
Patch (36.15 KB, patch)
2018-08-22 22:14 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.43 MB, application/zip)
2018-08-22 23:26 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.93 MB, application/zip)
2018-08-22 23:36 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.01 MB, application/zip)
2018-08-22 23:59 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.30 MB, application/zip)
2018-08-23 00:09 PDT, EWS Watchlist
no flags Details
Patch (74.96 KB, patch)
2018-08-23 11:49 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aditya Keerthi 2018-08-22 21:25:43 PDT
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
Comment 1 Aditya Keerthi 2018-08-22 22:14:05 PDT
Created attachment 347898 [details]
Patch
Comment 2 EWS Watchlist 2018-08-22 23:26:34 PDT
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
Comment 3 EWS Watchlist 2018-08-22 23:26:35 PDT
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
Comment 4 EWS Watchlist 2018-08-22 23:36:50 PDT
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
Comment 5 EWS Watchlist 2018-08-22 23:36:52 PDT
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
Comment 6 EWS Watchlist 2018-08-22 23:59:29 PDT
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
Comment 7 EWS Watchlist 2018-08-22 23:59:31 PDT
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
Comment 8 EWS Watchlist 2018-08-23 00:09:55 PDT
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
Comment 9 EWS Watchlist 2018-08-23 00:09:57 PDT
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
Comment 10 Ryosuke Niwa 2018-08-23 00:14:30 PDT
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.
Comment 11 Wenson Hsieh 2018-08-23 08:11:18 PDT
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>)
Comment 12 Daniel Bates 2018-08-23 11:16:54 PDT
(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&.
Comment 13 Aditya Keerthi 2018-08-23 11:49:04 PDT
Created attachment 347943 [details]
Patch
Comment 14 Aditya Keerthi 2018-08-23 11:50:13 PDT
(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.
Comment 15 WebKit Commit Bot 2018-08-23 14:24:49 PDT
Comment on attachment 347943 [details]
Patch

Clearing flags on attachment: 347943

Committed r235245: <https://trac.webkit.org/changeset/235245>
Comment 16 WebKit Commit Bot 2018-08-23 14:24:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2018-08-23 14:25:19 PDT
<rdar://problem/43658106>
Comment 18 Ryan Haddad 2018-08-23 16:05:16 PDT
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
Comment 19 Aditya Keerthi 2018-08-23 17:18:11 PDT
Committed r235263 as a fix.