Closed
Bug 975383
Opened 10 years ago
Closed 10 years ago
Dispatch compositionupdate from TextComposition rather than widget
Categories
(Core :: DOM: Events, defect)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
(Keywords: inputmethod)
Attachments
(9 files)
8.89 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
3.30 KB,
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
1.69 KB,
patch
|
smichaud
:
review+
|
Details | Diff | Splinter Review |
2.81 KB,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
1.62 KB,
patch
|
emk
:
review+
|
Details | Diff | Splinter Review |
4.06 KB,
patch
|
emk
:
review+
|
Details | Diff | Splinter Review |
2.64 KB,
patch
|
xyuan
:
review+
|
Details | Diff | Splinter Review |
2.19 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
70.41 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
Spinning out from bug 960871. Dispatching compositionupdate event needs to check the composition string is actually updating or not. It's possible to check in TextComposition. Therefore, it's very redundant doing it in widget level.
Assignee | ||
Comment 1•10 years ago
|
||
A compositionupdate should be dispatched when text event changes composition string. In other words, text event doesn't change composition string, compositionupdate event shouldn't be dispatched. This spec is annoying for every widget because they need to store last dispatched composition string and compare WidgetTextEvent::theText with it before dispatching the text event. This is redundant and error prone. Actually, looks like that Android widget dispatches compositionupdate event every time. For avoiding this issue, widget should only dispatch text events during composition. And TextComposition should dispatch compositionupdate if it's necessary. Then, we can guarantee that compositionupdate events are dispatched correctly on all platforms. First, this patch ignores compositionupdate events dispatched by widget. And call MaybyDispatchCompositionUpdate() before dispatching text event.
Attachment #8495005 -
Flags: review?(bugs)
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
By part.2 - part.7, all compositionupdate dispatchers have gone, therefore, we can remove NS_COMPOSITION_UPDATE handlers from ESM since it maybe redirects compositionupdate events coming from widget to another process. Similarly, IMEStateManager::DispatchCompositionEvent() doesn't need to handle NS_COMPOSITION_UPDATE because it's called by PresShell::HandleEventInternal() for dispatching composition events which come from widget into the tree.
Attachment #8495012 -
Flags: review?(bugs)
Assignee | ||
Comment 9•10 years ago
|
||
All tests shouldn't synthesize compositionupdate event manually.
Attachment #8495013 -
Flags: review?(bugs)
Updated•10 years ago
|
Attachment #8495012 -
Flags: review?(bugs) → review+
Comment 10•10 years ago
|
||
Comment on attachment 8495013 [details] [diff] [review] part.9 Remove compositionupdate event dispatchers from all tests > editor.addEventListener("compositionstart", handler, true); > editor.addEventListener("compositionend", handler, true); >- editor.addEventListener("compositionupdate", handler, true); Why do we want to remove this? > editor.removeEventListener("compositionstart", handler, true); > editor.removeEventListener("compositionend", handler, true); >- editor.removeEventListener("compositionupdate", handler, true); And this? We do want to still somehow test that compositionupdate is dispatched. Please explain, and possibly fix in a follow up patch (in this bug).
Attachment #8495013 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #10) > Comment on attachment 8495013 [details] [diff] [review] > part.9 Remove compositionupdate event dispatchers from all tests > > > editor.addEventListener("compositionstart", handler, true); > > editor.addEventListener("compositionend", handler, true); > >- editor.addEventListener("compositionupdate", handler, true); > Why do we want to remove this? > > > editor.removeEventListener("compositionstart", handler, true); > > editor.removeEventListener("compositionend", handler, true); > >- editor.removeEventListener("compositionupdate", handler, true); > And this? > > > We do want to still somehow test that compositionupdate is dispatched. > Please explain, and possibly fix in a follow up patch (in this bug). Oh, you're right. It's just my mistake! I'll remove these changes from the patch. This patch wants to remove just the dispatcher of compositionupdate event in all tests.
Comment 12•10 years ago
|
||
Comment on attachment 8495005 [details] [diff] [review] part.1 TextComposition should dispatch compositionupdate event automatically if text event changes composition string > if (aType.EqualsLiteral("compositionstart")) { > msg = NS_COMPOSITION_START; > } else if (aType.EqualsLiteral("compositionend")) { > msg = NS_COMPOSITION_END; > } else if (aType.EqualsLiteral("compositionupdate")) { >- msg = NS_COMPOSITION_UPDATE; >+ // Now we don't support manually dispatching composition update with this >+ // API. compositionupdate is dispatched when text event modifies >+ // composition string automatically. For backward compatibility, this >+ // shouldn't return error in this case. Could we at least warn here using NS_WARNING
Attachment #8495005 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8495006 [details] [diff] [review] part.2 Remove compositionupdate dispatchers in nsWindow of Android compoisitonupdate is dispatched by XP part (mozilla::TextComposition) if it's necessary and ignored even if widget dispatched. So, we should remove compositionupdate dispatcher from widget.
Attachment #8495006 -
Flags: review?(nchen)
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8495007 [details] [diff] [review] part.3 Remove compositionupdate dispatchers in TextInputHandler of Cocoa compoisitonupdate is dispatched by XP part (mozilla::TextComposition) if it's necessary and ignored even if widget dispatched. So, we should remove compositionupdate dispatcher from widget.
Attachment #8495007 -
Flags: review?(smichaud)
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8495008 [details] [diff] [review] part.4 Remove compositionupdate dispatchers in nsGtkIMModule of GTK compoisitonupdate is dispatched by XP part (mozilla::TextComposition) if it's necessary and ignored even if widget dispatched. So, we should remove compositionupdate dispatcher from widget.
Attachment #8495008 -
Flags: review?(karlt)
Assignee | ||
Comment 16•10 years ago
|
||
Comment on attachment 8495009 [details] [diff] [review] part.5 Remove compositionupdate dispatchers in nsIMM32Handler of Windows compoisitonupdate is dispatched by XP part (mozilla::TextComposition) if it's necessary and ignored even if widget dispatched. So, we should remove compositionupdate dispatcher from widget.
Attachment #8495009 -
Flags: review?(VYV03354)
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8495010 [details] [diff] [review] part.6 Remove compositionupdate dispatchers in nsTextStore of Windows ditto
Attachment #8495010 -
Flags: review?(VYV03354)
Assignee | ||
Comment 18•10 years ago
|
||
Comment on attachment 8495011 [details] [diff] [review] part.7 Remove compositionupdate dispatchers in forms.js of B2G compoisitonupdate is dispatched by XP part (mozilla::TextComposition) if it's necessary and ignored even if widget dispatched. So, we should remove compositionupdate dispatcher from widget. FYI: I got flame, but I've not succeeded to build and test this patch, so, this patch isn't tested. If you can, I'd like you check the behavior (not causes JS error and can input text via IME).
Attachment #8495011 -
Flags: review?(xyuan)
Assignee | ||
Comment 19•10 years ago
|
||
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/20 - 9/23, JST) from comment #18) > Comment on attachment 8495011 [details] [diff] [review] > part.7 Remove compositionupdate dispatchers in forms.js of B2G > FYI: I got flame, but I've not succeeded to build and test this patch, so, > this patch isn't tested. If you can, I'd like you check the behavior (not > causes JS error and can input text via IME). Okay, I probably succeeded to build and flash my flame with the patch. And looks like it works!
Updated•10 years ago
|
Attachment #8495007 -
Flags: review?(smichaud) → review+
Updated•10 years ago
|
Attachment #8495006 -
Flags: review?(nchen) → review+
Comment 20•10 years ago
|
||
Comment on attachment 8495009 [details] [diff] [review] part.5 Remove compositionupdate dispatchers in nsIMM32Handler of Windows Is mLastDispatchedCompositionString still needed?
Attachment #8495009 -
Flags: review?(VYV03354) → review+
Comment 21•10 years ago
|
||
Comment on attachment 8495010 [details] [diff] [review] part.6 Remove compositionupdate dispatchers in nsTextStore of Windows Same as above. (i.e. Is mLastData still useful?)
Attachment #8495010 -
Flags: review?(VYV03354) → review+
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #20) > Comment on attachment 8495009 [details] [diff] [review] > part.5 Remove compositionupdate dispatchers in nsIMM32Handler of Windows > > Is mLastDispatchedCompositionString still needed? Yes, this is used for setting data value of compositionend. However, I guess that it will be removed if compositionend event is also dispatched from TextComposition automatically. (In reply to Masatoshi Kimura [:emk] from comment #21) > Comment on attachment 8495010 [details] [diff] [review] > part.6 Remove compositionupdate dispatchers in nsTextStore of Windows > > Same as above. (i.e. Is mLastData still useful?) I checked this again. I guess I can get rid of this in this bug. Good catch.
Updated•10 years ago
|
Attachment #8495008 -
Flags: review?(karlt) → review+
Comment 23•10 years ago
|
||
Comment on attachment 8495011 [details] [diff] [review] part.7 Remove compositionupdate dispatchers in forms.js of B2G Review of attachment 8495011 [details] [diff] [review]: ----------------------------------------------------------------- How can we change the composition text after starting composition? Currently we may call setComposition() more than one times before calling endComposition(). The patch prevents a successive setComposition() calling to change the composition text. Please make sure the patch doesn't break the following user case: 1. calling setComposition('a') to start composition and set the composition text to 'a' 2. calling setComposition('ab') to change the composition text to 'ab' 3. calling endComposition('ab') to end composition and commit 'ab'
Attachment #8495011 -
Flags: review?(xyuan) → review-
Assignee | ||
Comment 24•10 years ago
|
||
Comment on attachment 8495011 [details] [diff] [review] part.7 Remove compositionupdate dispatchers in forms.js of B2G (In reply to Yuan Xulei [:yxl] (PTO until Oct 8) from comment #23) > How can we change the composition text after starting composition? From forms.js, it's possible to use nsICompositionStringSynthesizer. See here: http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#1244 Note that nsEditor doesn't handle compositionupdate event. It's just a notification event for web apps immediately before modifying composition string. Instead, DOM text event (not defined by any web stnadards) is handled by ns(Plaintext)Editor and it causes the composition string. nsICompositionStringSynthesizer is the dispatcher of a DOM text event. What this bug makes is that compositionupdate events which don't affect our behavior dispatched by TextComposition class when it receives DOM text event and composition string is actually changed. So, removing compositionupdate dispatcher from form.js doesn't change any behavior. The removed job is performed by TextComposition automatically. See part.1 patch if you want to confirm what I actually does in this bug. > Currently we may call setComposition() more than one times before calling > endComposition(). The patch prevents a successive setComposition() calling > to change the composition text. > Please make sure the patch doesn't break the following user case: > > 1. calling setComposition('a') to start composition and set the composition > text to 'a' > 2. calling setComposition('ab') to change the composition text to 'ab' > 3. calling endComposition('ab') to end composition and commit 'ab' I confirmed on my flame: 1. Activate built-in Chinese IME. 2. Type 'm' 3. Type 'a' 4. Tap the editor Then, "ma" is committed. This must be right result.
Attachment #8495011 -
Flags: review- → review?(xyuan)
Comment 25•10 years ago
|
||
Comment on attachment 8495011 [details] [diff] [review] part.7 Remove compositionupdate dispatchers in forms.js of B2G Review of attachment 8495011 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for clarification.
Attachment #8495011 -
Flags: review?(xyuan) → review+
Assignee | ||
Comment 26•10 years ago
|
||
Thank you everybody! https://hg.mozilla.org/integration/mozilla-inbound/rev/3c8da0709f07 https://hg.mozilla.org/integration/mozilla-inbound/rev/f784b7a2d718 https://hg.mozilla.org/integration/mozilla-inbound/rev/84a46c657337 https://hg.mozilla.org/integration/mozilla-inbound/rev/e9da63e6046b https://hg.mozilla.org/integration/mozilla-inbound/rev/7c64942d1d41 https://hg.mozilla.org/integration/mozilla-inbound/rev/432ecdbf058a https://hg.mozilla.org/integration/mozilla-inbound/rev/d4b4cca22c19 https://hg.mozilla.org/integration/mozilla-inbound/rev/e730625ff9e1 https://hg.mozilla.org/integration/mozilla-inbound/rev/3ec43ac8b65c
Comment 27•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3c8da0709f07 https://hg.mozilla.org/mozilla-central/rev/f784b7a2d718 https://hg.mozilla.org/mozilla-central/rev/84a46c657337 https://hg.mozilla.org/mozilla-central/rev/e9da63e6046b https://hg.mozilla.org/mozilla-central/rev/7c64942d1d41 https://hg.mozilla.org/mozilla-central/rev/432ecdbf058a https://hg.mozilla.org/mozilla-central/rev/d4b4cca22c19 https://hg.mozilla.org/mozilla-central/rev/e730625ff9e1 https://hg.mozilla.org/mozilla-central/rev/3ec43ac8b65c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•