168281 - type ahead does not work with IME

Status: VERIFIED FIXED

(SeaMonkey :: Find In Page, defect)

Description

[Guangyan Liu]

Mozilla build 2002-09-09-08-trunk

When I try to enter some Chinese characters using China (Taiwan) keyboard layout
on the Yahoo Taiwan homepage (http://tw.yahoo.com/), nothing happens. I do not
get any kind of error messages in the status bar saying word not found, etc.
It's as if I never typed anything. 

In contrast, I get visual feedback (characters are displayed on
the top left corner of the page) when I search for Simplified Chinese links with
the Chinese (China) keyboard layout.

An interesting problem I noticed was that although nothing seems to happen when
I enter some characters, if I then click in a textfield and attempt to enter
more characters inside that field the characters I entered previously suddenly
appear inside the field. Delete does not work in the textfield as expected after
the reappearance, as it somehow introduces more characters into the textfield.

Comment 1

[Aaron Leventhal]

I would really love it if someone from Sun's China Accessibilty Task Force could
investigate and help us see what's going on in the code. My Chinese is a little
rusty.

Yao Boo Yao Ma?

Comment 2

[Aaron Leventhal]

If we fix this it will probably also fix bug 168286.

Comment 3

[Kyle Yuan]

aaron, your chinese is really funny :)) If you want to visit us, we can teach
you more funny chinese.
BTW, "Yao Boo Yao Ma?" is kind of girly, "Hao Boo Hao?" is better :)

Comment 4

[Aaron Leventhal]

Oh no! No wonder it's girly, I learned it from my old girlfriend.

Comment 5

[Kyle Yuan]

Attachment: a primitive patch

a primitive patch based on my investigation.

Aaron, I'm sorry, I have no time to finish this work in Sept. But I think I've
found the right approach to solve this problem. I wrote a to-do comment in my
code, so someone can continue this work.

I'll ask someone else in ourteam to finish this work.

Comment 6

[Kyle Yuan]

*** Bug 168289 has been marked as a duplicate of this bug. ***

Comment 7

[Kyle Yuan]

*** Bug 168286 has been marked as a duplicate of this bug. ***

Comment 8

[Aaron Leventhal]

Kyle,

This is a great start, thanks! A couple of questions:

Why these changes? popuhidden, etc. are not from nsIDOMKeyListener
   chromeEventHandler->RemoveEventListener(NS_LITERAL_STRING("popuphidden"), 
-                                          NS_STATIC_CAST(nsIDOMEventListener*,
this), 
+                                          NS_STATIC_CAST(nsIDOMKeyListener*,
this), 
                                           PR_TRUE);

+  // To-Do:
+  // everything is ok, then need to pass the composedText to the search engine
+  //  (I guess it would be in KeyPress()), 
This makes sense to me. Would these text events encompass things beyond IME,
such as text pasted into document? I guess I'll have to take a look. Maybe
someone can show me how to run IME on my system.

+ then get the caret object, call
+  //  caret->GetCaretCoordinates(nsICaret::eIMECoordinates, selection,
+  //      &(textEventReply->mCursorPosition),
&(textEventReply->mCursorIsCollapsed), nsnull);
+  //  to give the feedback to the IME stuff
Why does the IME have a separate selection?

Comment 9

[Aaron Leventhal]

On second thought, we don't want to send things into KeyPress().
We'll need to create a helper method called something like
AppendCharacter(PRUnichar aCharacter)

Then both KeyPress() and HandleEvent() can use AppendCharacter to do their work.

Comment 10

[Aaron Leventhal]

Sorry for my ignorance, how do I make IME work? Do I need to install another
language? Or is there something for programmers who want to test using a
European character set? 

Comment 11

[Kyle Yuan]

> Why these changes? popuhidden, etc. are not from nsIDOMKeyListener ...
Because I got a compiler error for ambiguous conversion from |this| to
nsIDOMEventListener. Now, |this| inherited from both nsIDOMKeyListener and
nsIDOMTextListener, so there are 2 paths to convert |this| to
nsIDOMEventListener. But my change is temporary, there should be another better
approach.

> Would these text events encompass things beyond IME, 
> such as text pasted into document?
I dont think so, but I could be wrong.

> Maybe someone can show me how to run IME on my system.
You can go Control Panel, open "Regional Options", check some languages, such as
Chinese (Simplified) to install. Then you may have some IME for such language.
Use <ctrl>+<space> to turn on IME and <ctrl>+<shift> to switch between different
IMEs. 

> Why does the IME have a separate selection?
I just copied those code from nsPlaintextEditor::SetCompositionString, it is
used to set the textEventReply->mCursorPosition and
textEventReply->mCursorIsCollapsed. I'm not sure how to make a selection here :(

We absolutely should ask kin who is the author of
nsPlaintextEditor::SetCompositionString

Comment 12

[Aaron Leventhal]

Guangyan -

I'm curious if the strange problems you mention have anything to do with
typeaheadfind.

> An interesting problem I noticed was that although nothing seems to happen when
> I enter some characters, if I then click in a textfield and attempt to enter
> more characters inside that field the characters I entered previously suddenly
> appear inside the field. Delete does not work in the textfield as expected 
> after the reappearance, as it somehow introduces more characters into the 
> textfield

If you delete all the typeahead files from your install (*typeahead*) does the
problem still occur?

Comment 13

[Kyle Yuan]

> We'll need to create a helper method called something like
> AppendCharacter(PRUnichar aCharacter)
|PRUnichar aCharacter| is wrong, we can input a bunch of characters at once by 
using IME. so it should be nsAString.
aaron, if you can complete this function, I or Simford can do the other minor 
works or test.

> This makes sense to me. Would these text events encompass things beyond IME,
> such as text pasted into document? 
I tested, paste text will not emit HandleText event.

Comment 14

[kinmoz]

Cc jfrancis for editor related IME questions.

Comment 15

[Guangyan Liu]

Aaron, in response to your question in comment #12, I removed typeahead and
confirmed that the problem still exists. Therefore, it is not caused by typeahead. 

Comment 16

[Aaron Leventhal]

Guangyan -- let me know when you file a bug for the other problem. Perhaps it
should be marked as a blocker to this one.

Comment 17

[Aaron Leventhal]

Kyle, sounds good -- I need to get bug 167921 in first - it changes so many
things that it would be hard to work on this until I get done with that.

I still plan on doing AppendCharacter(), and sending the dom string 1 character
at a time to it. That way AppendCharacter() can also be used by KeyPress(). I'd
rather do that than turn the unichar KeyPress has into a string.

Comment 18

[Guangyan Liu]

Aaron, I have filed another IME bug. It's 169076 and I will mark it as a blocker
to this bug.  

Comment 19

[Aaron Leventhal]

Attachment: Not finished, but it might partially work

Kyle, does this work at all? 
If it works I will clean it up for review.

Comment 20

[Kyle Yuan]

Yes, it *partially* works if get rid of this line:
textEvent->GetInputRange(getter_AddRefs(textRangeList));
(it will cause mozilla crash, and actually unused anywhere)

I have 3 chinese IME, this patch can work with one of them, that has its own 
candidate window. Another two will use the target place, such as a input box, 
as its candidate window, so you have to deal with this situation.

Comment 21

[Aaron Leventhal]

Does that mean it works either way, but it just doesn't have a clean area to
type if the IME program doesn't provide one?

Comment 22

[Aaron Leventhal]

Attachment: Uses HandleText to get IME input, sends characters 1 at a time to new method called HandleKey()

Seeking r=

There is still a problem that a blank typing area is sometimes not available,
with some IME's, because they expect the blank area to be drawn by Mozilla.
That will have to be a separate bug.

Comment 23

[Aaron Leventhal]

Taking bug

Comment 24

[Frank Tang]

yokoyama, can you (or shanjian) r= this patch?
Talk to nhotta and gary liu about what is broken.

Comment 25

[Roy Yokoyama]

I talked to Aaron few minutes ago and he would like me to take a look
at the patch and see if we see problem with other IMEs.

Comment 26

[Kyle Yuan]

aaron, I don't think this patch is ready for review, because the HandleText() 
is lame. Some IME will send *every* char to HandleText() when typing, and the 
user is able to choose another word from candidates till he/she hit the 
<enter>. So you must not send the char to HandleKey(), just display them 
somewhere, such as the status bar, do the finding work only when received the 
end_composition event (I can't figure out the event name exactly yet).

The only IME that can work with your patch is not a popular one. You should let 
MS PinYin happy first :)

I'll test this on Solaris later and tell you the result.

Comment 27

[Kyle Yuan]

Same problem on Solaris. nsTypeAheadFind should be a nsIDOMCompositionListener 
too.

I'm going to update my local tree and make a new patch.

Comment 28

[Kyle Yuan]

Attachment: Add support for nsIDOMCompositionListener

this one works better - it can work with all my IME. I show the candidate
char/word in HandleText() and do finding in HandleEndComposition().

aaron, this patch is just for demo, still need more polish.

Comment 29

[Kyle Yuan]

Attachment: Add support for nsIDOMCompositionListener

sorry, my previous patch is a garbage!

Comment 30

[Aaron Leventhal]

Kyle, 

What still needs to be cleaned up. Do you want me to do something now?

In
HandleEndComposition(nsIDOMEvent* aCompositionEvent)
should we do mIMEString.Truncate() at the end, in case HandleEndComposition()
just to be safe?

Also, should we move the caret stuff from HandleText to HandleEndComposition?

Comment 31

[Roy Yokoyama]

Aaron:  I promised you to try this patch on my machine; but I am littel tied up
        other project.  I hope I can get to this soon.  Just to let you know :)

Comment 32

[Aaron Leventhal]

That's okay, Kyle and I are still working on it. One of us will put up a final
patch, and the other will r= it. Once it's in we can use the i18n team to
QA/verify it?

I'm just glad to give you and Brian the demo yesterday, so you know what's
coming soon.

Comment 33

[Guangyan Liu]

Type ahead doesn't work with IME in Mac OS X either as tested on 2002092408
mozilla trunk build.

Comment 34

[Kyle Yuan]

1) DisplayStatus() I just append an argument into this function, is there some 
better way to do this?

2) +  DisplayStatus(PR_FALSE, nsnull, PR_FALSE, mIMEString.get()); I just show 
the candidate here, Do we need something more, like a prompt and show 
mTypeAheadBuffer + mIMEString in case of the mixing input?

3) doing mIMEString.Truncate() at the end of HandleEndComposition() is a good 
idea. I remember that sometimes HandleEndComposition() will be called twice, 
but I'm not sure. We should leave the caret stuff in HandleText(), because the 
IME candidate window will disappear once HandleEndComposition() get called, we 
don't need to give feedback to IME any more.

Comment 35

[Aaron Leventhal]

Let's leave the DisplayStatus() stuff the way it is for now. I'll work with the
IME team to see what the long-term plan for the status line should be.

If you put the mIMEString.Truncate() in a new patch, I'll give r=

Comment 36

[Kyle Yuan]

Attachment: added mIMEString.Truncate() in HandleEndComposition per aaron's suggestion

ready for r= now.

Comment 37

[Aaron Leventhal]

Attachment: Reorganized typeaheadfind into smaller, more logical pieces, as per Kyle's suggestion

The new code makes a lot more sense.
It uses methods like KeyPress(), HandleChar(), HandleBackspace(), SaveFind()
etc. to split things up into more digestable pieces.

I also made it so that the first IME char that isn't found stops the IME
typeaheadfind, so that it doesn't keep going when it obviously cannot.

Seeking r=kyle

Comment 38

[Kyle Yuan]

Comment on attachment 100935 [details] [diff] [review]
Reorganized typeaheadfind into smaller, more logical pieces,  as per Kyle's suggestion

r=kyle

Comment 39

[Aaron Leventhal]

Seeking sr=!

Comment 40

[Kyle Yuan]

Aaron, your patch also fixed some other problems beyond this IME supporting 
bug, such as backspace and repeatmode enhancement. Could you say something 
about what that problem exactly is?

one comment for the patch:
should we merge the GetNodeIfTypeAheadOkay() and GetShellIfTypeAheadOkay() into 
one function, because we always use them together?

Comment 41

[Aaron Leventhal]

Attachment: Uses Kyle's suggestion: merges GetNodeIfTypeAheadOkay + GetShellIfTypeAheadOkay -> GetTargetIfTypeAheadOkay

Kyle, as I was cleaning moving the backspace code into it's own method, I
decided to deal with some cases that we ignored - what backspace and find
next/prev do in repeated char find.

Comment 42

[Kyle Yuan]

Comment on attachment 101080 [details] [diff] [review]
Uses Kyle's suggestion: merges GetNodeIfTypeAheadOkay + GetShellIfTypeAheadOkay  -> GetTargetIfTypeAheadOkay

r=kyle. looks good to me.

Comment 43

[Alec Flett]

Comment on attachment 101080 [details] [diff] [review]
Uses Kyle's suggestion: merges GetNodeIfTypeAheadOkay + GetShellIfTypeAheadOkay  -> GetTargetIfTypeAheadOkay

strange spacing here:
   browserChrome->SetStatus(nsIWebBrowserChrome::STATUS_LINK,
-			     PromiseFlatString(statusString).get());
+			   PromiseFlatString(statusString).get());

other than that sr=alecf

Comment 44

[Aaron Leventhal]

checked in

Comment 45

[Rui Xu]

Verified with trunk.