Bug 42958 - Pasting should fire textInput event
Summary: Pasting should fire textInput event
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 42957
  Show dependency treegraph
 
Reported: 2010-07-25 23:55 PDT by Hajime Morrita
Modified: 2010-08-08 22:27 PDT (History)
3 users (show)

See Also:


Attachments
Patch (35.71 KB, patch)
2010-08-04 04:36 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (23.17 KB, patch)
2010-08-04 22:16 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (24.33 KB, patch)
2010-08-05 18:23 PDT, Hajime Morrita
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2010-07-25 23:55:59 PDT
Currently pasting a text into textarea/input/contentEditable doesn't fire textInput event.
It should fire one.
Comment 1 Hajime Morrita 2010-08-04 04:36:44 PDT
Created attachment 63440 [details]
Patch
Comment 2 Tony Chang 2010-08-04 14:08:19 PDT
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.
Comment 3 Hajime Morrita 2010-08-04 22:16:09 PDT
Created attachment 63551 [details]
Patch
Comment 4 Hajime Morrita 2010-08-04 22:25:55 PDT
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.
Comment 5 Tony Chang 2010-08-05 09:39:08 PDT
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.
Comment 6 Hajime Morrita 2010-08-05 18:23:39 PDT
Created attachment 63674 [details]
Patch
Comment 7 Hajime Morrita 2010-08-05 18:31:24 PDT
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.
Comment 8 Tony Chang 2010-08-06 09:24:56 PDT
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.
Comment 9 Hajime Morrita 2010-08-08 22:27:05 PDT
Committed r64963: <http://trac.webkit.org/changeset/64963>