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

Fri Jun 18 02:18:32 PDT 2010

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

--- Comment #18 from MORITA Hajime <morrita at google.com>  2010-06-18 02:18:32 PST ---
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.

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