98434 - IME does not work correctly at the last characters in the text field

Status: VERIFIED FIXED

(Core :: Internationalization, defect, P2)

Description

[Teruko Kobayashi]

IME does not work correctly at the last characters in the text field.

Steps of reproduce
1. Go to above URL
2. Move the cursor to the 5th text field under Customer ID: 2667
   Maxlength of this field is 2 which means you can type 2 characters (even 
though single byte characters or double byte characters.) - see bug 18284.
3. Turn on IME (Make sure hiragana mode).
4. Type "ke" 
   Japanese hiragana "け" is displayed with underline (since this is 
unconverted, yet.)
5. Hit return to convert
6. Type "ke" again
   At this time, Japanese hiragana unconverted "け" with underline is not 
displayed.

It should be displayed "け". Once hit the return to convert, Japanese "け"
is displayed.

If you type "ke" and hit space key to select one of the chinese characters from 
candidate window, the chinese character sometime are not displayed after you 
committed in the last characters of the Max length field.

Tested 9-05 trunk Win32 and Linux build.

Comment 1

[Frank Tang]

looks like a P2

Comment 2

[Joe Francis]

I think this should be mine...
096

Comment 3

[Joe Francis]

098

Comment 4

[Joe Francis]

pushing off 098 to 099

Comment 5

[Joe Francis]

setting milestones based on reprioritization of buglist

Comment 6

[Roy Yokoyama]

*** Bug 109818 has been marked as a duplicate of this bug. ***

Comment 7

[Roy Yokoyama]

*** Bug 109263 has been marked as a duplicate of this bug. ***

Comment 8

[Kevin McCluskey (gone)]

Marking nsbeta1-. 

Comment 9

[Kevin McCluskey (gone)]

nsbeta1-

Comment 10

[Shotaro Kamio]

This bug is serious for Japanese environment (probably other languages also).

I wrote a patch.
I hope this bug will be fixed soon.

Comment 11

[Shotaro Kamio]

Attachment: patch

This patch has been tested on Linux and Win2k.
I know this patch is too rough and not enough modularized hack.
Please refine it.

Comment 12

[Joe Francis]

The trunk is the wave of the future!

Comment 13

[Joe Francis]

The days of having a half dozen milestones out in front of us to divide bugs 
between seem to be gone, though I dont know why.  Lumping everything together as 
far out as I can.  I'll pull back things that I am working on as I go.

Comment 14

[Frank Tang]

it seems IE allow two characters (in this case of sjis, 4bytes)
http://www.w3.org/TR/html4/index/attributes.html said it is "max chars for text
fields" 

what will we break without fixing this ?
remove nsbeta1- and change to nsbeta1 for m1.2final release

Comment 15

[Shotaro Kamio]

The main reason of this bug is nsTextEditRules::TruncateInsertionIfNeeded().
(see Bug 109818 also)
It truncates inputed string from IME according to MAXLENGTH.
We should do this truncation not when received converting string (underline
displayed), but only when received determined string (no-underline displayed).
Currently there is no way to decide if received string is determined one or not
in nsTextEditRules::TruncateInsertionIfNeeded().

Additionally, this method receives inputed string from IME not only a char you
typed, but all of converting string.
So, you must calculate for the truncation as follows:
  acceptable length = MAXLENGTH - ( docLength - selectionLength - (length of
previous converting string))
, where docLength means the length of strings the input field has (containing
previous converting string).
Current implementation lacks this minusing of converting string.

My patch fixes these problem.
It hacks nsEditor and adds a method. I think it is not modularized hack.

Comment 16

[Joe Francis]

Comment on attachment 89029 [details] [diff] [review]
patch

r=jfrancis.  thanks for working on this skamio.

Comment 17

[Joe Francis]

cc'ing kin for sr

Comment 18

[Joe Francis]

I want to think about this some more.  I believe there is a better way to tell
if we are editting uncommited IME.  I'll comment when I know more.

Comment 19

[Kevin McCluskey (gone)]

nsbeta1+

Comment 20

[Koike Kazuhiko]

Comment on attachment 89029 [details] [diff] [review]
patch

kin, please superreview the patch.

jfrancis, have you found the better way?

Comment 21

[kinmoz]

Comment on attachment 89029 [details] [diff] [review]
patch

I'm a bit backed up on review requests so I'd like to hear feedback from
jfrancis before doing a review.

Comment 22

[Joe Francis]

Kin, go ahead.  I don't realy understand the various types of IME Text Range
Lists, and I don't know if there is a more elegant way to detect the state they
need.

Comment 23

[kinmoz]

Comment on attachment 89029 [details] [diff] [review]
patch

==== Why do we need to add this to a public interface (nsIEditor.idl) if it's
only use is in the TextEditRules where |mEditor| is an nsPlaintextEditor which
has access to methods that are defined on our nsEditor implementation? If for
some other reason it has to  be in a public  interface, shouldn't it be in
nsIEditorIMESupport.idl?


+
+  /* -- Hack -- */
+  long getIMEBufferLength();


==== Can we keep |IsComposing()| and |SetIsComposing()| functions next to each
other in the .cpp file? According to the diff they will be roughly 1600 lines
apart.


==== This leaks since |Item()| addrefs the pointer it  returns.|rangePtr|
should be a COMPtr and |getter_AddRefs()| should be used.


+      result = mIMETextRangeList->Item(i, &rangePtr);


==== Shouldn't we be clearing |mIsComposing| in |EndComposition()| so that the
following will still work after previously using IME?


+  if ((-1 != aMaxLength) && (mFlags &
nsIPlaintextEditor::eEditorPlaintextMask) && !mEditor->IsComposing())

Comment 24

[Joe Francis]

Kin's last comment reminds me of what I was thinking about so long ago:

Why can't mIsComposing be driven entirles off of
StartComposition()/EndComposition()?

Comment 25

[Shotaro Kamio]

BeginComposition()/EndComposition() are triggered when IME is activated/deactivated.
However, Japanese IME has two states in activated mode.
We have no way to distinguish the states in current implementation.
One state is Composition mode. Another is Committed mode.
(I don't know suitable words to call them. See also comment#9 of bug 109818.)

It seems that we can distinguish them according to IME Text Range Lists.


> ==== Shouldn't we be clearing |mIsComposing| in |EndComposition()| so that the
> following will still work after previously using IME?

Although my patch will properly set mIsComposing before using it,
it should be cleared.

Thanks, kin. I will fix my patch.

Comment 26

[Shotaro Kamio]

Attachment: patch v2

I moved GetIMEBufferLength() and other methods into nsEditor class.
There is no needs to define them in public interface.
And I made some changes in names of methods and variables.
Please super-review.

Comment 27

[kinmoz]

Comment on attachment 107767 [details] [diff] [review]
patch v2

Ok, we're close ...


==== If we're not going to use the pre-existing |mIsComposing|, shouldn't it be
removed?


==== |SetIsIMEComposing()| returns an nsresult, but none of the return
statements return values. Should it be changed to have a void return value
since the callers never check it anyways?


==== Wow, the indentation of the IME related memebers is really messed up ...
not your fault though. If you feel like fixing the indentation as part of your
checkin, by all means go ahead. :-)


@@ -563,6 +566,8 @@
   nsCOMPtr<nsIDOMCharacterData> mIMETextNode;	      // current IME text node
   PRUint32						mIMETextOffset;      //
offset in text node where IME comp string begins
   PRUint32						mIMEBufferLength;    //
current length of IME comp string
+  PRBool	       mIsIMEComposing;     // is IME in composition state?
+					    // This is different from
mInIMEMode. see Bug 98434.

Comment 28

[Shotaro Kamio]

Attachment: patch v3

This will make us happy :->

Comment 29

[Teruko Kobayashi]

Changed QA contact to [e-mail].

Comment 30

[kinmoz]

Comment on attachment 108194 [details] [diff] [review]
patch v3

sr=[e-mail]

Comment 31

[Joe Francis]

fix landed on trunk

Comment 32

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

This problem is fixed with 2002122208-trunk/WinXP.
This problem is important bug for Japanese,
So it should be fixed on 1.0branch and Netscape7.

Comment 33

[Yuying Long]

Verified this specific problem was fixed in 12-26 trunk build on WinXP, WinME
and linux RH7.2.  However, the red underline doesn't display with un-commit
mode, open a new bug 186832 and mark this one as verified.

Please nominate as different keyword if want to check-in to other branch build.