1120851 - Candidate window sometimes doesn't set correct position with ibus + mozc when starting composition

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Makoto Kato [:m_kato] (PTO until 5/6)]

- Env
Linux (Debian sid) with ibus and Mozc.

- Step
1. Input 'あああ' using IM
2. Commit string using [enter] key
3. Input 'いいい' using IM

- Result
Candidate window position is as same as step 1 (bottom of 'あああ').  But, after step 3, when hit [space] to convert hiragana, candidate window is moved to correct position.

- Expected Result
After step 3, candidate window is bottom of 'いいい', not 'あああ'.

Comment 1

[Makoto Kato [:m_kato] (PTO until 5/6)]

we are calling gtk_im_context_set_cursor_position to set candidate window position, but that window sometimes isn't moved.  gtk_im_context_set_cursor_position should be called before gtk_im_context_filter_keypress(). (it is chromium way)

Comment 2

[Makoto Kato [:m_kato] (PTO until 5/6)]

Attachment: v1

This is tested on Debian/sid with ibus + mozc.

I need test

- Anthy with ibus
- other distributions such as RHEL/CentOS 7/Unbuntu TLS.

Comment 3

[Makoto Kato [:m_kato] (PTO until 5/6)]

Attachment: Part 1. Set candidate window when caret is moved

Comment 4

[Makoto Kato [:m_kato] (PTO until 5/6)]

Attachment: Part 1. Set candidate window position even if no composition string

mozc shows predict candidate window after typing any character.  So we should set candidate window position when updating caret position.

Comment 5

[Makoto Kato [:m_kato] (PTO until 5/6)]

Attachment: Part 2. Update caret rect for e10s by selection change

Update caret data cache on chrome process by selection change etc.

Comment 6

[Makoto Kato [:m_kato] (PTO until 5/6)]

also you can test on Ubuntu 15.04 with ibus + mozc

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 8603695 [details] [diff] [review]
Part 1. Set candidate window position even if no composition string

Doesn't this cause new orange? See bug 1130935 comment 33. Perhaps, we need to fix bug 1130935 first.

Comment 8

[Yohei Yukawa]

Attachment: A possible fix in ibus-mozc side

This is a unified diff between Ubuntu 15.04 mozc-1.15.1857.102-1build1 package and proposed fix in ibus-mozc side.
http://packages.ubuntu.com/source/vivid/mozc

I confirmed that the attached patch addresses both the reported issue as of Firefox 37.0.2.
Note that the attached patch addresses was originally made to fix Mozc issue 243.
https://github.com/google/mozc/issues/243

Comment 9

[Yohei Yukawa]

(In reply to Yohei Yukawa from comment #8)
> I confirmed that the attached patch addresses both the reported issue as of
> Firefox 37.0.2.
> Note that the attached patch addresses was originally made to fix Mozc issue
> 243.
> https://github.com/google/mozc/issues/243

Correction:
I confirmed that the attached patch addresses the reported issue in Firefox. Tested with Firefox 37.0.2.

Note that the attached patch was originally for fixing Mozc issue 243.
https://github.com/google/mozc/issues/243

Comment 10

[Yohei Yukawa]

Attachment: Quilt patch made from https://bug1120851.bugzilla.mozilla.org/attachment.cgi?id=8603875

This is a quilt patch for Ubuntu 15.04 mozc-1.15.1857.102-1build1 made from https://bug1120851.bugzilla.mozilla.org/attachment.cgi?id=8603875

Just for your convenience.

Comment 11

[Yohei Yukawa]

Hi IME firefighters.

tl;dr, I'm going to fix this issue in ibus-mozc side.  ETA is by the end of 2015 August at the latest scenario.

As you may be aware, ibus-mozc has several hacks to work around limitations in GTK immodule.  A teammate did great job for that, but I finally decided to remove those hacks in a few months, before we deprecate ibus-mozc.
https://github.com/google/mozc/issues/287

You can subscribe the following issue to monitor the progress in ibus-mozc side.
https://github.com/google/mozc/issues/243

Let me know if there is something I can help with you.

Comment 12

[Makoto Kato [:m_kato] (PTO until 5/6)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 5/2 - 5/6) from comment #7)
> Comment on attachment 8603695 [details] [diff] [review]
> Part 1. Set candidate window position even if no composition string
> 
> Doesn't this cause new orange? See bug 1130935 comment 33. Perhaps, we need
> to fix bug 1130935 first.

I should wait that bug.  Until fixing it, I cancel review.

Comment 13

[Makoto Kato [:m_kato] (PTO until 5/6)]

Comment on attachment 8603695 [details] [diff] [review]
Part 1. Set candidate window position even if no composition string

cancel until bug 1130935 is fixed

Comment 14

[Makoto Kato [:m_kato] (PTO until 5/6)]

Comment on attachment 8603696 [details] [diff] [review]
Part 2. Update caret rect for e10s by selection change

cancel until bug 1130935 is fixed.  But this will need another bug, so I may file a new bug if no new orange.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

bug 1130935 has been landed. Now, you don't need to dispatch NS_QUERY_SELECTED_TEXT for retrieving the offset, length and writing mode value. It's included in aIMENotification.mSelectionChangeData. See attachment 8602657 [details] [diff] [review].

Comment 16

[Makoto Kato [:m_kato] (PTO until 5/6)]

Attachment: Part 1. Set candidate window position even if no composition string

Comment 17

[Makoto Kato [:m_kato] (PTO until 5/6)]

Attachment: Part 2. Update caret information on chrome process

Comment 18

[Makoto Kato [:m_kato] (PTO until 5/6)]

I need update some tests to be ignore assertions...  I am still working...

Comment 19

[Makoto Kato [:m_kato] (PTO until 5/6)]

Attachment: Part 3. Ignore some assertions on unit test

Comment 20

[Makoto Kato [:m_kato] (PTO until 5/6)]

Comment on attachment 8606936 [details] [diff] [review]
Part 1.  Set candidate window position even if no composition string

ibus + mozc and fcitx + mozc sometimes show predict window per inputting character.  So we should set candidate window position when changing caret position even if no composing state.

Comment 21

[Makoto Kato [:m_kato] (PTO until 5/6)]

Attachment: Part 2. Update caret information on chrome process

Update caret cache on chrome process when updating it o content process

Comment 22

[Makoto Kato [:m_kato] (PTO until 5/6)]

Comment on attachment 8608011 [details] [diff] [review]
Part 3. Ignore some assertions on unit test

Ignore assertion.  This assertion occurs when NS_QUERY_CARET_RECT on non-focus area such as image element.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 8606936 [details] [diff] [review]
Part 1.  Set candidate window position even if no composition string

> void
> nsGtkIMModule::OnUpdateComposition()
> {
>     if (MOZ_UNLIKELY(IsDestroyed())) {
>         return;
>     }
> 
>-    SetCursorPosition(GetActiveContext(), mCompositionTargetOffset);
>+    uint32_t caretOffset = UINT32_MAX;
>+    if (mCompositionTargetOffset == UINT32_MAX) {

Why don't you use !IsComposing()?

>+        // Now composition string is committed.  So we should set candidate
>+        // window position by caret
>+        WidgetQueryContentEvent selection(true, NS_QUERY_SELECTED_TEXT,
>+                                          mLastFocusedWindow);
>+        InitEvent(selection);
>+        nsEventStatus status;
>+        mLastFocusedWindow->DispatchEvent(&selection, status);
>+        if (selection.mSucceeded) {
>+            caretOffset = selection.GetSelectionEnd();
>+        }

Why don't you cache selection range at every call of OnSelectionChange()? Then, we can reduce the cost of this query.

>@@ -729,16 +745,23 @@ nsGtkIMModule::OnSelectionChange(nsWindo
>     if (aCaller != mLastFocusedWindow) {
>         PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
>             ("    WARNING: the caller isn't focused window, "
>              "mLastFocusedWindow=%p",
>              mLastFocusedWindow));
>         return;
>     }
> 
>+    if (mCompositionTargetOffset == UINT32_MAX) {

ditto: cannot use !IsComposing()?

>+    if (aTargetOffset == UINT32_MAX) {
>+        WidgetQueryContentEvent caretRect(true, NS_QUERY_CARET_RECT,
>+                                          mLastFocusedWindow);
>+        caretRect.InitForQueryCaretRect(aCaretOffset);
>+        InitEvent(caretRect);
>+        nsEventStatus status;
>+        mLastFocusedWindow->DispatchEvent(&caretRect, status);
>+        if (!caretRect.mSucceeded) {
>+            PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
>+                ("    FAILED, NS_QUERY_CARET_RECT was failed"));
>+            return;
>+        }
>+
>+        // Compute the caret position in the IM owner window.
>+        area.x = caretRect.mReply.mRect.x + rootX - ownerX;
>+        area.y = caretRect.mReply.mRect.y + rootY - ownerY;
>+        area.width  = 0;
>+        area.height = caretRect.mReply.mRect.height;
>+    } else {
>+        WidgetQueryContentEvent charRect(true, NS_QUERY_TEXT_RECT,
>+                                         mLastFocusedWindow);
>+        charRect.InitForQueryTextRect(aTargetOffset, 1);
>+        InitEvent(charRect);
>+        nsEventStatus status;
>+        mLastFocusedWindow->DispatchEvent(&charRect, status);
>+        if (!charRect.mSucceeded) {
>+            PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
>+                ("    FAILED, NS_QUERY_TEXT_RECT was failed"));
>+            return;
>+        }
>+
>+        // Compute the caret position in the IM owner window.
>+        area.x = charRect.mReply.mRect.x + rootX - ownerX;
>+        area.y = charRect.mReply.mRect.y + rootY - ownerY;
>+        area.width  = 0;
>+        area.height = charRect.mReply.mRect.height;
>+    }

Almost all of each block are same. So, cannot do this?

WidgetQueryCotentEvent queryEvent(true,
  aTargetOffset == UINT32_MAX ? NS_QUERY_CARET_RECT :
                                NS_QUERY_TEXT_RECT,
  mLastFocusedWindow);

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 8608014 [details] [diff] [review]
Part 2. Update caret information on chrome process

> PuppetWidget::GetCaretOffset()
> {
>   nsEventStatus status;
>   WidgetQueryContentEvent selection(true, NS_QUERY_SELECTED_TEXT, this);
>   InitEvent(selection, nullptr);
>   DispatchEvent(&selection, status);
>   NS_ENSURE_TRUE(selection.mSucceeded, 0);
> 
>-  return selection.mReply.mOffset;
>+  return selection.GetSelectionEnd();

Could you add a comment to explain why you use selection end for caret offset if selection isn't collapsed?

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 8608011 [details] [diff] [review]
Part 3. Ignore some assertions on unit test

You specify 0-n range to expect assertions, are those assertion counts not stable? If so, it's okay, but not so, I don't like you specify 0 for the minimum count because when we fix the assertion (I believe that it's a bug!), we should back out this patch.

Comment 26

[Makoto Kato [:m_kato] (PTO until 5/6)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (working slowly due to bad health condition) from comment #23)
> Comment on attachment 8606936 [details] [diff] [review]
> Part 1.  Set candidate window position even if no composition string
> 
> > void
> > nsGtkIMModule::OnUpdateComposition()
> > {
> >     if (MOZ_UNLIKELY(IsDestroyed())) {
> >         return;
> >     }
> > 
> >-    SetCursorPosition(GetActiveContext(), mCompositionTargetOffset);
> >+    uint32_t caretOffset = UINT32_MAX;
> >+    if (mCompositionTargetOffset == UINT32_MAX) {
> 
> Why don't you use !IsComposing()?
> 
> >+        // Now composition string is committed.  So we should set candidate
> >+        // window position by caret
> >+        WidgetQueryContentEvent selection(true, NS_QUERY_SELECTED_TEXT,
> >+                                          mLastFocusedWindow);
> >+        InitEvent(selection);
> >+        nsEventStatus status;
> >+        mLastFocusedWindow->DispatchEvent(&selection, status);
> >+        if (selection.mSucceeded) {
> >+            caretOffset = selection.GetSelectionEnd();
> >+        }
> 
> Why don't you cache selection range at every call of OnSelectionChange()?
> Then, we can reduce the cost of this query.

Cannot.  Because GTK calls nsIMEUpdatePreference::DontNotifyChangesCausedByCompostion(), so OnSelectionChange won't be called during composition.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

> Cannot.  Because GTK calls nsIMEUpdatePreference::DontNotifyChangesCausedByCompostion(),
> so OnSelectionChange won't be called during composition.

Ah, okay. But if ignoring notifications caused by composition is better, let's do this. Otherwise, it's okay to this.

Comment 28

[Makoto Kato [:m_kato] (PTO until 5/6)]

Attachment: Part 1. Set candidate window position even if no composition string

Comment 29

[Makoto Kato [:m_kato] (PTO until 5/6)]

Comment on attachment 8608011 [details] [diff] [review]
Part 3. Ignore some assertions on unit test

This assertion of crashtest always becomes 1.  So I should use 1 instead of 0-1

Comment 30

[Makoto Kato [:m_kato] (PTO until 5/6)]

Attachment: Part 3. Ignore some assertions on unit tests

Comment 31

[Makoto Kato [:m_kato] (PTO until 5/6)]

Attachment: Set candidate window position for prediction even if no composition

Comment 32

[Makoto Kato [:m_kato] (PTO until 5/6)]

Comment on attachment 8621518 [details] [diff] [review]
Set candidate window position for prediction even if no composition

rebased on current tree.

When committed composition, we need query current caret position since OnSelectionChange isn't called

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 8621518 [details] [diff] [review]
Set candidate window position for prediction even if no composition

>+         "mSelection={ mOffset=%u, mLength=%u }"
>          "mSelection.mWritingMode=%s",

Could you check this as

mSelection={ mOffet=%u, mLength=%u, mWritingMode=%s }

>     if (!charRect.mSucceeded) {
>         MOZ_LOG(gGtkIMLog, LogLevel::Info,
>-            ("    FAILED, NS_QUERY_TEXT_RECT was failed"));
>+            ("    FAILED, NS_QUERY_CARET_RECT or NS_QUERY_TEXT_RECT was "
>+             "failed"));

Could you use %s for the name and set it with useCaret?

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85383f7a9d53

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85383f7a9d53

Comment 37

[Yohei Yukawa]

(In reply to Yohei Yukawa from comment #11)
> Hi IME firefighters.
> 
> tl;dr, I'm going to fix this issue in ibus-mozc side.  ETA is by the end of
> 2015 August at the latest scenario.
> 
> As you may be aware, ibus-mozc has several hacks to work around limitations
> in GTK immodule.  A teammate did great job for that, but I finally decided
> to remove those hacks in a few months, before we deprecate ibus-mozc.
> https://github.com/google/mozc/issues/287
> 
> You can subscribe the following issue to monitor the progress in ibus-mozc
> side.
> https://github.com/google/mozc/issues/243
> 
> Let me know if there is something I can help with you.

As I promised, the proposed fix was merged as https://github.com/google/mozc/commit/9fbcdd5e27cf26ff16d72bd2d92f269334912ede.
Mozc 2.17.2109.102 and later should work well with Firefox 40 and prior.

Just for the record.

Comment 38

[Makoto Kato [:m_kato] (PTO until 5/6)]

Yukawa-san, thanks.