[Webkit-unassigned] [Bug 26526] Add support for input events (oninput) to contentEditable elements

Thu Jun 17 15:45:04 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=26526

--- Comment #16 from Ojan Vafai <ojan at chromium.org>  2010-06-17 15:45:03 PST ---
(From update of attachment 58971)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.