668606 - oninput (input event) doesn’t work for contenteditable elements

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Mathias Bynens]

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]

Opera has the same bug (just filed it as DSK-341253). WebKit gets it right.

Comment 2

[Anne (:annevk)]

Note that this is a WebKit extension. There is no specification for it.

Comment 3

[Mathias Bynens]

http://www.w3.org/Bugs/Public/show_bug.cgi?id=13118

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 5

[Mathias Bynens]

https://www.w3.org/Bugs/Public/show_bug.cgi?id=13118 is now resolved: https://www.w3.org/Bugs/Public/show_bug.cgi?id=13118#c33

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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] (he/him)(JST, +0900)]

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] (he/him)(JST, +0900)]

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] (he/him)(JST, +0900)]

Attachment: Patch (WIP)

still making tests...

Comment 10

[(no longer active)]

(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] (he/him)(JST, +0900)]

Attachment: Patch

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

[(no longer active)]

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] (he/him)(JST, +0900)]

(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] (he/him)(JST, +0900)]

Attachment: Patch

The XXX comment is still there. I want to hear smaug's idea.

Comment 16

[(no longer active)]

(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

[(no longer active)]

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 (:ayg) (no longer with Mozilla)]

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] (he/him)(JST, +0900)]

(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] (he/him)(JST, +0900)]

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] (he/him)(JST, +0900)]

(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] (he/him)(JST, +0900)]

Attachment: 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] (he/him)(JST, +0900)]

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] (he/him)(JST, +0900)]

Ah, this may be wrong:

in HandlingTrustedAction::Init():

> aSelf->mHandlingTrustedAction = aIsTrusted;

Should it be:

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

?

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Only when it's nested, so, we need to manage nesting count too if comment 24 is correct.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

fixed above issue.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

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] (he/him)(JST, +0900)]

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]]

the test added here is failing intermittently, see bug 739557

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e4d4400c93b1

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

updated:
https://developer.mozilla.org/en/DOM/DOM_event_reference/input