Closed Bug 271815 Opened 20 years ago Closed 18 years ago

GTK2 IM over-the-spot doesn't work with Chinese IM because the editor doesn't return correct caret position

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: cp76, Assigned: masayuki)

References

Details

(4 keywords)

Attachments

(2 files, 6 obsolete files)

Patch rv1.2
(deleted), patch
masayuki
: review+
masayuki
: superreview+
dveditz
: approval1.8.0.7+
mtschrep
: approval1.8.1+
Details | Diff | Splinter Review
Patch rv1.2 (-u8pw)
(deleted), patch
roc
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a5) Gecko/20041121
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a5) Gecko/20041121

Check out http://www.csie.nctu.edu.tw/~cp76/linux/mozilla/gtk2.tar.bz2
for the patch.

Currently GTK2's XIM  input method doesn't support over-the-spot style.
If you'd like to try GTK2 XIM over-the-spot, please install this XIM module.
http://www.csie.nctu.edu.tw/~cp76/linux/gtk-xim

Over-the-spot style is very important for input methods(Chinese & Japanese) that
need a selection menu(window).  Others input style in XIM like on-the-spot,
off-the-spot and root doesn't report the text cursor position, so it is not
possible for a input method to move the selection menu along with the text
cursor.  The menu is usually placed below the window of mozilla, so it is very
uncomfortable for users.

Reproducible: Always
Steps to Reproduce:
1.Invoke any gtk2 input method
2. select text input area
3.

Actual Results:  
The XIM input window doesn't move close to the cursor.

Expected Results:  
The XIM input window should move close to the cursor.
I think this bug needs to have more details attached to it so non-Asian users
will understand why it's important.

Can someone from the project comment on this?
Attached file mozilla gtk2 over-the-spot patch files (obsolete) (deleted) — 
Please apply the patch to mozilla gtk2. Thank you very much.
Here is an example that shows what over-the-spot input style is.
  http://www.csie.nctu.edu.tw/~cp76/gcin/simple.png

In the .png above, there are two input method windows close to the google
textentry.  The reason the window close to textentry was not an accient and it
was not moved manually by user. Mozilla has to tell the input method server the
position of the cursor. The mozilla gtk+ 1.x widget has this capability, but
gtk2 doesn't have it. The patch is adapted from the gtk 1.x widget source.
Please add the patch to the mozilla CVS. I don't like to apply the patch over
and over again for each new mozilla/firefox release. Thank you.
Attached patch The OverTheSpot patch for mozilla (obsolete) (deleted) — — Splinter Review 
It is the OverTheSpot patch for mozilla 1.8a5.

This patch is the same as the patch sent by Edward Liu,
But it only contains diff information, not the whole source code.

After some testing, we feel that this patch is stable enough to update to the
upstream.
Please consider to apply this.

Thanks.


BTW,
There is a similar patch for firefox and thunderbird.
Please visit https://bugzilla.mozilla.org/show_bug.cgi?id=282422 for more
details.
Thanks.
I confirm that this patch does what it is supposed to, and stable enough. We CJk
users really need this patch. Thanks.
It's difinitely useful for we CJK users, please fix it. Thanks
i have tried this patch, it work perfectly for me and it's also very stable. so
Please consider to apply this patch. if do, it's a good news for CJK users. thanks.
I haven't try this patch but if works it will be a great news for CJK users. The
IME is a very important thing for us daily and at present firefox/mozilla does
not work very well.

Thanks.
Masayuki, jshin, can you look at this bug and patch? Thanks!
Component: Toolbars and Toolbar Customization → Widget: Gtk
Keywords: intl
Product: Toolkit → Core
Version: unspecified → Trunk
*** Bug 282422 has been marked as a duplicate of this bug. ***
We find this patch should update to fit the release of firefox/thunderbird
1.0.2.
Please consider to apply this.

Thanks.


BTW,
There is some screenshots and discussions about OverTheSpot mode for libgtk
2.x.
Please visit
http://mail.gnome.org/archives/gtk-devel-list/2005-March/msg00095.html for more
details.
Thanks!
Sorry for my carelessness,
This patch (the older one and the last one) were all written by Edward Liu.
You may visit
http://www.csie.nctu.edu.tw/~cp76/linux/mozilla/mdk-firefox-thunderbird-1.0.2/
for more details.

The attached patch above is the same as the patch written by Edward Liu,
But it only contains diff information, not the whole source code.

Please consider to apply this. Thanks.
Please, make a patch against the trunk rather than against 1.0.x branch. It'll
never be landed before being tested, reviewed and landed on the trunk. 
Moreover, if GTK2's XIM doesn't support over-the-spot at the moment, I don't see
much point in adding support on our end. Wouldn't it make more sense to add this
once GTK2 with over-the-spot support is released? 


(In reply to comment #13)

> Moreover, if GTK2's XIM doesn't support over-the-spot at the moment, I don't see
> much point in adding support on our end. Wouldn't it make more sense to add this
> once GTK2 with over-the-spot support is released? 

It seems not likely that the GTK2 patch is gonna be included in the next release.

http://mail.gnome.org/archives/gtk-devel-list/2005-March/msg00107.html

http://bugzilla.gnome.org/show_bug.cgi?id=158678

I'll see if mozilla works with scim and others with 'built-in' over-the-spot
support.




(In reply to comment #13)
> Moreover, if GTK2's XIM doesn't support over-the-spot at the moment, I don't see
> much point in adding support on our end. Wouldn't it make more sense to add this
> once GTK2 with over-the-spot support is released? 

Both gcin and scim support over-the-spot style. They have their own GTK_IM_MODULE.
(In reply to comment #13)
> Moreover, if GTK2's XIM doesn't support over-the-spot at the moment, I don't see
> much point in adding support on our end. Wouldn't it make more sense to add this
> once GTK2 with over-the-spot support is released? 
> 

The point is: even with gtk im module, input methods for CJK (scim for example)
still can not receive spot location update. This patch fixed it.
Hi,

There are several gif animations showing the defferents between
OnTheSpot and OverTheSpot mode.

We must key in *several* "codes" to get *one* Chinese character when
typing Chinese characters.

When using OnTheSpot mode,
The "code" will take about 4~5 Chinese character wide,
and it will make the whole line move (shake) too much when typing.

Please visit the screenshot bellow for more details:

 http://home.pchome.com.tw/net/tetralet/Linux/OnTheSpotNotGood.gif

And

 http://home.pchome.com.tw/net/tetralet/Linux/OnTheSpotNotGood2.gif

Unlike OnTheSpot mode,
We feel that the OverTheSpot mode is more pleasing to use.
Please visit the screenshot bellow for more details::

 http://home.pchome.com.tw/net/tetralet/Linux/OverTheSpotIsBetter.gif

So that We feel this patch is very important to us.
Please consider to apply this. Thanks.
Blocks: majorbugs
The patch is adapted from mozilla widget of gtk1. I must admit it is not too
good because it uses timer to periodically report the cursor position. 
Reporting the cursor position after the cursor is moved is a better way.
Here is the latest/better patch.

http://www.csie.nctu.edu.tw/~cp76/linux/mozilla/mdk-firefox-thunderbird-1.0.4/gtk2-ff-src.tbz
No longer blocks: majorbugs
When 'aLen ==0', the cursor position  is invalid. Thus this value should not be
reported to input method  server.


diff -b -u -r mozilla.orig/widget/src/gtk2/nsWindow.cpp
mozilla/widget/src/gtk2/nsWindow.cpp
--- mozilla.orig/widget/src/gtk2/nsWindow.cpp   2005-07-01 12:29:41.000000000 +0800
+++ mozilla/widget/src/gtk2/nsWindow.cpp        2005-09-07 18:31:10.000000000 +0800
@@ -4405,6 +4405,9 @@
         delete[] textEvent.rangeArray;
     }
 
+    if (aLen == 0)
+        return;
+
     gint x1, y1, x2, y2;
     GtkWidget *widget =
         get_gtk_widget_for_gdk_window(this->mDrawingarea->inner_window);
Attached patch new gtk2 patch ( 2 lines changes only) (obsolete) (deleted) — — Splinter Review 
Forget all the patch files posted. 
The patch is much simpler. Only two lines are added. I hope it can be accepted.

Thanks.

When aLen == 0, the vaule of textEvent.theReply.mCursorPosition.x is invalid.
Thus the value should not be reported to IM.
Comment on attachment 195129 [details] [diff] [review]
new gtk2 patch ( 2 lines changes only)

Masayuki, can you check this? It looks simple enough... Thanks!
Attachment #195129 - Flags: superreview?(roc)
Attachment #195129 - Flags: review?(masayuki)
Attachment #174274 - Attachment is patch: false
Attachment #174274 - Attachment mime type: text/plain → application/x-bz2-compressed
Comment on attachment 195129 [details] [diff] [review]
new gtk2 patch ( 2 lines changes only)

I think that this patch may 'suppress' this bug, but cannot fix this. Because if the caret position is updated by this event, the candidate window position may be wrong. I think that we should always return *correct* caret position even if the inputted text is empty.

I think that we should change |nsPlaintextEditor::SetCompositionString|.
http://lxr.mozilla.org/mozilla/source/editor/libeditor/text/nsPlaintextEditor.cpp#1541
Attachment #195129 - Flags: superreview?(roc)
Attachment #195129 - Flags: review?(masayuki)
Attachment #195129 - Flags: review-
Note that this problem may not occur at using IM-Ja. Because IM-Ja always has composition string (preedit string) when the candidate list is visible.
Edward Liu:

Can you continue this work? This is very many votes, we should fix this in early time. If you cannot work on this now, I'll work on this. But I cannot test(see comment 23), if I'll work. In that time, I need your help for testing.

Can you continue this work?
We don't have response from Edward Liu.
I work on this.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Component: Widget: Gtk → Editor
Summary: GTK2 IM over-the-spot patch → GTK2 IM over-the-spot doesn't work with Chinese IM because the editor doesn't return correct caret position
Attached patch Patch rv1.0 (obsolete) (deleted) — — Splinter Review 

        Attachment #174274 -
        Attachment is obsolete: true

        Attachment #174960 -
        Attachment is obsolete: true

        Attachment #179170 -
        Attachment is obsolete: true

        Attachment #195129 -
        Attachment is obsolete: true

        Attachment #223788 -
        Flags: review?(timeless)

    
    

    
  
timeless:

Would you review the patch?

The Chinese IM doesn't have composition string while the composing. This case is not thought on the editor. The editor doesn't return caret position if the text range is null. But if composition string is empty, the text range should be null. Therefore, the editor should return correct caret position every time.

    
    

    
  
Comment on attachment 223788 [details] [diff] [review]
Patch rv1.0

>+    NS_ERROR("aTextRangeList is null irrespective of the composition string is not empty.");

i don't understand this error message.

goto FINISH; (not such a great label, and i don't think all caps is the way we label labels).
you're skipping over this hack marker, why? isn't it important?
>   // XXX_kin: BEGIN HACK! HACK! HACK!
>   // XXX_kin:
>   // XXX_kin: This is lame! The IME stuff needs caret coordinates
>   // XXX_kin: synchronously, but the editor could be using async
>   // XXX_kin: updates (reflows and paints) for performance reasons.
>   // XXX_kin: In order to give IME what it needs, we have to temporarily
>   // XXX_kin: switch to sync updating during this call so that the
>   // XXX_kin: nsAutoPlaceHolderBatch can force sync reflows, paints,

>+FINISH:

        Attachment #223788 -
        Flags: review?(timeless) → review-

    
    

    
  
(In reply to comment #28)
> you're skipping over this hack marker, why? isn't it important?
> >   // XXX_kin: BEGIN HACK! HACK! HACK!
> >   // XXX_kin:
> >   // XXX_kin: This is lame! The IME stuff needs caret coordinates
> >   // XXX_kin: synchronously, but the editor could be using async
> >   // XXX_kin: updates (reflows and paints) for performance reasons.
> >   // XXX_kin: In order to give IME what it needs, we have to temporarily
> >   // XXX_kin: switch to sync updating during this call so that the
> >   // XXX_kin: nsAutoPlaceHolderBatch can force sync reflows, paints,
> 
> >+FINISH:
> 

The cursor position is not updated by the event. I think that we need the hack when the cursor position is updated, right?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rv1.1 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
updating, and see my previous comment.

        Attachment #223788 -
        Attachment is obsolete: true

        Attachment #223809 -
        Flags: review?(timeless)

    
    

    
  
Are there any reasons mozilla cannot report the cursor position in the text entry/editing widget ?

Most gtk/qt widgets report the cursor position actively, but mozilla does this passively.
I think it's the cleanest solution.

    
    

    
  
(In reply to comment #31)
> Are there any reasons mozilla cannot report the cursor position in the text
> entry/editing widget ?
> 
> Most gtk/qt widgets report the cursor position actively, but mozilla does this
> passively.
> I think it's the cleanest solution.

What do you want to say? The caret of the Gecko is *not* a native caret. We cannot get the caret position from the native widgets.

    
    

    
  
I mean if there is a centralized function in the text widget.   The function is called to move the cursor. We can add a function call to report the cursor position.


MoveCursor(x, y)
{
..   
..
    report_cursor_position(translated x, translated y)
}

    
    

    
  
(In reply to comment #33)
> I mean if there is a centralized function in the text widget.   The function is
> called to move the cursor. We can add a function call to report the cursor
> position.

I think that your idea is not good for performance. Because in many times that is moving the caret are not needed the information on nsIWidget. We are get the cursor when the nsIWidget needs the cursor position.

    
    

    
  
I don't think it will cause performance problems.
The cursor movement is triggered by human keystrokes.  How fast can the keystrokes be sent ? I think 30 keys/second is probably the limit.   If you DO think it can cause performance problems, it can be buffered.  I am not familiar with mozilla, so I use gtk2 as a example.  I think the static variables are ok, since only one widget can receive the input focus.


static int lastx,  lasty;
gboolean cb_timeout()
{
   report_to_IM(lastx, lasty);
   return FALSE;
}

void buffered_report(int x,  int y)
{
   timeout_handle = g_timeout_add(200, cb_timeout, NULL); // call cb_timeout 0.2 sec later
   lastx = x;  lasty = y;
}

    
    

    
  
E.g., non-IM users doesn't need the caret position report, right? And there may be some platforms they doesn't need the caret position, because they cannot controling the IM position(e.g., BeOS before Gecko 1.7), right? Your idea sends the non-need information in these cases.

    
    

    
  
Can it be turned on/off by #ifdef like
#ifdef GTK2
#endif

There are still problems in the current implementation.
I think actively reporting cursor position solves the problems.

1. In Chinese, the IM is usually activated by ctrl-space.
   Currently the reporting of cursor is in the display of on-the-spot
   editing (ComposeXXX).  Activating IM doesn't need to display the
   Compose string beause ctrl-space are not actual input method keys.
   How can you tell IM the cursor position ?
   That's the reason the NULL compose is need, but I still think the solution
   is a little dirty.

2. Use mouse click to select a text entry or change the cursor position. The IM input window should follow the text cursor. Currently the cursor reporting is only in the ComposeXXX function. There is no keystorke to display, so it is not possible to report the cursor position.


-----------------------------------
The one I wrote previously doesn't work. Here is the correct one.

static int lastx,  lasty;
static int timeout_handle;

gboolean cb_timeout()
{
   timeout_handle = 0;
   report_to_IM(lastx, lasty);
   return FALSE;
}

void buffered_report(int x,  int y)
{
   lastx = x;  lasty = y;

   if (timeout_handle)
     return;

   // call cb_timeout 0.2 sec later
   timeout_handle = g_timeout_add(200, cb_timeout, NULL);
}


    
    

    
  
(In reply to comment #37)
> Can it be turned on/off by #ifdef like
> #ifdef GTK2
> #endif

We don't like |#ifdef platforms| on XP code. It makes not good for XP.
And it's not readable.

> There are still problems in the current implementation.
> I think actively reporting cursor position solves the problems.

We should fix all bugs by each best way. We should not use shortcut.
If there are other bugs, please file new bugs and cc me.

> 1. In Chinese, the IM is usually activated by ctrl-space.
>    Currently the reporting of cursor is in the display of on-the-spot
>    editing (ComposeXXX).  Activating IM doesn't need to display the
>    Compose string beause ctrl-space are not actual input method keys.
>    How can you tell IM the cursor position ?
>    That's the reason the NULL compose is need, but I still think the solution
>    is a little dirty.
> 
> 2. Use mouse click to select a text entry or change the cursor position. The IM
> input window should follow the text cursor. Currently the cursor reporting is
> only in the ComposeXXX function. There is no keystorke to display, so it is not
> possible to report the cursor position.

nsIWidget can get the current caret position by using dummy event that name is QueryCaretRect. See bug 278061.

    
    

    
  
Comment on attachment 223809 [details] [diff] [review]
Patch rv1.1

fwiw, some people use x11 over 2400bps or 14.4kbps dialup. and i can type at least 30 characters per second. i can also *try* to paste a couple of hundred characters instantly and i expect reasonable behavior.

i've had geckos where i couldn't type 12 cpm (that's characters per minute).

        Attachment #223809 -
        Flags: review?(timeless) → review+

        Attachment #223809 -
        Flags: superreview?(roc)

    
    

    
  
I would prefer not to use "goto" here. I think you can just rewrite it as an if statement.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rv1.2 (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #223809 -
        Attachment is obsolete: true

        Attachment #226123 -
        Flags: superreview?(roc)

        Attachment #226123 -
        Flags: review+

        Attachment #223809 -
        Flags: superreview?(roc)

    
    

    
  
checked-in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED

        Attachment #226123 -
        Flags: superreview?(roc) → superreview+

    
    

    
  
If somebody confirm the fixed on trunk, I'll work on 1.8 branch too.

    
    

    
  
I have tried Patch rv1.2 (-u8pw) on Firefox 1.5.0.4.

I think it's OverTheSpot works well. :-)

My build may be found in http://www.calno.com/moto/gcin/ .

Anyone want to test is very welcome.

    
    

    
  
Comment on attachment 226123 [details] [diff] [review]
Patch rv1.2

O.K. We don't have the regression reports by this. Let's go to 1.8.1 and 1.8.0.x.

        Attachment #226123 -
        Flags: approval1.8.1?

        Attachment #226123 -
        Flags: approval1.8.0.6?

    
    

    
  
Comment on attachment 226123 [details] [diff] [review]
Patch rv1.2

approved by schrep for drivers

        Attachment #226123 -
        Flags: approval1.8.1? → approval1.8.1+

    
    

    
  
checked-in to 1.8 branch. Please check the fix on 1.8 nightly builds.
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla1.8/
Keywords: fixed1.8.1

    
    

    
  
I have tried the 1.8 nightly build.

(firefox-2.0b1.en-US.linux-i686.tar.gz 31-Jul-2006 05:05)

The OverTheSpot works well. Thank you very much. :-)

    
    

    
  
Thank you for your test.

-> v1.8.1
Keywords: fixed1.8.1 → verified1.8.1

    
    

    
  
Comment on attachment 226123 [details] [diff] [review]
Patch rv1.2

approved for 1.8.0 branch, a=dveditz for drivers

        Attachment #226123 -
        Flags: approval1.8.0.7? → approval1.8.0.7+

    
    

    
  
checked-in to 1.8.0 branch too.

Please test the nightly builds on 1.8.0 branch after 8/17 builds.
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla1.8.0/
Keywords: fixed1.8.0.7

    
    

    
  
I have tried the 1.8.0 branch nightly build.

(firefox-1.5.0.7pre.en-US.linux-i686.tar.gz 17-Aug-2006 05:12)

I used it for about half a day, and the OverTheSpot works well.

I think it is as stable as 1.5.0.6. :-)

    
    

    
  
thank you for your test!
Keywords: fixed1.8.0.7 → verified1.8.0.7

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: