Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla14

Status

()

Core
Editor
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Mathias Bynens, Assigned: masayuki)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

Trunk
mozilla14
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
Opera has the same bug (just filed it as DSK-341253). WebKit gets it right.
Component: General → Editor
Product: Firefox → Core
QA Contact: general → editor

Comment 2

6 years ago
Note that this is a WebKit extension. There is no specification for it.
(Reporter)

Comment 3

6 years ago
http://www.w3.org/Bugs/Public/show_bug.cgi?id=13118
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.
OS: Mac OS X → All
Hardware: x86 → All
Summary: oninput doesn’t work for contenteditable elements oninput doesn’t work for contenteditable elements → oninput (input event) doesn’t work for contenteditable elements oninput (input event) doesn’t work for contenteditable elements
(Reporter)

Comment 5

6 years ago
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
With tests, too.  Please review and give feedback if you want to implement this.  Ryosuke has expressed interest in implementing this soon for WebKit.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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().
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Created attachment 607850 [details] [diff] [review]
Patch (WIP)

still making tests...
(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!
Created attachment 608574 [details] [diff] [review]
Patch
Attachment #607850 - Attachment is obsolete: true
Attachment #608574 - Flags: superreview?(bugs)
Attachment #608574 - Flags: review?(ehsan)
Attachment #608574 - Flags: review?(bugs)
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 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?
Attachment #608574 - Flags: review?(ehsan)
Keywords: dev-doc-needed
(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...
Created attachment 608645 [details] [diff] [review]
Patch

The XXX comment is still there. I want to hear smaug's idea.
Attachment #608574 - Attachment is obsolete: true
Attachment #608645 - Flags: superreview?(bugs)
Attachment #608645 - Flags: review?(ehsan)
Attachment #608645 - Flags: review?(bugs)
Attachment #608574 - Flags: superreview?(bugs)
Attachment #608574 - Flags: review?(bugs)
(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 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?
Attachment #608645 - Flags: review?(ehsan) → review+
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
(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
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
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.
(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.
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.
Attachment #608645 - Attachment is obsolete: true
Attachment #608953 - Flags: superreview?(bugs)
Attachment #608953 - Flags: review?(bugs)
Attachment #608645 - Flags: superreview?(bugs)
Attachment #608645 - Flags: review?(bugs)
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.
Ah, this may be wrong:

in HandlingTrustedAction::Init():

> aSelf->mHandlingTrustedAction = aIsTrusted;

Should it be:

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

?
Only when it's nested, so, we need to manage nesting count too if comment 24 is correct.
Created attachment 608955 [details] [diff] [review]
Patch

fixed above issue.
Attachment #608953 - Attachment is obsolete: true
Attachment #608955 - Flags: superreview?(bugs)
Attachment #608955 - Flags: review?(bugs)
Attachment #608953 - Flags: superreview?(bugs)
Attachment #608953 - Flags: review?(bugs)
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 :)
Attachment #608955 - Flags: superreview?(bugs)
Attachment #608955 - Flags: superreview+
Attachment #608955 - Flags: review?(bugs)
Attachment #608955 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4d4400c93b1

Thank you. I'll post the timestamp patch when I have much time...
the test added here is failing intermittently, see bug 739557

Updated

5 years ago
Depends on: 739557
https://hg.mozilla.org/mozilla-central/rev/e4d4400c93b1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
updated:
https://developer.mozilla.org/en/DOM/DOM_event_reference/input
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 741193
Blocks: 627726

Updated

5 years ago
Depends on: 825924
You need to log in before you can comment on or make changes to this bug.