Closed Bug 668606 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: DOM: Editor, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: mathias, Assigned: masayuki)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 4 obsolete files)

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
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
Note that this is a WebKit extension. There is no specification for it.
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
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
Attached patch Patch (WIP) (obsolete) — Splinter Review
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!
Attached patch Patch (obsolete) — Splinter Review
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...
Attached patch Patch (obsolete) — Splinter Review
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.
Attached patch Patch (obsolete) — Splinter Review
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.
Attached patch PatchSplinter Review
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
Depends on: 739557
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Depends on: 825924
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: