317782 - A character is deleted on HTML Editor when running IME reconvertion with non-selected text

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

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

On HTML editor, if we try to reconvert by IME when the text is not selected, the next character from the caret is deleted. It seems we pressed delete key.
This is not reproduced on Text editor.

Comment 1

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

Attachment: Patch rv1.0

We have a bug in nsHTMLEditRules::WillDeleteSelection.
If aAction is nsIEditor::eNone, the function is running as with nsIEditor::ePrevious. Because the process is checking aAction is nsIEditor::eNext or not. So, we should return in early time if aAction is nsIEditor::eNone.

And we don't need to run nsPlaintextEditor::DeleteSelection if the selection text is empty.

And we don't need IME code in nsHTMLEditor. Becuase nsHTMLEditor::SetCompositionString and nsHTMLEditor::GetReconversionString are same as nsPlaintextEditor's methods.(nsHTMLEditor's methods are overriding nsPlaintextEditor's one.) These code are duplicated by bug 66290. We should have removed these methods in this time.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm sorry, but this needs review or at least module owner approval from someone resembling an editor peer before I can sr it.

Comment 3

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

boris:

What? This patch was already reviewed by timeless. He is editor peer.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Doh!  I missed his name in the list...

I'll try to get to this before as soon as I can, but I really don't know anything about this code (which means I'll have to learn it) and I've already got a huge backlog of reviews to do before this one, so I don't know that I'll be able to do it soon enough (that is, before I lose net access; if I don't get to it before then, then it won't happen till 2006).  No matter what, I won't be able to sr this for at least a week, which means things will be _really_ tight as far as getting it done in time...

If at all possible, I would recommend seeking another sr, preferably one familiar with IME or editor code (Simon Fraser, perhaps?)

Comment 5

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

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

Boris:

Thanks, I'm changing to Simon Fraser.

Comment 6

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

Simon:

This patch fixes bug 296686 too. That bug is very serious usability issue of Japanese EGBRIDGE users. Please sr it.

Comment 7

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

checked-in to Trunk, Thanks!

Comment 8

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

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

This bug dosen't have any regression reports. So this patch has low risk.

But the fixed problem is serious for Mac EGBRIDGE(IME of Ja) users.
# When the users hit space key between inputted characters, the next character of caret is deleted by this bug.

Let's go to 1.8.1 and 1.8.0.1.

Comment 9

[Daniel Veditz [:dveditz]]

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

Too late for 1.8.0.1, but thanks for flagging it so we can consider it for .0.2

Comment 10

[Daniel Veditz [:dveditz]]

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

approved for 1.8.0 branch, a=dveditz for drivers

Comment 11

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

-> v.