Closed Bug 1120851 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Widget: Gtk, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

(Keywords: inputmethod)

Attachments

(3 files, 10 obsolete files)

20.30 KB, text/plain
Details
21.21 KB, text/plain
Details
5.27 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
- 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 'あああ'.
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)
Summary: Candidate windows sometimes doesn't set correct position when starting composition → Candidate window sometimes doesn't set correct position when starting composition
Summary: Candidate window sometimes doesn't set correct position when starting composition → Candidate window sometimes doesn't set correct position with ibus + mozc when starting composition
Attached patch v1 (obsolete) — Splinter Review
This is tested on Debian/sid with ibus + mozc.

I need test

- Anthy with ibus
- other distributions such as RHEL/CentOS 7/Unbuntu TLS.
Attachment #8558439 - Attachment is obsolete: true
mozc shows predict candidate window after typing any character.  So we should set candidate window position when updating caret position.
Attachment #8603169 - Attachment is obsolete: true
Attachment #8603695 - Flags: review?(masayuki)
Update caret data cache on chrome process by selection change etc.
Attachment #8603696 - Flags: review?(masayuki)
also you can test on Ubuntu 15.04 with ibus + mozc
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.
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
(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
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.
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.
(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 on attachment 8603695 [details] [diff] [review]
Part 1. Set candidate window position even if no composition string

cancel until bug 1130935 is fixed
Attachment #8603695 - Flags: review?(masayuki)
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.
Attachment #8603696 - Flags: review?(masayuki)
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].
Attachment #8603695 - Attachment is obsolete: true
Attachment #8603696 - Attachment is obsolete: true
I need update some tests to be ignore assertions...  I am still working...
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.
Attachment #8606936 - Flags: review?(masayuki)
Update caret cache on chrome process when updating it o content process
Attachment #8606969 - Attachment is obsolete: true
Attachment #8608014 - Flags: review?(masayuki)
Attachment #8608014 - Attachment is patch: true
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.
Attachment #8608011 - Flags: review?(masayuki)
Blocks: 1153733
Blocks: 1121408
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);
Attachment #8606936 - Flags: review?(masayuki) → review-
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?
Attachment #8608014 - Flags: review?(masayuki) → review+
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.
(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.
> 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.
Attachment #8606936 - Attachment is obsolete: true
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
Attachment #8608011 - Flags: review?(masayuki)
Attachment #8608011 - Attachment is obsolete: true
Attachment #8608014 - Attachment is obsolete: true
Attachment #8609265 - Attachment is obsolete: true
Attachment #8610037 - Attachment is obsolete: true
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
Attachment #8621518 - Flags: review?(masayuki)
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?
Attachment #8621518 - Flags: review?(masayuki) → review+
https://hg.mozilla.org/mozilla-central/rev/85383f7a9d53
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1176843
Duplicate of this bug: 1005602
(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.
Yukawa-san, thanks.
You need to log in before you can comment on or make changes to this bug.