Bug 42958 – Pasting should fire textInput event

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

Currently pasting a text into textarea/input/contentEditable doesn't fire textInput event.
It should fire one.

[Hajime Morrita]

Created attachment 63440 [details]
Patch

[Tony Chang]

Comment on attachment 63440 [details]
Patch

> diff --git a/LayoutTests/editing/pasteboard/script-tests/paste-text-events.js b/LayoutTests/editing/pasteboard/script-tests/paste-text-events.js
> +function pastingTextInputHandler(evt)
> +{
> +    shouldBe("event.data", toStringLiteral(expectedTextEventData));
> +    textInputCount++;
> +    if (willCancelTextInput)
> +	evt.preventDefault();

Nit: Looks like a tab character got in here.

> +willCancelTextInput = false;
> +for (var i = 0; i < proceedingTestCases.length; ++i)
> +    runSingleTest(proceedingTestCases[i]);
> +shouldBe("textInputCount", "proceedingTestCases.length");
> +
> +textInputCount = 0;
> +willCancelTextInput = true;
> +for (var i = 0; i < cancelingTestCases.length; ++i)
> +    runSingleTest(cancelingTestCases[i]);
> +shouldBe("textInputCount", "cancelingTestCases.length");

These tests look great!  Does this event fire for ontextinput declared in the HTML?  If so, it'd be nice to add some tests for that too.

> diff --git a/WebCore/editing/Editor.cpp b/WebCore/editing/Editor.cpp
> +void Editor::pasteAsPlainText(const String& pastingText, bool smartReplace)
> +void Editor::pasteAsFragment(PassRefPtr<DocumentFragment> pastingFragment, bool smartReplace, bool matchStyle)

These methods seem to have only one caller.  Since they're pretty short, maybe we should just inline them?  I think it's confusing to have pasteAsPlainText and pasteAsPlainTextWithPasteboard.


> +class FragmentPasteTextEvent : public TextEvent {
> +public:
> +    static PassRefPtr<FragmentPasteTextEvent> create(PassRefPtr<AbstractView> view, PassRefPtr<DocumentFragment> pastingData, bool smartReplace, bool matchStyle);

Subclassing seems like overkill just to keep track of the type of event.  Can we just add an argument to the TextEvent constructor that keeps track of the event type?  The spec mentions an inputMode parameter to initTextEvent, but that should probably happen in a different patch.

[Hajime Morrita]

Created attachment 63551 [details]
Patch

[Hajime Morrita]

Hi Tony, thank you fro reviewing.
I updated the patch. Could you take a look at new one?

(In reply to comment #2)
> (From update of attachment 63440 [details])
> > diff --git a/LayoutTests/editing/pasteboard/script-tests/paste-text-events.js b/LayoutTests/editing/pasteboard/script-tests/paste-text-events.js
> Nit: Looks like a tab character got in here.

Oops. removed. (I should change a setting for my javascript-mode.)

> 
> > +willCancelTextInput = false;
> > +for (var i = 0; i < proceedingTestCases.length; ++i)
> > +    runSingleTest(proceedingTestCases[i]);
> > +shouldBe("textInputCount", "proceedingTestCases.length");
> > +
> > +textInputCount = 0;
> > +willCancelTextInput = true;
> > +for (var i = 0; i < cancelingTestCases.length; ++i)
> > +    runSingleTest(cancelingTestCases[i]);
> > +shouldBe("textInputCount", "cancelingTestCases.length");
> 
> These tests look great!  Does this event fire for ontextinput declared in the HTML?  If so, it'd be nice to add some tests for that too.

WebKit doesn't implement ontextinput yet. 
Though I filed it to Bug 43538,
I noticed that there seems no standard that mentions about ontextinput.
I'm not sure if we should implement this. Any ideas?

> 
> > diff --git a/WebCore/editing/Editor.cpp b/WebCore/editing/Editor.cpp
> > +void Editor::pasteAsPlainText(const String& pastingText, bool smartReplace)
> > +void Editor::pasteAsFragment(PassRefPtr<DocumentFragment> pastingFragment, bool smartReplace, bool matchStyle)
> 
> These methods seem to have only one caller.  Since they're pretty short, maybe we should just inline them?  I think it's confusing to have pasteAsPlainText and pasteAsPlainTextWithPasteboard.

They are also called from WebHTMLView.mm.
(The problem is that there are 2 very similar pasting code, 
one for WebCore and another for WebKit/mac.)
By the way, pasteXxWithoutSendingEvent() looks verbose and I removed them.

> 
> 
> > +class FragmentPasteTextEvent : public TextEvent {
> > +public:
> > +    static PassRefPtr<FragmentPasteTextEvent> create(PassRefPtr<AbstractView> view, PassRefPtr<DocumentFragment> pastingData, bool smartReplace, bool matchStyle);
> 
> Subclassing seems like overkill just to keep track of the type of event.  Can we just add an argument to the TextEvent constructor that keeps track of the event type?  The spec mentions an inputMode parameter to initTextEvent, but that should probably happen in a different patch.

Agreed and removed subclasses.

[Tony Chang]

Comment on attachment 63551 [details]
Patch

This looks great!  Just some small changes left.

> diff --git a/WebCore/dom/TextEvent.h b/WebCore/dom/TextEvent.h
> +        static PassRefPtr<TextEvent> createForPlainTextPaste(PassRefPtr<AbstractView> view, const String& data, bool shouldSmartReplace)
> +        {
> +            RefPtr<TextEvent> event = adoptRef(new TextEvent(view, data));

Can you move this body into the .cpp file?

> +            event->setIsPaste(true);
> +            event->setShouldSmartReplace(shouldSmartReplace);

Can the member variables change after the TextEvent is created?  If not, maybe we should use constructor arguments and not have set methods.

> +        static PassRefPtr<TextEvent> createForFragmentPaste(PassRefPtr<AbstractView> view, PassRefPtr<DocumentFragment> data, bool shouldSmartReplace, bool shouldMatchStyle)
> +        {
> +            RefPtr<TextEvent> event = adoptRef(new TextEvent(view, ""));

Same as above.

> diff --git a/WebCore/editing/Editor.cpp b/WebCore/editing/Editor.cpp
> +bool Editor::handleTextEvent(TextEvent* event)
> +{
> +    if (event->isPaste()) {

This refactoring looks much nicer!


> diff --git a/LayoutTests/editing/pasteboard/paste-text-events.html b/LayoutTests/editing/pasteboard/paste-text-events.html

One other testing suggestion: Should we add a test to make sure that canceling keydown prevents textInput from firing?  This is mentioned here: http://www.w3.org/TR/DOM-Level-3-Events/#keyset-cancelable_keys

It would be fine to add that in a follow up change too.

[Hajime Morrita]

Created attachment 63674 [details]
Patch

[Hajime Morrita]

Thanks again, Tony! The patch was updated.

> > diff --git a/WebCore/dom/TextEvent.h b/WebCore/dom/TextEvent.h
> > +        static PassRefPtr<TextEvent> createForPlainTextPaste(PassRefPtr<AbstractView> view, const String& data, bool shouldSmartReplace)
> > +        {
> > +            RefPtr<TextEvent> event = adoptRef(new TextEvent(view, data));
> 
> Can you move this body into the .cpp file?
Doe.

> 
> > +            event->setIsPaste(true);
> > +            event->setShouldSmartReplace(shouldSmartReplace);
> 
> Can the member variables change after the TextEvent is created?  If not, maybe we should use constructor arguments and not have set methods.
Sure. moved to the constructor and removed setters.

> One other testing suggestion: Should we add a test to make sure that canceling keydown prevents textInput from firing?  This is mentioned here: http://www.w3.org/TR/DOM-Level-3-Events/#keyset-cancelable_keys
OK, filed Bug 43601 for that. A patch will come shortly.

[Tony Chang]

Comment on attachment 63674 [details]
Patch

In general, using enums instead of bools is preferred for readability.  But I realize that some of this is historic for smart replace and match style.  A good follow up patch would be to convert these bools to enums across all the editing code.

WebCore/dom/TextEvent.cpp:56
 +      , m_isBackTab(false)
Nit: Please initialize the bools here too.

[Hajime Morrita]

Committed r64963: <http://trac.webkit.org/changeset/64963>