294168 - IME transaction of Undo/Redo buffer is broken when selected text is deleted by IME input

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If the text that is in editor is selected, we can destroy undo/redo buffer by
IME input.(See this report by UTF-8)

i.e.,

1. type 'ab' in an editor
 a bI
2. select 'a' in the editor
[a]b
3. type any key with IME
 あIb
4. Undo
 bI                               <- 'a' is not recovered and caret position is 
                                     wrong
5. Redo
 あI                              <- 'b' is deleted!

## I ... caret position
## [ ] ... selection range


However, if we selected all text in the editor, we cannot reproduce it.

i.e.,
1. type 'ab'
 a bI
2. select 'ab'
[a b]
3. type with IME
 あI
4. Undo                   <-- 'ab' is recovered and it's selected.
[a b]
5. Redo
[あ]

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

if defined DEBUG_IMETXN in IMETextTxn.cpp, we can get following log.

first case(buggy)
Do IME Text element = 037FE0D4 replace = 0 len = 1
nsIPrivateTextRangeList[02F57A28]
range[0] start=1 end=1 type=TEXTRANGE_CARETPOSITION
range[1] start=0 end=1 type=TEXTRANGE_RAWINPUT
------------------------------------------------------------------
Do IME Text element = 037FE0D4 replace = 1 len = 1
nsIPrivateTextRangeList[02F58590]
range[0] start=1 end=1 type=TEXTRANGE_CARETPOSITION
range[1] start=0 end=1 type=TEXTRANGE_RAWINPUT
------------------------------------------------------------------
Do IME Text element = 037FE0D4 replace = 1 len = 1
nsIPrivateTextRangeList[035D85C0]
Merge IME Text element = 037FE0D4
IMETextTxn assimilated IMETextTxn:02F489C8

second case
Do IME Text element = 037FE1B4 replace = 0 len = 1
nsIPrivateTextRangeList[03A6F5E0]
range[0] start=1 end=1 type=TEXTRANGE_CARETPOSITION
range[1] start=0 end=1 type=TEXTRANGE_RAWINPUT
------------------------------------------------------------------
Do IME Text element = 037FE1B4 replace = 1 len = 1
nsIPrivateTextRangeList[03A77FF8]
range[0] start=1 end=1 type=TEXTRANGE_CARETPOSITION
range[1] start=0 end=1 type=TEXTRANGE_RAWINPUT
Merge IME Text element = 037FE1B4
IMETextTxn assimilated IMETextTxn:03A80820
------------------------------------------------------------------
Do IME Text element = 037FE1B4 replace = 1 len = 1
nsIPrivateTextRangeList[03A6F7E8]
Merge IME Text element = 037FE1B4
IMETextTxn assimilated IMETextTxn:03A80740

See second block of both case.
First case faild to marge the IMETextTxn.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

first case log.
nsPlaintextEditor::SetCompositionString
nsPlaintextEditor::InsertText
nsEditor::InsertTextIntoTextNodeImpl
CreateTxnForIMEText
IMETextTxn::Init
Editor::DoTransaction ----------
mPlaceHolderBatch: 2; mPlaceHolderTxn: 00000000;
Editor::DoTransaction ----------
mPlaceHolderBatch: 2; mPlaceHolderTxn: 03EBC0E8;
PlaceholderTxn::Merge 03EBB7E0                    <- it is PlaceholderTxn pointer
Do IME Text element = 03E9BA94 replace = 1 len = 1
nsIPrivateTextRangeList[03EBBE00]
range[0] start=1 end=1 type=TEXTRANGE_CARETPOSITION
range[1] start=0 end=1 type=TEXTRANGE_RAWINPUT
PlaceholderTxn::Merge 03EBC010                    <- difference previous object
PlaceholderTxn::Merge mAbsorb is true.
PlaceholderTxn::Merge mIMETextTxn is null.        <- fail to merge
nsEditor::EndPlaceHolderTransaction 2
nsEditor::EndPlaceHolderTransaction 1

second case log.

nsPlaintextEditor::SetCompositionString
nsPlaintextEditor::InsertText
nsEditor::InsertTextIntoTextNodeImpl
CreateTxnForIMEText
IMETextTxn::Init
Editor::DoTransaction ----------
mPlaceHolderBatch: 2; mPlaceHolderTxn: 00000000;
Editor::DoTransaction ----------
mPlaceHolderBatch: 2; mPlaceHolderTxn: 03E66008;
PlaceholderTxn::Merge 03E63E08                    <- it is PlaceholderTxn pointer
Do IME Text element = 03E479B4 replace = 1 len = 1
nsIPrivateTextRangeList[03E64530]
range[0] start=1 end=1 type=TEXTRANGE_CARETPOSITION
range[1] start=0 end=1 type=TEXTRANGE_RAWINPUT
PlaceholderTxn::Merge 03E63E08                    <- same as previous object
PlaceholderTxn::Merge mAbsorb is true.
Merge IME Text element = 03E479B4
IMETextTxn assimilated IMETextTxn:03E646D8        <- successed to merge
nsEditor::EndPlaceHolderTransaction 2
nsEditor::EndPlaceHolderTransaction 1

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

It fixes this bug.
But I don't know that this is right or not.
The if statement is added by Joe Francis in Bug 24573.

Joe:
Is it necessary?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0 -u20pw

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This patch changes following behavior, but I think it is better than current
behavior.

1. type 'aaaa'
2. select 'aa[aa]'
3. type 'bbbb'

Current behavior:
1st undo:  'aabb'
2nd undo:  'aaaa'
3rd undo:  ''

The patch's behavior:
1st undo:  'aaaa'
2nd undo:  ''

I think that the 'natural' is new behavior. Because user's step is not related
the selection.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 183607 [details] [diff] [review]
Patch rv1.0

timeless:

Can you review it?

Comment 7

[timeless]

Comment on attachment 183607 [details] [diff] [review]
Patch rv1.0

i wondered if the qi comment was correct but got distracted trying to follow
bonsai links and i've decided it's a bad use of my time.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

timeless:

I sent mail to tor. But he doesn't reply...
Aren't there other superreviewer?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 183607 [details] [diff] [review]
Patch rv1.0

Simon: Could you check it?

Comment 10

[Simon Fraser [no longer active]]

diff -w would be useful here.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Simon:
See attachment 183608 [details] [diff] [review] that is -u20pw.

Comment 12

[Simon Fraser [no longer active]]

Did you verify that this change doesn't regress any of the bugs listed in the
checkin comment for the line being removed:
"make selection sticky across undo/redo (24573); get bold/italic/underline
toolbar feedback working (24574); fix for 24856: unable to unbold text; fix for
24572: cant get selection between split lists"?

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

All of the checked-in is here.
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2000-01-31+02%3A30&maxdate=2000-01-31+02%3A30&cvsroot=%2Fcvsroot

I think that the code is for bug 24573(undo/redo should remember and set
selection) only. I tested for bug 24573, but I cannot find regression.
# Other bugs are HTML Editor's bug.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Maybe...

bug 24573 -> mozilla/ editor/ base/ PlaceholderTxn.cpp
             mozilla/ editor/ base/ PlaceholderTxn.h

bug 24574 -> mozilla/ editor/ base/ nsInterfaceState.cpp

bug 24856 -> ???

bug 24572 -> mozilla/ editor/ base/ nsHTMLEditRules.cpp

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

O.K. I completed to test for all bugs now, I cannot reproduce these bugs.

Comment 16

[Simon Fraser [no longer active]]

Comment on attachment 183607 [details] [diff] [review]
Patch rv1.0

Thanks for testing.

Comment 17

[chris hofmann]

Comment on attachment 183607 [details] [diff] [review]
Patch rv1.0

a=chofmann

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in. thank you!