Summary: | Support InputEvent.inputType for the new InputEvent spec | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||||||
Component: | UI Events | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, gyuyoung.kim, rniwa, webkit-bug-importer | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | 163021 | ||||||||||||||||||||
Bug Blocks: | 163112 | ||||||||||||||||||||
Attachments: |
|
Description
Wenson Hsieh
2016-10-06 09:59:44 PDT
Created attachment 290883 [details]
Patch
Comment on attachment 290883 [details] Patch Attachment 290883 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2235177 New failing tests: fast/events/before-input-prevent-paste.html fast/events/before-input-prevent-cut.html Created attachment 290888 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 290883 [details] Patch Attachment 290883 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2235180 New failing tests: fast/events/before-input-prevent-paste.html fast/events/before-input-prevent-cut.html Created attachment 290889 [details]
Archive of layout-test-results from ews116 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 290905 [details]
Fix DRT failures.
Created attachment 291006 [details]
Rebased on ToT
Created attachment 291007 [details]
Fix Windows build
Comment on attachment 291007 [details] Fix Windows build View in context: https://bugs.webkit.org/attachment.cgi?id=291007&action=review r- because while the implementation is pretty good, there are significant issues with the way tests are written. > Source/WebCore/editing/EditCommand.cpp:41 > +AtomicString inputTypeNameForEditingAction(EditAction action) We shouldn't really be using AtomicString for these strings. Use regular String instead. > Source/WebCore/editing/EditCommand.cpp:46 > + return "formatJustifyLeft"; Please wrap these strings in ASCIILiteral. > Source/WebCore/testing/Internals.cpp:3376 > +void Internals::executeEditingCommand(const String& commandName) const We already have this facility as testRunner.execCommand: https://trac.webkit.org/browser/trunk/Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl#L87 Please use that instead. > LayoutTests/fast/events/before-input-prevent-biu-expected.txt:2 > + You should add an instruction on how to manually test this in a browser by using description() function. > LayoutTests/fast/events/before-input-prevent-biu-expected.txt:8 > +PASS firstChild.tagName is undefined. > +PASS firstChild.textContent is 'abc' > +PASS secondChild.tagName is 'I' > +PASS secondChild.textContent is 'def' > +PASS secondChild.children[0].tagName is 'U' These outputs don't tell us why these outputs are correct. > LayoutTests/fast/events/before-input-prevent-biu.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> This is an old obsolete doctype. Please use modern doctype: <!DOCTYPE html> instead. > LayoutTests/fast/events/before-input-prevent-biu.html:36 > + shouldBeUndefined("firstChild.tagName"); > + shouldBe("firstChild.textContent", "'abc'"); > + shouldBe("secondChild.tagName", "'I'"); > + shouldBe("secondChild.textContent", "'def'"); > + shouldBe("secondChild.children[0].tagName", "'U'"); This is not a great way of testing the output of editing operations, which can easily change over time as we update editing code. Use LayoutTests/resources/dump-as-markup.js and Markup.dump('foo') after each call to keyDown & execCommand. (you don't need js-test-pre.js then). > LayoutTests/fast/events/before-input-prevent-biu.html:47 > +<body onload=beginTest()> There's no need to wait for unload event. You can just execute the script right away once div is parsed. e.g. place the script element above right between div & script element below. > LayoutTests/fast/events/before-input-prevent-cut-expected.txt:6 > +Initial value: "helloworld" > +After the prevented cut: "helloworld" > +After the actual cut: "" Again, this output isn't clear as to what this test is testing and why these results are expected. Using Markup.dump would make the semantics clear because it'll show selection, DOM, etc... > LayoutTests/fast/events/before-input-prevent-cut.html:34 > + internals.executeEditingCommand("Cut"); For cut, copy, & paste, you can just invoke the regular execCommand. We enable all these commands inside DRT & WTR. Comment on attachment 291007 [details] Fix Windows build View in context: https://bugs.webkit.org/attachment.cgi?id=291007&action=review >> Source/WebCore/editing/EditCommand.cpp:41 >> +AtomicString inputTypeNameForEditingAction(EditAction action) > > We shouldn't really be using AtomicString for these strings. Use regular String instead. Ah, right -- changed to use String. >> Source/WebCore/editing/EditCommand.cpp:46 >> + return "formatJustifyLeft"; > > Please wrap these strings in ASCIILiteral. Done. >> Source/WebCore/testing/Internals.cpp:3376 >> +void Internals::executeEditingCommand(const String& commandName) const > > We already have this facility as testRunner.execCommand: > https://trac.webkit.org/browser/trunk/Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl#L87 > > Please use that instead. Oh, awesome! Changed to use execCommand. >> LayoutTests/fast/events/before-input-prevent-biu-expected.txt:2 >> + > > You should add an instruction on how to manually test this in a browser by using description() function. Sounds good -- done. >> LayoutTests/fast/events/before-input-prevent-biu-expected.txt:8 >> +PASS secondChild.children[0].tagName is 'U' > > These outputs don't tell us why these outputs are correct. Good point. See below (changed to use Markup.dump). >> LayoutTests/fast/events/before-input-prevent-biu.html:1 >> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > > This is an old obsolete doctype. Please use modern doctype: <!DOCTYPE html> instead. Good catch. Fixed. >> LayoutTests/fast/events/before-input-prevent-biu.html:36 >> + shouldBe("secondChild.children[0].tagName", "'U'"); > > This is not a great way of testing the output of editing operations, which can easily change over time as we update editing code. > Use LayoutTests/resources/dump-as-markup.js and Markup.dump('foo') after each call to keyDown & execCommand. > (you don't need js-test-pre.js then). I changed these tests to use dump-as-markup.js. Thanks for the tip! >> LayoutTests/fast/events/before-input-prevent-biu.html:47 >> +<body onload=beginTest()> > > There's no need to wait for unload event. > You can just execute the script right away once div is parsed. > e.g. place the script element above right between div & script element below. Got it. Moved the script tags to the body after the element. >> LayoutTests/fast/events/before-input-prevent-cut-expected.txt:6 >> +After the actual cut: "" > > Again, this output isn't clear as to what this test is testing and why these results are expected. > Using Markup.dump would make the semantics clear because it'll show selection, DOM, etc... Changed to use Markup.dump and Markup.description. >> LayoutTests/fast/events/before-input-prevent-cut.html:34 >> + internals.executeEditingCommand("Cut"); > > For cut, copy, & paste, you can just invoke the regular execCommand. We enable all these commands inside DRT & WTR. Yep -- changed to use execCommand. Created attachment 291016 [details]
Refactored layout tests and added a new one.
Comment on attachment 291016 [details] Refactored layout tests and added a new one. View in context: https://bugs.webkit.org/attachment.cgi?id=291016&action=review > Source/WebCore/editing/InsertListCommand.h:47 > + bool preservesTypingStyle() const final { return true; } Actually, I should update these in a separate patch. Created attachment 291058 [details]
Patch for landing
Comment on attachment 291058 [details] Patch for landing Clearing flags on attachment: 291058 Committed r206979: <http://trac.webkit.org/changeset/206979> |