Last Comment Bug 668606 - oninput (input event) doesn’t work for contenteditable elements
: oninput (input event) doesn’t work for contenteditable elements
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on: 741193 739557 825924
Blocks: 627726
  Show dependency treegraph
 
Reported: 2011-06-30 12:37 PDT by Mathias Bynens
Modified: 2013-01-02 10:12 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (WIP) (26.98 KB, patch)
2012-03-20 21:47 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Patch (42.51 KB, patch)
2012-03-22 19:23 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Patch (41.86 KB, patch)
2012-03-23 03:27 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
ehsan: review+
Details | Diff | Review
Patch (43.98 KB, patch)
2012-03-23 19:24 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Patch (44.37 KB, patch)
2012-03-23 19:59 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
bugs: superreview+
Details | Diff | Review

Description Mathias Bynens 2011-06-30 12:37:10 PDT
The `oninput` event doesn’t seem to fire for contenteditable elements.

Test case:

data:text/html;charset=utf-8,%3C!DOCTYPE%20html%3E%3Ctitle%3Eoninput%2Bcontenteditable%3C%2Ftitle%3E%3Cp%20oninput%3D%22alert('YAY')%22%20contenteditable%3Eediting%20me%20should%20alert%20YAY
Comment 1 Mathias Bynens 2011-07-01 03:30:57 PDT
Opera has the same bug (just filed it as DSK-341253). WebKit gets it right.
Comment 2 Anne (:annevk) 2011-07-02 02:23:39 PDT
Note that this is a WebKit extension. There is no specification for it.
Comment 3 Mathias Bynens 2011-07-02 03:54:24 PDT
http://www.w3.org/Bugs/Public/show_bug.cgi?id=13118
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-01-09 20:06:06 PST
IE supports it too. However, the event target is different from WebKit.
https://developer.mozilla.org/en/DOM/DOM_event_reference/input

WebKit's target is the editing host. However, IE's target is the innermost element at the caret position.
Comment 6 :Aryeh Gregor (away until August 15) 2012-02-24 09:35:37 PST
With tests, too.  Please review and give feedback if you want to implement this.  Ryosuke has expressed interest in implementing this soon for WebKit.
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-02-24 15:52:42 PST
That's interesting. If I had time in March, I'll research it and write patches. However, if somebody wants to take this bug, it's welcome.
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-02-24 16:36:59 PST
Currently, we're dispatching input events from nsTextControlFrame via nsTextEditorState (nsIEditorObserver). It did make sense for not dispatching on HTML editor. However, for this bug, we should dispatch it from ns*Editor directly.

Looks like we're dispatching input event synchronously and discards the input events if it's unsafe to dispatch events. I think that we should use nsContentUtils::AddScriptRunner().
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-20 21:47:12 PDT
Created attachment 607850 [details] [diff] [review]
Patch (WIP)

still making tests...
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2012-03-21 10:00:05 PDT
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #8)
> Looks like we're dispatching input event synchronously and discards the
> input events if it's unsafe to dispatch events. I think that we should use
> nsContentUtils::AddScriptRunner().

I would definitely do that!
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-22 19:23:39 PDT
Created attachment 608574 [details] [diff] [review]
Patch
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-22 19:31:52 PDT
The patch makes nsEditor dispatch input event. The input event is made of nsEvent rather than nsUIEvent. The event shouldn't have detail attribute. So, the current implementation is wrong.

And if it's HTML editor, the event target is the active editing host. I have no idea that the NotifyEditorAction() is called when the editor doesn't have focus.

If contenteditable element is nested, the event target becomes the top most element in the tree. I.e., it has non-editable element as parent or is root element.
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2012-03-22 20:59:29 PDT
Comment on attachment 608574 [details] [diff] [review]
Patch

Review of attachment 608574 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great, thanks a lot for your patch, Masayuki!

I'd like to see a version of the patch which addresses my comments below.  Thanks!

::: editor/libeditor/base/nsEditor.cpp
@@ +1771,5 @@
> +    // Note that we don't need to check mDispatchInputEvent here.  We need
> +    // to check it only when the editor requests to dispatch the input event.
> +
> +    // XXX Do we need to check if the editor has been destroyed already?
> +    //     But even if so, content may not be removed...

How can the editor be destroyed when you're holding a strong reference to it?  :-)

The editor might be detached from the document/content though.  But I think the check below should take care of everything we need to worry about.  Do you agree?

If so, we should probably get rid of this comment.

@@ +1786,5 @@
> +    nsEvent inputEvent(mIsTrusted, NS_FORM_INPUT);
> +    nsEventStatus status = nsEventStatus_eIgnore;
> +    DebugOnly<nsresult> rv =
> +      ps->HandleEventWithTarget(&inputEvent, nsnull, mTarget, &status);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

Is this guaranteed that this will *never* fail?  MOZ_ASSERT is fatal...

@@ +1809,5 @@
> +
> +  // We don't need to dispatch multiple input events if there is a pending
> +  // input event.  However, it may have different event target.  If we resloved
> +  // this issue, we need to manage the pending events in an array.  But it's
> +  // overwork.  We don't need to do it for the very rare case.

Does this actually happen in practice?  If so, under what circumstances?

::: editor/libeditor/base/nsEditor.h
@@ +752,5 @@
>    virtual already_AddRefed<nsIDOMNode> FindUserSelectAllNode(nsIDOMNode* aNode) { return nsnull; }
>  
> +  NS_STACK_CLASS class HandlingTrustedAction {
> +  public:
> +    HandlingTrustedAction(nsEditor* aSelf, bool aIsTrusted = true)

Nit: please make the constructor explicit.

@@ +773,5 @@
> +    {
> +      MOZ_ASSERT(aSelf);
> +
> +      mEditor = aSelf;
> +      mWasHandlingTrustedAction = aSelf->mHandlingTrustedAction;

Hmm, how does this compile?  Shouldn't you make HandlingTrustedAction a friend of nsEditor?

::: editor/libeditor/html/tests/test_dom_input_event_on_htmleditor.html
@@ +30,5 @@
> +const kIsMac = navigator.platform.indexOf("Mac") == 0;
> +
> +function runTests()
> +{
> +

Nit: remove the empty line please.

@@ +51,5 @@
> +    var inputEvent = null;
> +
> +    var handler = function (aEvent) {
> +      if (aEvent.target != eventTarget) {
> +        ok(false, "input event is fired on unexpected element: " + aEvent.target.tagName);

Nit: Use is(aEvent.target, eventTarget, "...");

@@ +62,5 @@
> +
> +    inputEvent = null;
> +    synthesizeKey("a", { }, aWindow);
> +    if (editTarget != eventTarget) {
> +      is(editTarget.innerHTML, "a", aDescription + "wrong element was editted");

Nit: edited.

@@ +67,5 @@
> +    }
> +    ok(inputEvent, aDescription + "input event wasn't fired by 'a' key");
> +    if (inputEvent) {
> +      ok(inputEvent.isTrusted, aDescription + "input event by 'a' key wasn't trusted event");
> +    }

Nit: no need to check inputEvent here and elsewhere in this function.

::: editor/libeditor/text/tests/test_dom_input_event_on_texteditor.html
@@ +27,5 @@
> +const kIsMac = navigator.platform.indexOf("Mac") == 0;
> +
> +function runTests()
> +{
> +

Nit: empty new line.

Also, my nits in test_dom_input_event_on_htmleditor.html apply here as well.  :-)

::: layout/forms/nsTextControlFrame.h
@@ +223,5 @@
>        // but only if user hasn't changed the value.
>        , mFocusValueInit(!mFrame->mFireChangeEventState && aHasFocusValue)
>        , mOuterTransaction(false)
>        , mInited(false)
> +      , mCanceled(false)

Can't you get rid of mInited and just use mCanceled here?  (You need to set it to false in Init()).

@@ +231,3 @@
>      }
>      void Cancel() {
> +      mCanceled = false;

This should be true, right?
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-22 22:32:13 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> ::: editor/libeditor/base/nsEditor.cpp
> @@ +1771,5 @@
> > +    // Note that we don't need to check mDispatchInputEvent here.  We need
> > +    // to check it only when the editor requests to dispatch the input event.
> > +
> > +    // XXX Do we need to check if the editor has been destroyed already?
> > +    //     But even if so, content may not be removed...
> 
> How can the editor be destroyed when you're holding a strong reference to
> it?  :-)
> 
> The editor might be detached from the document/content though.  But I think
> the check below should take care of everything we need to worry about.  Do
> you agree?
> 
> If so, we should probably get rid of this comment.

Ah, I worried about the detached situation. And I'm not sure the safety because I don't know what happens if editor is detached from content/document, therefore, I wrote the comment in the patch. Smaug, do you have any idea?

> @@ +1786,5 @@
> > +    nsEvent inputEvent(mIsTrusted, NS_FORM_INPUT);
> > +    nsEventStatus status = nsEventStatus_eIgnore;
> > +    DebugOnly<nsresult> rv =
> > +      ps->HandleEventWithTarget(&inputEvent, nsnull, mTarget, &status);
> > +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> 
> Is this guaranteed that this will *never* fail?  MOZ_ASSERT is fatal...

It's possible. If it failed, nsEditor couldn't do nothing additionally. So, did you mean I should just remove the rv checking on debug build?

> 
> @@ +1809,5 @@
> > +
> > +  // We don't need to dispatch multiple input events if there is a pending
> > +  // input event.  However, it may have different event target.  If we resloved
> > +  // this issue, we need to manage the pending events in an array.  But it's
> > +  // overwork.  We don't need to do it for the very rare case.
> 
> Does this actually happen in practice?  If so, under what circumstances?

I guess that if editor action observer dispatches trusted input events, it's possible. It never happens normally, but it were possible if some addons did it.

> @@ +773,5 @@
> > +    {
> > +      MOZ_ASSERT(aSelf);
> > +
> > +      mEditor = aSelf;
> > +      mWasHandlingTrustedAction = aSelf->mHandlingTrustedAction;
> 
> Hmm, how does this compile?  Shouldn't you make HandlingTrustedAction a
> friend of nsEditor?

Because the HandlingTrustedAction is a child class of nsEditor. (I'm not sure "child" is valid term of this case, though)

> @@ +231,3 @@
> >      }
> >      void Cancel() {
> > +      mCanceled = false;
> 
> This should be true, right?

Oops...
Comment 15 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-23 03:27:34 PDT
Created attachment 608645 [details] [diff] [review]
Patch

The XXX comment is still there. I want to hear smaug's idea.
Comment 16 :Ehsan Akhgari (busy, don't ask for review please) 2012-03-23 11:22:41 PDT
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> (In reply to Ehsan Akhgari [:ehsan] from comment #13)
> > ::: editor/libeditor/base/nsEditor.cpp
> > @@ +1771,5 @@
> > > +    // Note that we don't need to check mDispatchInputEvent here.  We need
> > > +    // to check it only when the editor requests to dispatch the input event.
> > > +
> > > +    // XXX Do we need to check if the editor has been destroyed already?
> > > +    //     But even if so, content may not be removed...
> > 
> > How can the editor be destroyed when you're holding a strong reference to
> > it?  :-)
> > 
> > The editor might be detached from the document/content though.  But I think
> > the check below should take care of everything we need to worry about.  Do
> > you agree?
> > 
> > If so, we should probably get rid of this comment.
> 
> Ah, I worried about the detached situation. And I'm not sure the safety
> because I don't know what happens if editor is detached from
> content/document, therefore, I wrote the comment in the patch. Smaug, do you
> have any idea?

Currently for plaintext editors, the editor is always attached to the content node (except for input elements where we initialize the editor lazily if needed, but once the editor is initialized, it is always attached to the content node.)  There are some editor operations which require a frame to exist (firing the input event was one of them) and those operations will fail if the frame for the element does not exist.

For HTML editors, the editor is always attached to the document object, but again some of its operations require a frame/presshell to be present.

> > @@ +1786,5 @@
> > > +    nsEvent inputEvent(mIsTrusted, NS_FORM_INPUT);
> > > +    nsEventStatus status = nsEventStatus_eIgnore;
> > > +    DebugOnly<nsresult> rv =
> > > +      ps->HandleEventWithTarget(&inputEvent, nsnull, mTarget, &status);
> > > +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> > 
> > Is this guaranteed that this will *never* fail?  MOZ_ASSERT is fatal...
> 
> It's possible. If it failed, nsEditor couldn't do nothing additionally. So,
> did you mean I should just remove the rv checking on debug build?

Then you should replace it with NS_ENSURE_SUCCESS.

> > @@ +1809,5 @@
> > > +
> > > +  // We don't need to dispatch multiple input events if there is a pending
> > > +  // input event.  However, it may have different event target.  If we resloved
> > > +  // this issue, we need to manage the pending events in an array.  But it's
> > > +  // overwork.  We don't need to do it for the very rare case.
> > 
> > Does this actually happen in practice?  If so, under what circumstances?
> 
> I guess that if editor action observer dispatches trusted input events, it's
> possible. It never happens normally, but it were possible if some addons did
> it.

OK.

> > @@ +773,5 @@
> > > +    {
> > > +      MOZ_ASSERT(aSelf);
> > > +
> > > +      mEditor = aSelf;
> > > +      mWasHandlingTrustedAction = aSelf->mHandlingTrustedAction;
> > 
> > Hmm, how does this compile?  Shouldn't you make HandlingTrustedAction a
> > friend of nsEditor?
> 
> Because the HandlingTrustedAction is a child class of nsEditor. (I'm not
> sure "child" is valid term of this case, though)

Hmm, it's a nested class, yes.  I didn't know that nested classes automatically gain access to their nester's private parts!
Comment 17 :Ehsan Akhgari (busy, don't ask for review please) 2012-03-23 11:24:57 PDT
Comment on attachment 608645 [details] [diff] [review]
Patch

Review of attachment 608645 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments addressed.

::: layout/forms/nsTextControlFrame.h
@@ +221,5 @@
>        // restores it afterwards (ie. we want 'change' events for those changes).
>        // Focused value must be updated to prevent incorrect 'change' events,
>        // but only if user hasn't changed the value.
>        , mFocusValueInit(!mFrame->mFireChangeEventState && aHasFocusValue)
> +      , mCanceled(true)

I think you should make this default to false.

@@ +236,5 @@
> +      mCanceled = true;
> +    }
> +    void Init() {
> +      mEditor->SetSuppressDispatchingInputEvent(true);
> +      mCanceled = false;

If you do the above, you can get rid of this assignment.

@@ +246,2 @@
>          return;
> +      }

Shouldn't this be at the beginning of the destructor?
Comment 18 :Aryeh Gregor (away until August 15) 2012-03-23 13:42:46 PDT
Is this implemented at all like the spec?  How does your patched build do on the official test-case?

http://dvcs.w3.org/hg/editing/raw-file/tip/conformancetest/event.html
Comment 19 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-23 16:51:38 PDT
(In reply to Aryeh Gregor from comment #18)
> Is this implemented at all like the spec?  How does your patched build do on
> the official test-case?
> 
> http://dvcs.w3.org/hg/editing/raw-file/tip/conformancetest/event.html

current trunk build:

> Fail	Simple editable div: beforeinput event, canceled	assert_equals: number of beforeinput events fired expected 1 but got 0
> Fail	Simple editable div: input event, canceled	assert_true: div contents must not be changed expected true got false
> Fail	Simple editable div: beforeinput event, uncanceled	assert_equals: number of beforeinput events fired expected 1 but got 0
> Fail	Simple editable div: input event, uncanceled	assert_equals: number of input events fired expected 1 but got 0

patched build:

> Fail	Simple editable div: beforeinput event, canceled	assert_equals: number of beforeinput events fired expected 1 but got 0
> Fail	Simple editable div: input event, canceled	assert_equals: number of input events fired expected 0 but got 1
> Fail	Simple editable div: beforeinput event, uncanceled	assert_equals: number of beforeinput events fired expected 1 but got 0
> Fail	Simple editable div: input event, uncanceled	assert_equals: event.cancelable expected false but got true
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-23 17:05:01 PDT
Cancelable input event is my mistake in the patch, I'll fix it and adding the automated tests.

If it's possible, I'd like to request that you separate the tests two files, one tests only input event, the other tests with beforeinput event.
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-23 19:20:32 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #17)
> @@ +246,2 @@
> >          return;
> > +      }
> 
> Shouldn't this be at the beginning of the destructor?

I don't think so. If Cancel() is called after Init(), we must restore the old state. If we don't do so, <input> or <textarea> never fires input event after that.

(In reply to Ehsan Akhgari [:ehsan] from comment #16)
> Currently for plaintext editors, the editor is always attached to the
> content node (except for input elements where we initialize the editor
> lazily if needed, but once the editor is initialized, it is always attached
> to the content node.)  There are some editor operations which require a
> frame to exist (firing the input event was one of them) and those operations
> will fail if the frame for the element does not exist.
> 
> For HTML editors, the editor is always attached to the document object, but
> again some of its operations require a frame/presshell to be present.

Thank you for the explanation. It might be better to check whether the target content has primary frame or not. However, I'm not sure whether it's correct. I just removed the XXX comment from here without any change.
Comment 22 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-23 19:24:00 PDT
Created attachment 608953 [details] [diff] [review]
Patch

I add nsEvent::SetCurrentTime(). Looks like |PR_Now() / 1000| returns the expected time for DOM Event.timeStamp. However, some other events are set PR_IntervalNow() value. It's not correct when I use the value for DOM Date object. We should fix them in another bug.
Comment 23 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-23 19:36:01 PDT
The new patched build:

> Fail	Simple editable div: beforeinput event, canceled	assert_equals: number of beforeinput events fired expected 1 but got 0
> Fail	Simple editable div: input event, canceled	assert_equals: number of input events fired expected 0 but got 1
> Fail	Simple editable div: beforeinput event, uncanceled	assert_equals: number of beforeinput events fired expected 1 but got 0
> Fail	Simple editable div: input event, uncanceled	assert_equals: event.isTrusted expected true but got false

Looks like still failed by isTrusted value at sending command. But I think that we should fix it in another bug. It needs more tests.
Comment 24 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-23 19:40:08 PDT
Ah, this may be wrong:

in HandlingTrustedAction::Init():

> aSelf->mHandlingTrustedAction = aIsTrusted;

Should it be:

> aSelf->mHandlingTrustedAction = (aSelf->mHandlingTrustedAction && aIsTrusted);

?
Comment 25 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-23 19:41:56 PDT
Only when it's nested, so, we need to manage nesting count too if comment 24 is correct.
Comment 26 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-23 19:59:45 PDT
Created attachment 608955 [details] [diff] [review]
Patch

fixed above issue.
Comment 27 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-03-26 16:27:57 PDT
Comment on attachment 608955 [details] [diff] [review]
Patch

>+  NS_STACK_CLASS class HandlingTrustedAction {
{ should be in the next line


>+  void SetCurrentTime()
>+  {
>+    time = static_cast<PRUint64>(PR_Now() / 1000);
>+  }
There is an existing bug for this. This change shouldn't happen in this bug.
Could you perhaps post a new patch without changes to time handling.


Looks good though :)
Comment 28 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-26 18:39:53 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4d4400c93b1

Thank you. I'll post the timestamp patch when I have much time...
Comment 29 Marco Bonardo [::mak] 2012-03-27 04:32:30 PDT
the test added here is failing intermittently, see bug 739557
Comment 30 Ed Morley [:emorley] 2012-03-27 05:17:49 PDT
https://hg.mozilla.org/mozilla-central/rev/e4d4400c93b1
Comment 31 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-27 19:33:18 PDT
updated:
https://developer.mozilla.org/en/DOM/DOM_event_reference/input

Note You need to log in before you can comment on or make changes to this bug.