Build: 07-10 branch build on Win2k-CN Steps to reproduce: 1. Add a few Simp. Chinese IME from Control Panel | Keyboard. 2. Launch browser and open Composer. 3. Turn on the Simp. Chinese IME Result: The candidate window covered the current input line position, it's really hard to know what you are typing unless you commit the string. (see attached image) Traditional Chinese IME has a little similar result, but not bad as Simp. Chinese though. There was a very similar bug about Japanese before - bug 59924, which works fine with Japanese now.

Reassign to yokoyama, cc to shanjian. Shanjian, is this related to the IME positioning change you worked before?

Reassign to shanjian

I tried global IME on NT, and it works fine. I guess some IME implementation (like the one use in this case) is not implemented correctly in positioning candidate window. I'll see what I can do when I reproduce the problem.

reassign back to yokoyama and target m0.9.4

changing to m0.9.5

P3 IME bug

Posted a help for this bug in i18n_moz newsgroup. No response so far. Any help would be appreciated.

Still reproduce it on WinXP-Simp. Chinese on both N6.2.1 and 02-14 trunk build, both Composer and Mail. -> nsbeta1

Yuying, what Simplified Chinese IMEs did you use?

Anything other than MS IME: Zhineng ABC, ZhengMa, Shuangpin

ok, I find the problem, we set the wrong value to rcArea when we call SETCANDIDATE

some how the height of the mCursorPosition is 0 when we are using chiense ime but not when we use jap ime, if we click on other window and come back, sometimes it will be correct value.

Ok, the real problem is nsCaret somehow sometimes return error but we ignore those erorr and still use the result to set the candidate window position. I have not figure out why the nsCaret code failed, but I know it failed after it call err = frameSelection->GetFrameForNodeOffset(contentNode, focusOffset, hint, &theFrame, &theFrameOffset); What we can do now is in the nsWindow.cpp to check the return result, only set the candidate window position if the result is valid.

Comment on attachment 69710 [details] [diff] [review] patch v1 sorry, there are error.

Comment on attachment 69719 [details] [diff] [review] patch v2 /r=yokoyama this patch made the Chinese-traditional IME to behave nicer. We may want to keep this bug open for Simplified Chinese.

reassign to ftang

Comment on attachment 69719 [details] [diff] [review] patch v2 sr=alecf do you want to put an NS_ASSERTION or NS_WARNING in that else { } so we at least know that the invalid result happened?

>do you want to put an NS_ASSERTION or NS_WARNING in that else { } so we at no, it is too annoying.

fixed and check in .

ylong: when we test our patch, I saw no changes to the behaviour of Simplified Chinese IME. Can you verify the bug?

Still not see today's trunk build through sweetlou, will try tomorrow.

Check it on 02-20 trunk build/WinXP-CN, the problem got fixed on some places, but still has left over: With IME "QuanPin", "ShuangPin", "ZhengMa": Type something , e.g. "ai", the candidate panel will appear in good position (we fixed here), then you might find that is not what you want, press backspace to delete "i". Result: the candidate window will still cover the input line. Re-open it.

ok, i will try to handle the rest of the issue in m1.0 time

nsbeta1+ per i18n triage

ok, I find out what we failed in the GetCaretCoordinates. the GetCaretCoordinates are called in 4 places, 3 of them are related to ime. in the nsEditor.cpp, it is called when the editor receive a query composition message, which is ok. However, in the nsHTMLEditor/nsPlaintextEditor::SetCompositionString, the GetCaretCoordinates are called right we set the new content into the content model and before the reflow, and therefore the nsTextFrame::GetChildFrameContainingOffset always return NS_ERROR_FAILURE; since the if (contentOffset != mContentLength) //that condition was only for when there is a choice is true. and after it return the nsAutoPlaceHolderBatch is out of its life time in nsHTMLEditor/nsPlaintextEditor::SetComposition and therefore the nsAutoPlaceHolderBatch::~nsAutoPlaceHolderBatch is called and trigger the reflow and make the frame model sync with the content model. However, it is already too late for the caret becuase we already failed. Therefore, it is easy to fix it, just make sure we call the GetCaretCoordinates AFTER the reflow happen. Just make sure GetCaretCoordinates outside the block of nsAutoPlaceHolderBatch object. I will attach a patch. the patch add { before nsAutoPlaceHolderBatch and } before the call to GetCaretCoordinates . I also indent it to the new block. I seperate the return into a seperate line from the if statement so it is easier to set break point. And I check the return value and assert.

add shanjian, mjudge, jfrancis , sfraser to the cc list mjudge/jfrancis/sfraser : could you please start r= this one ? I will test this patch on Mac and Linux in the mean time.

katakai- could you verify this patch make no harm on Linux/Solaris IME ? Thanks

Comment on attachment 72405 [details] [diff] [review] the real patch in editor code solve the real problem. r=mjudge in my cube

Comment on attachment 72405 [details] [diff] [review] the real patch in editor code solve the real problem. + // we need the nsAutoPlaceHolderBatch destroctor called before hitting + // GetCaretCoordinates so the states in Frame system sync with content + // therefore, we put the nsAutoPlaceHolderBatch into a inner block Move the comment above your new scoping in both files. Fix spelling for "destructor" + NS_ASSERTION(NS_SUCCEEDED(result), "cannot get carret"); Fix "caret" spelling ... this should probably say "caret position" like the assertion you added to nsPlaintextEditor::SetCompositionString() // second part of 23558 fix: if (aCompositionString.IsEmpty()) To satisfy my paranoia, can we move the |if| block that nulls out mIMETextNode, into your scoping block so that the only thing that happens after your block is the GetCaretCoordinates() call? If I recall correctly, some of the calls the nsAutoPlaceHolderBatch() destructor triggers, checks some IME members. Also, you do realize that these nsAutoPlaceHolderBatch stack vars can nest during function calls, so that if we ever move SetCompositionString() into a method that does use one, the scoping you added here will have no effect. We are fine for now though since I believe the call to SetCompositionString() happens only from the event handlers. I just wanted you to be aware. sr=kin@netscape.com with those changes to both files.

Comment on attachment 69719 [details] [diff] [review] patch v2 sr=kin@netscape.com Do we need an NS_WARNING() or NS_ASSERTION() to flag the case when we have zero width or height? Do you need an r= from a widget module owner?

>(From update of attachment 69719 [details] [diff] [review]) >sr=kin@netscape.com kin: attachment for widget directory have already check in for weeks. I am seeking sr= for 72405 only

>Also, you do realize that these nsAutoPlaceHolderBatch stack vars can nest during >function calls, so that if we ever move SetCompositionString() into a method that >does use one, the scoping you added here will have no effect. We are fine for now >though since I believe the call to SetCompositionString() happens only from the >event handlers. I just wanted you to be aware. thank you to tell me this. I don't know that. But this should be fine. As long as the last one return the caret position correctly, the ime candidate window should be put in the right position.

I'm not seeing any problem by this changes on Linux.

test the newest patch on both window and mac with no problem.

Comment on attachment 72644 [details] [diff] [review] v3 according to kin's suggestion sr=kin@netscape.com no need to generate another diff if you just take care of the following: Index: text/nsPlaintextEditor.cpp =================================================================== + // we need nsAutoPlaceHolderBatch destructor called before hitting + // the GetCaretCoordinate so the states in content and frame sync + // therefore, we put the nsAutoPlaceHolderBatch into a inner block Replace this comment with the one you used in the nsHTMLEditor.cpp, it just sounds better. Make sure you replace the "destroctor" typo, in the nsHTMLEditor.cpp version, with "destructor" in both files. Index: html/nsHTMLEditor.cpp =================================================================== + NS_ASSERTION(NS_SUCCEEDED(result), "cannot get caret"); Replace the assertion text with the one you used in nsPlaintextEditor.cpp: "cannot get caret position"

Comment on attachment 72644 [details] [diff] [review] v3 according to kin's suggestion a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment on attachment 72644 [details] [diff] [review] v3 according to kin's suggestion These changes should be fine from ana editting point of view. r=jfrancis

fixed and check in.

fixed and check in the v3 patch

Fixed verified(ZhengMa, ShuanhPin, NeiMa...etc.) on 03-08 trunk build, but there still a problem on intelligent ABC, filed bug 129769 for that, mark this one as verified.