Bug 163025

Summary: Support InputEvent.inputType for the new InputEvent spec
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: UI EventsAssignee: 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 Flags
Patch
none
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews116 for mac-yosemite
none
Fix DRT failures.
none
Rebased on ToT
none
Fix Windows build
none
Refactored layout tests and added a new one.
darin: review+
Patch for landing none

Description Wenson Hsieh 2016-10-06 09:59:44 PDT
Support InputEvent.inputType for the new InputEvent spec. See https://www.w3.org/TR/input-events/ for more details.
Comment 1 Wenson Hsieh 2016-10-06 13:51:14 PDT
<rdar://problem/28658092>
Comment 2 Wenson Hsieh 2016-10-06 20:16:23 PDT
Created attachment 290883 [details]
Patch
Comment 3 Build Bot 2016-10-06 21:23:33 PDT
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
Comment 4 Build Bot 2016-10-06 21:23:36 PDT
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 5 Build Bot 2016-10-06 21:30:34 PDT
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
Comment 6 Build Bot 2016-10-06 21:30:36 PDT
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
Comment 7 Wenson Hsieh 2016-10-06 23:48:45 PDT
Created attachment 290905 [details]
Fix DRT failures.
Comment 8 Wenson Hsieh 2016-10-07 20:28:10 PDT
Created attachment 291006 [details]
Rebased on ToT
Comment 9 Wenson Hsieh 2016-10-07 20:40:47 PDT
Created attachment 291007 [details]
Fix Windows build
Comment 10 Ryosuke Niwa 2016-10-07 23:58:56 PDT
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 11 Wenson Hsieh 2016-10-08 11:15:20 PDT
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.
Comment 12 Wenson Hsieh 2016-10-08 11:22:23 PDT
Created attachment 291016 [details]
Refactored layout tests and added a new one.
Comment 13 Wenson Hsieh 2016-10-08 11:42:08 PDT
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.
Comment 14 Wenson Hsieh 2016-10-09 19:39:10 PDT
Created attachment 291058 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2016-10-09 20:13:54 PDT
Comment on attachment 291058 [details]
Patch for landing

Clearing flags on attachment: 291058

Committed r206979: <http://trac.webkit.org/changeset/206979>