Bug 26526 – Add support for input events (oninput) to contentEditable elements

• Component: HTML Editing
• Status: RESOLVED FIXED

[Erik Arvidsson]

This came up in bug 15189

It would be nice if the input event could fire for contentEditable elements as well. Basically it would provide a cleaner way than mutation events and would fire any time the content changes (any time innerHTML has changed?).

[Erik Arvidsson]

Created attachment 31517 [details]
Test case

Editing in either of the content editable elements should output its HTML below it.

[Ojan Vafai]

WhatWG discussion: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-June/020553.html

[Ojan Vafai]

Noticed that Hixie's response doesn't show up in that link due to being in a different month: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-July/021101.html

I'll add that, as I said in that thread, that this should only fire for user-gestures. Modifications done by script don't need the event. This also matches the input event for other text controls.

[Hajime Morrita]

Created attachment 55516 [details]
patch v0

[Hajime Morrita]

Hi, we've restarted the work. 
Here is some background links:

- http://lists.macosforge.org/pipermail/webkit-dev/2010-February/011579.html
  - http://lists.macosforge.org/pipermail/webkit-dev/2010-February/011581.html
- http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-March/025406.html

As little objection, we are going to implement it behind the flag to play with.
As maciej mentioned at webkit-dev list, the proposal consists of several changes:

>1) Fire "input" event for contentEditable areas as well as for text- 
>entry form controls.
>2) For every case where we'd fire "input" per #1, add a new  
>"beforeinput" event.
>3) Add a new InputEvent (or something) interface with an "action"  
>attribute to use for "input" and "beforeinput" events.

This patch approaches (1) against which the community (seems to) arose no clear objections.

[Erik Arvidsson]

Comment on attachment 55516 [details]
patch v0

> +// A trivial case: inserting text should dispatch the event
> +var target0 = document.getElementById("target0");
> +target0.addEventListener("input", function(evt) { fired(evt, "target0", "Text should be inserted here:Text"); });
> +var target0TextLength = target0.firstChild.data.length;
> +sel.setBaseAndExtent(target0.firstChild, target0TextLength, target0.firstChild, target0TextLength);
> +document.execCommand("insertText", false, "Text");
> +expectedInputEventCount++;

If script changes should not dispatch input events, then execCommand should not either. You can use eventSender to test user input.

[Hajime Morrita]

Created attachment 55645 [details]
v1; took feedback

[Hajime Morrita]

Hi Erik, thank you for the reviewing!

> If script changes should not dispatch input events, then execCommand should not either. You can use eventSender to test user input.
Ah, the code comment, not  code itself, is wrong. I'm sorry for confusion.
I updated the patch to fix it like:
> // A direct DOM manipulation shouldn't dispatch the event.

Note that existing behavior fires input events for <input> and <textarea>
even via execCommand().

[James Robinson]

Why is the event dispatching guarded by an if(renderer()) guard?

[Hajime Morrita]

Hi James, thank you for your feedback!

(In reply to comment #9)
> Why is the event dispatching guarded by an if(renderer()) guard?
"input" event from other than contentEditable (i.e. <input> and <textarea>) is 
Because other code where handle webkitEditableContentChangedEvent does such as 
TMLFormControlElement.cpp such, I followed the same manner.
I think it is safer to check this because webkitEditableContentChangedEvent is fired inside renderer, 
and is aimed to be handled by visible elements, only which can trigger an user interaction.

But this isn't a strong opinion.

[Kent Tamura]

Comment on attachment 55645 [details]
v1; took feedback

WebCore/Configurations/FeatureDefines.xcconfig:51
 +  ENABLE_EDITING_INPUT_EVENTS = ENABLE_EDITING_INPUT_EVENTS; 
Please make the same changes to all of FeatureDefines.xcconfig in other directories.


WebCore/dom/Node.cpp:3082
 +  #if ENABLE(EDITING_INPUT_EVENTS)
#if should be put before "} else if"


- We may enable ENABLE_EDITING_INPUT_EVENTS in WebKit/chromium/features.gypi
  This doesn't affect Chromium/Chrome browsers.
- New test should be skipped in other platforms

[Hajime Morrita]

Created attachment 58864 [details]
patch v2

[Hajime Morrita]

Hi Kent-san, thank you for reviewing!
I Updated the patch.

> (From update of attachment 55645 [details])
> WebCore/Configurations/FeatureDefines.xcconfig:51
>  +  ENABLE_EDITING_INPUT_EVENTS = ENABLE_EDITING_INPUT_EVENTS; 
> Please make the same changes to all of FeatureDefines.xcconfig in other directories.
Done.

> WebCore/dom/Node.cpp:3082
>  +  #if ENABLE(EDITING_INPUT_EVENTS)
> #if should be put before "} else if"
Done.

> - We may enable ENABLE_EDITING_INPUT_EVENTS in WebKit/chromium/features.gypi
>   This doesn't affect Chromium/Chrome browsers.
> - New test should be skipped in other platforms
Done.

[Kent Tamura]

Comment on attachment 58864 [details]
patch v2

The patch should have ChangeLogs for JavaScriptCore, WebKit/mac, and WebKit2.

[Hajime Morrita]

Created attachment 58971 [details]
patch v3; reaname ENABLE_EDITING_INPUT_EVENTS to ENABLE_INPUT_EVENT_EXTENSIONS

[Ojan Vafai]

Comment on attachment 58971 [details]
patch v3; reaname ENABLE_EDITING_INPUT_EVENTS to ENABLE_INPUT_EVENT_EXTENSIONS

Thanks for making this change. Sorry this took so long to get reviewed. I mostly just have nits about the test.

I don't think we need to do this behind a flag. There was pretty unanimous support for this on whatwg and webkit-dev and it's an existing event that's specced in HTML5 for text controls. We're just making it apply to contentEditable. Also, as a result, you won't need to add the test to the Skipped lists. :)

(In reply to comment #8)
> > If script changes should not dispatch input events, then execCommand should not either. You can use eventSender to test user input.
> Ah, the code comment, not  code itself, is wrong. I'm sorry for confusion.
> I updated the patch to fix it like:
> > // A direct DOM manipulation shouldn't dispatch the event.
> 
> Note that existing behavior fires input events for <input> and <textarea>
> even via execCommand().

I agree that we should match the input/textarea behavior. If we want to avoid firing "input" for execCommands, we should do so across the board. That's a separate bug.

(In reply to comment #10)
> > Why is the event dispatching guarded by an if(renderer()) guard?
> "input" event from other than contentEditable (i.e. <input> and <textarea>) is 
> Because other code where handle webkitEditableContentChangedEvent does such as 
> TMLFormControlElement.cpp such, I followed the same manner.
> I think it is safer to check this because webkitEditableContentChangedEvent is fired inside renderer, 
> and is aimed to be handled by visible elements, only which can trigger an user interaction.
> 
> But this isn't a strong opinion.

I'm not sure we need this restriction. It exists on input/textarea because this event is fired from within RenderTextControl*, but that seems like more of an implementation detail than an explicitly desired behavior. For example, we could move the dispatch of the input event from RenderTextControl* to HTMLFormControlElementWithState::defaultEventHandler and then we wouldn't need to restrict to rendered nodes. 

That said, it's not clear to me what the correct behavior here is or even if it's possible to hit this code without a renderer. If an element is display:none, the user certainly can't modify it's content and execCommands will fail. So, I don't think we need to explicitly add this guard.

---------------------------------
http://wkrietveld.appspot.com/26526/diff/2001/3004
File LayoutTests/fast/events/event-input-contentEditable.html (right):

http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode1
LayoutTests/fast/events/event-input-contentEditable.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
These tests are great. I'd like to to add a couple extra test cases:
1. A test or two that uses eventSender. That way we can confirm that input fires for keypresses in addition to execCommands.
2. Nested contentEditables. Specifically, two cases:
  a) <div contentEditable><div contentEditable>foo</div></div> <-- I think input should fire on the outer-most div.
  b) <div contentEditable><div contentEditable=false><div contentEditable>foo</div></div></div> <-- I think input should fire on the innerMost div and then bubble up.

http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode5
LayoutTests/fast/events/event-input-contentEditable.html:5: <script src="../js/resources/js-test-pre.js"></script>
This should just be a script-test. There are other script-tests in fast/events/script-tests. You just need to add one there and then run make-script-test-wrappers to generate the HTML file.

The only difference from what you have below is that you'll need to create your DOM in script instead of in lines 11-20 below. Otherwise, you can just move all the contents of the script tag below into a JS file.

http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode36
LayoutTests/fast/events/event-input-contentEditable.html:36: shouldBe("window.actualId", "'" + expectedId + "'");
Can this just be:
shouldBe("event.target.id",  "'" + expectedId + "'");

Note that event is a global object that points to the currently active event.

http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode39
LayoutTests/fast/events/event-input-contentEditable.html:39: shouldBe("window.actualText", "window.expectedText");
Similarly, can this be:
shouldBe("event.target.innerHTML",  "'" + expectedText + "'");

http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode48
LayoutTests/fast/events/event-input-contentEditable.html:48: sel.setBaseAndExtent(target0.firstChild, target0TextLength, target0.firstChild, target0TextLength);
Nit: Can you just use sel.selectAllChildren(target0) here? I realize it's not exactly the same thing, but it should test the same case.

http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode55
LayoutTests/fast/events/event-input-contentEditable.html:55: target1.firstChild.data += "Text";
Nit: Here and below, instead of doing target1.firstChild.data can you use target1.innerHTML? That makes it clear what's going on. With firstChild.data, whoever is reading this test will need to understand the initial DOM to figure out what firstChild is.

http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode81
LayoutTests/fast/events/event-input-contentEditable.html:81: // An event shouldn't be dispatched to a invisible node.
Lets call this a display:none node or an unrendered node. Invisible could be confused with visibility:hidden.

http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode86
LayoutTests/fast/events/event-input-contentEditable.html:86: document.execCommand("insertText", false, "Text");
Each test has a lot of duplicate code.  Maybe you could do something like the following?

function setupTarget(target, inputHandler) {
    target.addEventListener("input", inputHandler);
    sel.selectAllChildren(target);
}

var target5 = document.getElementById("target5");
setupTarget(target5, function(evt) { testFailed("should not be reached"); });
target5.style.display = "none";
document.execCommand("insertText", false, "Text");

http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode92
LayoutTests/fast/events/event-input-contentEditable.html:92: var target6end    = document.getElementById("target6end");
Nit: webkit style is to not align the equals.

http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode106
LayoutTests/fast/events/event-input-contentEditable.html:106: setTimeout(function() {
Can this setTimeout be removed? Then you wouldn't need to call waitUntilDone/notifyDone. In addition to making tests slower, setTimeouts often lead to flaky tests. We should avoid them unless they're really necessary.

http://wkrietveld.appspot.com/26526/diff/2001/3011
File WebCore/dom/Node.cpp (right):

http://wkrietveld.appspot.com/26526/diff/2001/3011#newcode3032
WebCore/dom/Node.cpp:3032: event->setDefaultHandled();
Why do you need to set default handled here? Other events don't. I tried patching in your change without this line and the layout test still passes.

[Hajime Morrita]

Created attachment 59080 [details]
patch v4

[Hajime Morrita]

Hi Ojan, thank you for your careful review!
I updated the patch.

> I don't think we need to do this behind a flag. There was pretty unanimous support for this on whatwg and webkit-dev and it's an existing event that's specced in HTML5 for text controls. We're just making it apply to contentEditable. Also, as a result, you won't need to add the test to the Skipped lists. :)

OK. The compilation flags are removed. 

> 
> (In reply to comment #8)
> I agree that we should match the input/textarea behavior. If we want to avoid firing "input" for execCommands, we should do so across the board. That's a separate bug.

Sure. 
It's not clear to me whether we should suppress event for execCommand().
For example, we can implement some automation(macro) tool for the browser as, say, an extension.
In such case, the tool want to emulate user action. Suppressing events would be undesirable 
for such purpose. On other hand, web applications may want to control event timings.

> I'm not sure we need this restriction. It exists on input/textarea because this event is fired from within RenderTextControl*, but that seems like more of an implementation detail than an explicitly desired behavior. For example, we could move the dispatch of the input event from RenderTextControl* to HTMLFormControlElementWithState::defaultEventHandler and then we wouldn't need to restrict to rendered nodes. 
> 
> That said, it's not clear to me what the correct behavior here is or even if it's possible to hit this code without a renderer. If an element is display:none, the user certainly can't modify it's content and execCommands will fail. So, I don't think we need to explicitly add this guard.

Agreed. At this time, I just removed the guard. 
Trivial tests cannot unveil the difference. (modifying the tree inside an event handler may unveil it.)

> 
> ---------------------------------
> http://wkrietveld.appspot.com/26526/diff/2001/3004
> File LayoutTests/fast/events/event-input-contentEditable.html (right):
> 
> http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode1
> LayoutTests/fast/events/event-input-contentEditable.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> These tests are great. I'd like to to add a couple extra test cases:
> 1. A test or two that uses eventSender. That way we can confirm that input fires for keypresses in addition to execCommands.
> 2. Nested contentEditables. Specifically, two cases:
>   a) <div contentEditable><div contentEditable>foo</div></div> <-- I think input should fire on the outer-most div.
>   b) <div contentEditable><div contentEditable=false><div contentEditable>foo</div></div></div> <-- I think input should fire on the innerMost div and then bubble up.

Added these tests. (they behave like above expectations.)

> 
> http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode5
> LayoutTests/fast/events/event-input-contentEditable.html:5: <script src="../js/resources/js-test-pre.js"></script>
> This should just be a script-test. There are other script-tests in fast/events/script-tests. You just need to add one there and then run make-script-test-wrappers to generate the HTML file.
> 

Fixed.

> http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode36
> LayoutTests/fast/events/event-input-contentEditable.html:36: shouldBe("window.actualId", "'" + expectedId + "'");
> Can this just be:
> shouldBe("event.target.id",  "'" + expectedId + "'");
Fixed.

> http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode39
> LayoutTests/fast/events/event-input-contentEditable.html:39: shouldBe("window.actualText", "window.expectedText");
> Similarly, can this be:
> shouldBe("event.target.innerHTML",  "'" + expectedText + "'");

Fixed.

> 
> http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode48
> LayoutTests/fast/events/event-input-contentEditable.html:48: sel.setBaseAndExtent(target0.firstChild, target0TextLength, target0.firstChild, target0TextLength);
> Nit: Can you just use sel.selectAllChildren(target0) here? I realize it's not exactly the same thing, but it should test the same case.
Agreed. Fixed to use selectAllChild()

> http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode55
> LayoutTests/fast/events/event-input-contentEditable.html:55: target1.firstChild.data += "Text";
> Nit: Here and below, instead of doing target1.firstChild.data can you use target1.innerHTML? That makes it clear what's going on. With firstChild.data, whoever is reading this test will need to understand the initial DOM to figure out what firstChild is.

Fixed.

> 
> http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode81
> LayoutTests/fast/events/event-input-contentEditable.html:81: // An event shouldn't be dispatched to a invisible node.
> Lets call this a display:none node or an unrendered node. Invisible could be confused with visibility:hidden.
Fixed.

> 
> http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode86
> LayoutTests/fast/events/event-input-contentEditable.html:86: document.execCommand("insertText", false, "Text");
> Each test has a lot of duplicate code.  Maybe you could do something like the following?

Certainly. So I tried to gather common part up.

> http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode92
> LayoutTests/fast/events/event-input-contentEditable.html:92: var target6end    = document.getElementById("target6end");
> Nit: webkit style is to not align the equals.

Fixed.

> 
> http://wkrietveld.appspot.com/26526/diff/2001/3004#newcode106
> LayoutTests/fast/events/event-input-contentEditable.html:106: setTimeout(function() {
> Can this setTimeout be removed? Then you wouldn't need to call waitUntilDone/notifyDone. In addition to making tests slower, setTimeouts often lead to flaky tests. We should avoid them unless they're really necessary.

Removed, and it works.
I don't remember why I did such...

> 
> http://wkrietveld.appspot.com/26526/diff/2001/3011
> File WebCore/dom/Node.cpp (right):
> 
> http://wkrietveld.appspot.com/26526/diff/2001/3011#newcode3032
> WebCore/dom/Node.cpp:3032: event->setDefaultHandled();
> Why do you need to set default handled here? Other events don't. I tried patching in your change without this line and the layout test still passes.

Removed.
I thought we need this because we are canceling default action.
But it doesn't matter because it's an internal event.

[Hajime Morrita]

Committed r62741: <http://trac.webkit.org/changeset/62741>