Closed
Bug 90583
Opened 24 years ago
Closed 23 years ago
[WinIME Simp. Chinese] The candidate window covered the current input line
Categories
(Core :: Internationalization, defect, P3)
Tracking
()
VERIFIED
FIXED
mozilla1.0
People
(Reporter: amyy, Assigned: ftang)
Details
(Keywords: intl, Whiteboard: need help)
Attachments
(3 files, 3 obsolete files)
37.60 KB,
image/jpeg
|
Details | |
31.85 KB,
image/jpeg
|
Details | |
4.12 KB,
patch
|
mozeditor
:
review+
kinmoz
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•24 years ago
|
||
Reporter | ||
Updated•24 years ago
|
QA Contact: andreasb → ylong
Reporter | ||
Comment 2•24 years ago
|
||
Comment 3•24 years ago
|
||
Reassign to yokoyama, cc to shanjian. Shanjian, is this related to the IME positioning change you worked before?
Assignee: nhotta → yokoyama
Comment 5•24 years ago
|
||
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.
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•23 years ago
|
||
reassign back to yokoyama and target m0.9.4
Assignee: shanjian → yokoyama
Status: ASSIGNED → NEW
Target Milestone: --- → mozilla0.9.4
Updated•23 years ago
|
Status: NEW → ASSIGNED
Updated•23 years ago
|
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Comment 9•23 years ago
|
||
Posted a help for this bug in i18n_moz newsgroup. No response so far. Any help would be appreciated.
Whiteboard: need help
Target Milestone: mozilla0.9.6 → mozilla0.9.9
Reporter | ||
Comment 10•23 years ago
|
||
Still reproduce it on WinXP-Simp. Chinese on both N6.2.1 and 02-14 trunk build, both Composer and Mail. -> nsbeta1
Keywords: nsbeta1
Assignee | ||
Comment 13•23 years ago
|
||
ok, I find the problem, we set the wrong value to rcArea when we call SETCANDIDATE
Assignee | ||
Comment 14•23 years ago
|
||
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.
Assignee | ||
Comment 15•23 years ago
|
||
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.
Assignee | ||
Comment 16•23 years ago
|
||
Assignee | ||
Comment 17•23 years ago
|
||
Comment on attachment 69710 [details] [diff] [review] patch v1 sorry, there are error.
Attachment #69710 -
Attachment is obsolete: true
Assignee | ||
Comment 18•23 years ago
|
||
Comment 19•23 years ago
|
||
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.
Attachment #69719 -
Flags: review+
Assignee | ||
Comment 20•23 years ago
|
||
reassign to ftang
Assignee: yokoyama → ftang
Status: ASSIGNED → NEW
Comment 21•23 years ago
|
||
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?
Assignee | ||
Comment 22•23 years ago
|
||
>do you want to put an NS_ASSERTION or NS_WARNING in that else { } so we at
no, it is too annoying.
Assignee | ||
Comment 23•23 years ago
|
||
fixed and check in .
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 24•23 years ago
|
||
ylong: when we test our patch, I saw no changes to the behaviour of Simplified Chinese IME. Can you verify the bug?
Reporter | ||
Comment 25•23 years ago
|
||
Still not see today's trunk build through sweetlou, will try tomorrow.
Reporter | ||
Comment 26•23 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 27•23 years ago
|
||
ok, i will try to handle the rest of the issue in m1.0 time
Status: REOPENED → ASSIGNED
Target Milestone: mozilla0.9.9 → mozilla1.0
Assignee | ||
Comment 29•23 years ago
|
||
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.
Assignee | ||
Comment 30•23 years ago
|
||
Assignee | ||
Comment 31•23 years ago
|
||
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.
Assignee | ||
Comment 32•23 years ago
|
||
katakai- could you verify this patch make no harm on Linux/Solaris IME ? Thanks
Assignee | ||
Comment 33•23 years ago
|
||
Comment on attachment 72405 [details] [diff] [review] the real patch in editor code solve the real problem. r=mjudge in my cube
Attachment #72405 -
Flags: review+
Assignee | ||
Updated•23 years ago
|
Comment 34•23 years ago
|
||
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.
Attachment #72405 -
Flags: superreview+
Comment 35•23 years ago
|
||
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?
Attachment #69719 -
Flags: superreview+
Assignee | ||
Comment 36•23 years ago
|
||
>(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
Assignee | ||
Comment 37•23 years ago
|
||
>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.
Assignee | ||
Updated•23 years ago
|
Attachment #69719 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #72405 -
Attachment is obsolete: true
Assignee | ||
Comment 39•23 years ago
|
||
Comment 41•23 years ago
|
||
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"
Attachment #72644 -
Flags: superreview+
Assignee | ||
Updated•23 years ago
|
Comment 42•23 years ago
|
||
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
Attachment #72644 -
Flags: approval+
Comment 43•23 years ago
|
||
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
Attachment #72644 -
Flags: review+
Assignee | ||
Comment 45•23 years ago
|
||
fixed and check in the v3 patch
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 46•23 years ago
|
||
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.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•