Closed Bug 98434 Opened 20 years ago Closed 19 years ago

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


(Core :: Internationalization, defect, P2)






(Reporter: teruko, Assigned: mozeditor)




(Keywords: inputmethod, intl, Whiteboard: fixinhand)


(1 file, 2 obsolete files)

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 

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.
Keywords: intl
QA Contact: andreasb → teruko
looks like a P2
Priority: -- → P2
I think this should be mine...
Assignee: yokoyama → jfrancis
Target Milestone: --- → mozilla0.9.6
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Keywords: nsbeta1
Target Milestone: mozilla0.9.7 → mozilla0.9.8
pushing off 098 to 099
Target Milestone: mozilla0.9.8 → mozilla0.9.9
setting milestones based on reprioritization of buglist
Target Milestone: mozilla0.9.9 → mozilla1.0.1
*** Bug 109818 has been marked as a duplicate of this bug. ***
*** Bug 109263 has been marked as a duplicate of this bug. ***
Marking nsbeta1-. 
Keywords: nsbeta1nsbeta1-
This bug is serious for Japanese environment (probably other languages also).

I wrote a patch.
I hope this bug will be fixed soon.
Attached patch patch (obsolete) — Splinter Review
This patch has been tested on Linux and Win2k.
I know this patch is too rough and not enough modularized hack.
Please refine it.
The trunk is the wave of the future!
Target Milestone: mozilla1.0.1 → mozilla1.1beta
Blocks: 157673
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.
Target Milestone: mozilla1.1beta → mozilla1.2beta
it seems IE allow two characters (in this case of sjis, 4bytes) said it is "max chars for text

what will we break without fixing this ?
remove nsbeta1- and change to nsbeta1 for m1.2final release
Keywords: nsbeta1-nsbeta1
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 on attachment 89029 [details] [diff] [review]

r=jfrancis.  thanks for working on this skamio.
Attachment #89029 - Flags: review+
cc'ing kin for sr
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.
Keywords: nsbeta1nsbeta1+
Comment on attachment 89029 [details] [diff] [review]

kin, please superreview the patch.

jfrancis, have you found the better way?
Attachment #89029 - Flags: superreview?(kin)
Comment on attachment 89029 [details] [diff] [review]

I'm a bit backed up on review requests so I'd like to hear feedback from
jfrancis before doing a review.
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
Whiteboard: fixinhand
Comment on attachment 89029 [details] [diff] [review]

==== 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

+  /* -- 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

==== 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())
Attachment #89029 - Flags: superreview?(kin) → superreview-
Kin's last comment reminds me of what I was thinking about so long ago:

Why can't mIsComposing be driven entirles off of
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.
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #89029 - Attachment is obsolete: true
Attachment #107767 - Flags: superreview?(kin)
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

==== |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.
Attachment #107767 - Flags: superreview?(kin)
Attachment #107767 - Flags: superreview-
Attachment #107767 - Flags: review?(jfrancis)
Attached patch patch v3Splinter Review
This will make us happy :->
Attachment #107767 - Attachment is obsolete: true
Attachment #108194 - Flags: superreview?(kin)
Attachment #107767 - Flags: review?(jfrancis)
Changed QA contact to
QA Contact: teruko → kasumi
Comment on attachment 108194 [details] [diff] [review]
patch v3
Attachment #108194 - Flags: superreview?(kin) → superreview+
Attachment #108194 - Flags: review?(jfrancis)
Attachment #108194 - Flags: review?(jfrancis) → review+
fix landed on trunk
Closed: 19 years ago
Resolution: --- → FIXED
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.
Attachment #108194 - Flags: approval1.0.x?
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.

No longer blocks: 157673
Depends on: 180372
Attachment #108194 - Flags: approval1.0.x?
You need to log in before you can comment on or make changes to this bug.