Last Comment Bug 713502 - input event should be fired after compositionupdate
: input event should be fired after compositionupdate
Status: RESOLVED FIXED
: dev-doc-complete, inputmethod
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
:
Mentors:
Depends on: 717147 717560 1166695 1167095
Blocks: 543789
  Show dependency treegraph
 
Reported: 2011-12-25 23:32 PST by Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
Modified: 2015-09-25 12:00 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (12.54 KB, patch)
2011-12-26 05:42 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
ehsan: review+
bugs: review+
Details | Diff | Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-12-25 23:32:23 PST
I realized that two compatibility issues of composition event handling.

1. at compositionupdate event handler, <input>.value returns old value on Gecko and WebKit, but IE returns new value.

2. input events are fired during composition on IE and WebKit, but we don't dispatch them. And input event handler on WebKit can get the newer value of the composing editor.

So, web developers cannot get newer value on Gecko. I think that we should fire input event during composition too.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-12-26 00:45:14 PST
Hmm, there are a lot of input event handlers in chrome. I think that we need to change most of them. These handlers shouldn't process during composition. E.g., if the handler changes the editor's size or content, IME is committed forcibly. It breaks IME's behavior. We can take one of following 3 ways for fix this issue:

1. Add MozIsComposing attribute for DOM Input event which returns TRUE if it's fired during composition. (Or inputMethod attribute like textinput of D3E?)

2. Gets the target editor element in each handler, and check if IME is composing. Like this: http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/forms.js#296

3. Dispatch "inputwithoutcomposition" event or something and handle it instead of new "input" event.

#3 is ugly, I don't like to do this, it's the safest way though.

#2 might be difficult to get the target editor in some handlers. And the code looks a little bit complicated.

#1 might be better, however, it adds the non-standard attribute to the event. And input event is now using nsUIEvent and nsDOMUIEvent. So, we need to create a new internal event struct and a new DOM event implementation.

Smaug, how do you think about this?
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-12-26 01:06:32 PST
input event handlers:

http://mxr.mozilla.org/mozilla-central/search?string=addEventListener%28\%22input\%22&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
http://mxr.mozilla.org/mozilla-central/search?string=event%3D\%22input\%22&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

There might be other handlers, but they will be found by testers if so :-(
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-12-26 01:09:44 PST
And I worry that our UI developers may create new IME related bugs if they don't know about the problem (composition is forcibly committed by some actions in input handlers) after this bug is fixed.
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-12-26 04:34:46 PST
Hmm, contrary to my expectation, I have not found any problems with patched build without any changes of input event handlers. I think I don't need to change them for now. I'll post a patch soon.

Sorry for the spam comment (comment 1).
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-12-26 05:42:08 PST
Created attachment 584322 [details] [diff] [review]
Patch
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-12-26 05:46:25 PST
FYI:

The nsTextEditListener is the only observer of edit actions, some add-ons might be so too, though.

http://mxr.mozilla.org/mozilla-central/ident?i=AddEditorObserver&filter=
Comment 7 :Ehsan Akhgari (busy, don't ask for review please) 2011-12-29 12:07:08 PST
Comment on attachment 584322 [details] [diff] [review]
Patch

r=me on the editor part.
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-12-31 06:07:41 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc0a1ede6a9
Comment 9 Phil Ringnalda (:philor) 2011-12-31 19:02:35 PST
https://hg.mozilla.org/mozilla-central/rev/7fc0a1ede6a9
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-01-09 18:51:19 PST
Created:
https://developer.mozilla.org/en/DOM/DOM_event_reference/input

Updated:
https://developer.mozilla.org/en/DOM/DOM_event_reference/compositionupdate

They need some help of native English speakers.
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-01-10 20:28:05 PST
Hmm, I realized that why the input events during composition don't cause any regressions in chrome.

By the patch, the input events are fired as untrusted events because "text" event handler doesn't make nsAutoEditorKeypressOperation instance.

I write a patch which makes nsAutoEditorKeypressOperation in the stack at text event handler. Then, I can see some regressions in chrome UI.

I think whether trusted or untrusted event is not checked by web pages, therefore, the new input events also useful for web pages. Therefore, I don't think that the patch should be backed out from m-c. How do you think, smaug?
Comment 12 Olli Pettay [:smaug] 2012-01-11 01:46:06 PST
Well, looks like you got patches to fix chrome and make the event trusted.
I think there is still time to fix that all before FF12 goes to Aurora.
Comment 13 Eric Shepherd [:sheppy] 2012-02-13 12:36:36 PST
Documentation updated:

https://developer.mozilla.org/en/DOM/DOM_event_reference/compositionupdate

And mentioned on Firefox 12 for developers.
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-13 18:48:37 PST
(In reply to Eric Shepherd [:sheppy] from comment #13)
> Documentation updated:
> 
> https://developer.mozilla.org/en/DOM/DOM_event_reference/compositionupdate
> 
> And mentioned on Firefox 12 for developers.

Thank you, sheppy. Could you check input event document too?
https://developer.mozilla.org/en/DOM/DOM_event_reference/input
Comment 15 Eric Shepherd [:sheppy] 2012-02-14 05:41:28 PST
I cleaned up the input article a bit. Looks good to me.
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-15 19:17:44 PST
Thanks, I translated it to Japanese.

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