283136 - preedit/candidate window position should follow the cursor position for Chinese/Japanese/Korean input methods

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Ervin Yan]

On JDS Chinese locales.

1. open mozilla, click in URL entry.
2. type Ctrl+space to open Chinese input methods.
3. type some letters, such as "ai".
4. you will see the preedit/candidate windows are always on top-left corner of
Mozilla window.

the windows should follow the cusor position.

Comment 1

[Ervin Yan]

Attachment: patch to set cursor position in IMEFilter()

Comment 2

[Ervin Yan]

Need call gtk_im_context_set_cursor_location() in IMEFilter(), instead of
IMEComposeText(), because in some language engines, the preedit strings are
rendering by auxiliary window, and not by Mozilla IMEComposeText().

Comment 3

[Masaki Katakai]

Attachment: screen shot on my Solaris 10 zh locale

it seems that pre-edit text and candidate window are overlapped.
Is it OK, Ervin?

Comment 4

[Masaki Katakai]

Comment on attachment 175119 [details] [diff] [review]
patch to set cursor position in IMEFilter()


>+    nsCompositionEvent compositionEvent(NS_COMPOSITION_QUERY, this);
>+
>+    nsEventStatus status;
>+    DispatchEvent (&compositionEvent, status);

>+    gtk_im_context_set_cursor_location(im, &area);

I'm afraid of performance regression,

- How does it cost for calling gtk_im_context_set_cursor_location() and
   querying NS_COMPOSITION_QUERY?

- this will be called whenever both key press and key release
   Really needed?

- nsWindow::IMEComposeStart() and nsWindow::IMEComposeText() also
   call gtk_im_context_set_cursor_location()

   If Mozilla client renders pre-edit (for example, kinput2 and atok),
   it seems duplicate call will happen for
gtk_im_context_set_cursor_location().

   Is it possible to separete codes for client-rendered pre-edit case and
   aux rendered case?

Comment 5

[Ervin Yan]

The performance is OK. see more details as follow:

- How does it cost for calling gtk_im_context_set_cursor_location() and
   querying NS_COMPOSITION_QUERY?

  in gtk_im_context_set_cursor_location(), for IIIM instance, only four assign
sentences are called. as below:
 
static void
im_context_iiim_set_cursor_location (GtkIMContext * context,
                                     GdkRectangle * area)
{
  GtkIMContextIIIM *context_iiim = GTK_IM_CONTEXT_IIIM (context);
  DEBUG_DO (g_message ("im_context_iiim_set_cursor_location"));
  if (!context_iiim)
    return;
  context_iiim->cursor.x = area->x;
  context_iiim->cursor.y = area->y;
  context_iiim->cursor.height = area->height;
  context_iiim->cursor.width = area->width;
  return;
}

for the performance of querying NS_COMPOSITION_QUERY, the source code for this
action is in "view/src/nsViewManager.cpp". 

it think the performance for this action should be also OK. compared with the
worst case, for each mouse motion event, Mozilla will call a similar function as
DispatchEvent(NS_MOUSE_MOVE event) to get/set the mouse point, since the
performance for so many mouse motion events is OK, I think the performance for
key press event should be also OK.



- this will be called whenever both key press and key release
   Really needed?

Yes, only key press event is needed, so I will update the patch.

- nsWindow::IMEComposeStart() and nsWindow::IMEComposeText() also
   call gtk_im_context_set_cursor_location()

   On trunk, no gtk_im_context_set_cursor_location() call in
nsWindow::IMEComposeStart() and nsWindow::IMEComposeText(). 


>   If Mozilla client renders pre-edit (for example, kinput2 and atok),
>   it seems duplicate call will happen for
gtk_im_context_set_cursor_location().

    I think kinput2 and atok will not call gtk_im_context_set_cursor_location(),
  this function should be called by gtk im modules(such as iiimgcf), not by
client, kinput2 and atok just get the cursor location through XIMP or IIIM aux
object. so no duplicate call happen for gtk_im_context_set_cursor_location().

>   Is it possible to separete codes for client-rendered pre-edit case and
>   aux rendered case?

I think it should be too complex to separete codes for client-rendered and aux
rendered case, because mozilla do not know which kind of client is there.

Comment 6

[Ervin Yan]

>- nsWindow::IMEComposeStart() and nsWindow::IMEComposeText() also
>   call gtk_im_context_set_cursor_location()

>   On trunk, no gtk_im_context_set_cursor_location() call in
>nsWindow::IMEComposeStart() and nsWindow::IMEComposeText(). 

sorry, I just mistake, on Trunk, nsWindow::IMEComposeText() have called
gtk_im_context_set_cursor_location(), we need remove it.

Comment 7

[Jungshik Shin]

This needs a thorough testing with various input methods for CJK (and possibly
Thai). I'll try to test it.

Comment 8

[Ervin Yan]

Oops, someone have check in a patch to fix bug 281399 yesterday. the patch added
calls in gtk_im_context_set_cursor_location() in IMEComposeStart() and
IMEComposeText().

but this bug still happen on JDS since Chinese use auxiliary window to show
preedit strings, and IMEComposeStart() and IMEComposeText() will not be called
in Mozilla.

will attach a snapshot.

Comment 9

[Ervin Yan]

Attachment: still wrong position on JDS after apply the patch for bug 281399

image to the wrong preedit position on JDS chinese locale. in which, the
preedit string is shown by a auxiliary window.

Comment 10

[Jungshik Shin]

Ervin, why did you change the product and the component back to 'Suite' and
'General'? Is this bug  specific to the suite? Your patch changes nsWindow.cpp
for gtk2 which must be shared by all mozilla products (suite, firefox,
thunderbird, sunbird, etc). 

Comment 11

[Ervin Yan]

Attachment: updated patch

only call gtk_im_context_set_cursor_position() in OnKeyPressEvent().

Comment 12

[Ervin Yan]

sorry, the bug id in comment #8 #9 is wrong, should be bug 281339.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

I agree with Katakai san.

Ervin's patch is not suited for calling gtk_im_set_cursor_location(), because
OnKeyPressEvent() is not only for IM. It will cause performance regression
whether you feel or not.

Ervin, is there another signal to get the sign of starting preedit in your im
module?

The solution I think is "preedit-changed" signal is emitted in your im module
when starting preediting.  Can you change your im module such way?

Comment 14

[Ervin Yan]

Attachment: updated patch to remove performance issue.

Comment 15

[Eric Pignet]

I'm sorry the two last patches don't work with my configuration: Linux + SCIM
(Smart Common Input Method), Chinese language.

In the first patch (175405), the IMESetComposePosition() fonction is called in
OnKeyPressEvent(). So the first time I launch the IME with CTRL+SPACE, the
fonction is called and the preedit window is displayed at the right location,
but then all key-presses are catched by the IME, OnKeyPressEvent() is no more
called and the preedit window stays at the initial location, until the IME is
closed.
Chinese users don't close and reopen their IME every time they write something.

In the last patch (179817), the IMESetComposePosition() fonction is called in
IMESetFocus(). This solves the performance issue and now the preedit window is
always displayed at the right place. But the patch adds new problems.
The most serious is that with this patch mozilla frequently crashes due to an
infinite loop. When I add traces in the code, I can see that in
IMESetComposePosition(), a SetFocus event is sometimes sent by
DispatchEvent(&compEvent, status), and IMESetFocus() is called again:
(with export NSPR_LOG_MODULES=WidgetIM:5,WidgetFocus:5)

1084777728[8093878]:   SetFocus [85aea68]
1084777728[8093878]: IMESetFocus 85aea68
1084777728[8093878]: IMESetComposePosition [85aea68]
1084777728[8093878]:   SetFocus [85aea68]
1084777728[8093878]: IMESetFocus 85aea68
1084777728[8093878]: IMESetComposePosition [85aea68]
and so on.

Another issue with the last patch is that when I select a text in an input field
and then try to close or open the IME, the preedit window is hidden and will
never come back, even though I can write Chinese.

I have no solution which works, is stable and does not cause performance issue,
but I'm sure such a solution exists.

Ervin, an idea ? I'm available to test any patch with my configuration.

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Any progress here?  We'd like the candidate window to be correct for zh_TW
locales, as well.

Comment 17

[Eric Pignet]

There is a patch for bug 271815 which works perfectly with gtk2 and
over-the-spot method.

But bug 271815 isn't ever assigned, so we shouldn't hope having it included in a
near Firefox release. And yet it is a very important issue for asian users.
I think that a reviewer should test and review it.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 175119 [details] [diff] [review]
patch to set cursor position in IMEFilter()

This patch has been obsoleted.
And Mr.Katakai is not active reviewer.

If you request the review for new patch, you must change the reviewer.

Comment 19

[Eric Pignet]

I don't request the last patch to be reviewed. As I said in comment #17 this
patch is wrong.
There is actually no patch which fixes this bug for any input style.

But there is a patch which works for the over-the-spot style, attached to the
bug 271815 . I'm not allowed to assign this bug, and I can't request a review
for the patch either.
Maybe you Masayuki could take the bug, if you have time. Or you can assign it to
anybody else. What is sure is that a bug not assigned is unlikely to be commited
one day.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry, I cannot write the IM code for Linux.
I can write only XP code and Windows code.

Comment 21

[AGC Tester]

The problem still happens with Firefox3.5.1. But only happens at the first invoking of input method. Afterwards invoking behaves fine.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'll fix this in bug 520732.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

should be fixed by bug 520732. If it's not so, please reopen this bug.