Summary: | [iOS] Support the inputmode attribute on contenteditable elements | ||
---|---|---|---|
Product: | WebKit | Reporter: | Aditya Keerthi <pxlcoder> |
Component: | Forms | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | cdumez, commit-queue, dbates, ews-watchlist, rniwa, ryanhaddad, webkit-bug-importer, wenson_hsieh |
Priority: | P2 | Keywords: | InRadar |
Version: | WebKit Nightly Build | ||
Hardware: | iPhone / iPad | ||
OS: | Unspecified | ||
Attachments: |
Description
Aditya Keerthi
2018-08-22 21:25:43 PDT
Created attachment 347898 [details]
Patch
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 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 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 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 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 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 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 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 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 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>) (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&. Created attachment 347943 [details]
Patch
(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 on attachment 347943 [details] Patch Clearing flags on attachment: 347943 Committed r235245: <https://trac.webkit.org/changeset/235245> All reviewed patches have been landed. Closing bug. 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 Committed r235263 as a fix. |