Closed Bug 90583 Opened 23 years ago Closed 22 years ago

[WinIME Simp. Chinese] The candidate window covered the current input line

Categories

(Core :: Internationalization, defect, P3)

x86
Windows 2000
defect

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: amyy, Assigned: ftang)

Details

(Keywords: intl, Whiteboard: need help)

Attachments

(3 files, 3 obsolete files)

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.
QA Contact: andreasb → ylong
Reassign to yokoyama, cc to shanjian.
Shanjian, is this related to the IME positioning change you worked before?
Assignee: nhotta → yokoyama
Reassign to shanjian
Assignee: yokoyama → 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. 
Status: NEW → ASSIGNED
reassign back to yokoyama and target m0.9.4
Assignee: shanjian → yokoyama
Status: ASSIGNED → NEW
Target Milestone: --- → mozilla0.9.4
Keywords: intl
changing to m0.9.5
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.5 → mozilla0.9.6
P3 IME bug
Priority: -- → P3
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
Still reproduce it on WinXP-Simp. Chinese on both N6.2.1 and 02-14 trunk build,
both Composer and Mail.

-> nsbeta1
Keywords: 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.
Attached patch patch v1 (obsolete) — Splinter Review
Comment on attachment 69710 [details] [diff] [review]
patch v1

sorry, there are error.
Attachment #69710 - Attachment is obsolete: true
Attached patch patch v2 (obsolete) — Splinter Review
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+
reassign to ftang
Assignee: yokoyama → ftang
Status: ASSIGNED → NEW
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 .
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
nsbeta1+ per i18n triage
Keywords: nsbeta1nsbeta1+
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
Blocks: 104056
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+
Blocks: 104148
No longer blocks: 104056
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 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+
>(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.
Attachment #69719 - Attachment is obsolete: true
Attachment #72405 - Attachment is obsolete: true
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"
Attachment #72644 - Flags: superreview+
Blocks: 104060
No longer blocks: 104148
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 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+
fixed and check in.
fixed and check in the v3 patch
Status: ASSIGNED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
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
No longer blocks: 104060
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: