Closed Bug 98434 Opened 19 years ago Closed 18 years ago
IME does not work correctly at the last characters in the text field
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.
looks like a P2
Priority: -- → P2
I think this should be mine... 096
Assignee: yokoyama → jfrancis
Status: ASSIGNED → NEW
Target Milestone: --- → mozilla0.9.6
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. ***
This bug is serious for Japanese environment (probably other languages also). I wrote a patch. I hope this bug will be fixed soon.
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
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) 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
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] patch r=jfrancis. thanks for working on this skamio.
Attachment #89029 - Flags: review+
cc'ing kin for sr
Status: NEW → ASSIGNED
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 on attachment 89029 [details] [diff] [review] patch kin, please superreview the patch. jfrancis, have you found the better way?
Attachment #89029 - Flags: superreview?(kin)
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.
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 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())
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 StartComposition()/EndComposition()?
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.
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 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.
This will make us happy :->
Attachment #107767 - Attachment is obsolete: true
Changed QA contact to email@example.com.
QA Contact: teruko → kasumi
Comment on attachment 108194 [details] [diff] [review] patch v3 firstname.lastname@example.org
Attachment #108194 - Flags: superreview?(kin) → superreview+
fix landed on trunk
Status: ASSIGNED → RESOLVED
Closed: 18 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.
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.