Last Comment Bug 622247 - Remove the hack for bug 23558
: Remove the hack for bug 23558
: inputmethod
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
: Makoto Kato [:m_kato]
Depends on:
Blocks: 23558 271815 543789 725233
  Show dependency treegraph
Reported: 2010-12-31 01:22 PST by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2012-02-15 09:16 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (11.77 KB, patch)
2012-02-08 00:02 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch (11.67 KB, patch)
2012-02-08 01:21 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
ehsan: review+
Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-12-31 01:22:53 PST
Bug 23558 fixed the bug by hacky code. That makes a problem for implementing DOM3 composition event.

If first composition string in a transaction is empty string, editor does nothing. See nsPlaintextEditor::UpdateIMEComposition(). Against this, nsTextStore dispatches dummy text event. This will cause wrong composition event behavior. Therefore, I need to remove the hack first.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-02-08 00:02:38 PST
Created attachment 595332 [details] [diff] [review]
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-02-08 01:21:53 PST
Created attachment 595342 [details] [diff] [review]
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-02-08 07:08:39 PST
Comment on attachment 595342 [details] [diff] [review]

Now, aTextRangeList must not be NULL even if OOM happened.

mIMETextNode is already NULL after InsertText. If the composition string is empty, nsEditor::InsertTextIntoTextNodeImpl() sets NULL.

By this change, empty text event causes deleting selected text even if it's fired immediately after compositionstart event. This is important for implementing TSF of Windows. And also, such behavior is better for consistency with other actions.

The bugs which caused current hacky implementation are tested by the new automated tests.
Comment 4 :Ehsan Akhgari 2012-02-14 07:42:01 PST
Comment on attachment 595342 [details] [diff] [review]

Review of attachment 595342 [details] [diff] [review]:

r=me with the blow.

::: editor/libeditor/text/nsPlaintextEditor.cpp
@@ +978,2 @@
> +  if (true) {

Please remove this.
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-02-14 07:46:15 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> ::: editor/libeditor/text/nsPlaintextEditor.cpp
> @@ +978,2 @@
> >  
> > +  if (true) {
> Please remove this.

Did you mean, should I use just |{}| instead of |if () {}|? The block is needed for nsAutoPlaceHolderBatch. After it's destroyed, we need to call NotifyEditorObservers().
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-02-14 21:55:26 PST
I removed |if (true)|, it's not just |{}|.
Comment 7 Marco Bonardo [::mak] 2012-02-15 09:16:34 PST

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