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

VERIFIED FIXED in mozilla1.2beta

Status

()

P2
normal
VERIFIED FIXED
18 years ago
9 years ago

People

(Reporter: teruko, Assigned: mozeditor)

Tracking

({inputmethod, intl})

Trunk
mozilla1.2beta
inputmethod, intl
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixinhand, URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

18 years ago
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.
(Reporter)

Updated

18 years ago
Keywords: intl
QA Contact: andreasb → teruko

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 1

18 years ago
looks like a P2
Priority: -- → P2
(Assignee)

Comment 2

18 years ago
I think this should be mine...
096
Assignee: yokoyama → jfrancis
Status: ASSIGNED → NEW
Target Milestone: --- → mozilla0.9.6
(Assignee)

Updated

18 years ago
Target Milestone: mozilla0.9.6 → mozilla0.9.7
(Reporter)

Updated

17 years ago
Keywords: nsbeta1
(Assignee)

Comment 3

17 years ago
098
Target Milestone: mozilla0.9.7 → mozilla0.9.8
(Assignee)

Comment 4

17 years ago
pushing off 098 to 099
Target Milestone: mozilla0.9.8 → mozilla0.9.9
(Assignee)

Comment 5

17 years ago
setting milestones based on reprioritization of buglist
Target Milestone: mozilla0.9.9 → mozilla1.0.1

Comment 6

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

Comment 7

17 years ago
*** Bug 109263 has been marked as a duplicate of this bug. ***
Marking nsbeta1-. 
nsbeta1-
Keywords: nsbeta1 → nsbeta1-

Comment 10

17 years ago
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

17 years ago
Created attachment 89029 [details] [diff] [review]
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.
(Assignee)

Comment 12

17 years ago
The trunk is the wave of the future!
Target Milestone: mozilla1.0.1 → mozilla1.1beta

Updated

17 years ago
Blocks: 157673
(Assignee)

Comment 13

17 years ago
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

Comment 14

17 years ago
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
Keywords: nsbeta1- → nsbeta1

Comment 15

17 years ago
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.
(Assignee)

Comment 16

17 years ago
Comment on attachment 89029 [details] [diff] [review]
patch

r=jfrancis.  thanks for working on this skamio.
Attachment #89029 - Flags: review+
(Assignee)

Comment 17

17 years ago
cc'ing kin for sr
Status: NEW → ASSIGNED
(Assignee)

Comment 18

17 years ago
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.
nsbeta1+
Keywords: nsbeta1 → nsbeta1+

Comment 20

17 years ago
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 21

17 years ago
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.
(Assignee)

Comment 22

17 years ago
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.
(Assignee)

Updated

17 years ago
Whiteboard: fixinhand

Comment 23

17 years ago
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-
(Assignee)

Comment 24

17 years ago
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

17 years ago
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

16 years ago
Created attachment 107767 [details] [diff] [review]
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.

Updated

16 years ago
Attachment #89029 - Attachment is obsolete: true

Updated

16 years ago
Attachment #107767 - Flags: superreview?(kin)

Comment 27

16 years ago
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.
Attachment #107767 - Flags: superreview?(kin)
Attachment #107767 - Flags: superreview-
Attachment #107767 - Flags: review?(jfrancis)

Comment 28

16 years ago
Created attachment 108194 [details] [diff] [review]
patch v3

This will make us happy :->
Attachment #107767 - Attachment is obsolete: true

Updated

16 years ago
Attachment #108194 - Flags: superreview?(kin)

Updated

16 years ago
Attachment #107767 - Flags: review?(jfrancis)
(Reporter)

Comment 29

16 years ago
Changed QA contact to kasumi@netscape.com.
QA Contact: teruko → kasumi

Comment 30

16 years ago
Comment on attachment 108194 [details] [diff] [review]
patch v3

sr=kin@netscape.com
Attachment #108194 - Flags: superreview?(kin) → superreview+

Updated

16 years ago
Attachment #108194 - Flags: review?(jfrancis)
(Assignee)

Updated

16 years ago
Attachment #108194 - Flags: review?(jfrancis) → review+
(Assignee)

Comment 31

16 years ago
fix landed on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 16 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.

Updated

16 years ago
Attachment #108194 - Flags: approval1.0.x?

Comment 33

16 years ago
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

Updated

16 years ago
No longer blocks: 157673

Updated

16 years ago
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.