Closed
Bug 1120851
Opened 10 years ago
Closed 9 years ago
Candidate window sometimes doesn't set correct position with ibus + mozc when starting composition
Categories
(Core :: Widget: Gtk, defect)
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)
- 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 'あああ'.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Summary: Candidate windows sometimes doesn't set correct position when starting composition → Candidate window sometimes doesn't set correct position when starting composition
Assignee | ||
Updated•10 years ago
|
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
Assignee | ||
Comment 2•10 years ago
|
||
This is tested on Debian/sid with ibus + mozc.
I need test
- Anthy with ibus
- other distributions such as RHEL/CentOS 7/Unbuntu TLS.
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8558439 -
Attachment is obsolete: true
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
Update caret data cache on chrome process by selection change etc.
Attachment #8603696 -
Flags: review?(masayuki)
Assignee | ||
Comment 6•10 years ago
|
||
also you can test on Ubuntu 15.04 with ibus + mozc
Comment 7•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
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.
Assignee | ||
Comment 12•10 years ago
|
||
(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.
Assignee | ||
Comment 13•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
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)
Comment 15•10 years ago
|
||
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].
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8603695 -
Attachment is obsolete: true
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8603696 -
Attachment is obsolete: true
Assignee | ||
Comment 18•10 years ago
|
||
I need update some tests to be ignore assertions... I am still working...
Assignee | ||
Comment 19•10 years ago
|
||
Assignee | ||
Comment 20•10 years ago
|
||
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)
Assignee | ||
Comment 21•10 years ago
|
||
Update caret cache on chrome process when updating it o content process
Attachment #8606969 -
Attachment is obsolete: true
Attachment #8608014 -
Flags: review?(masayuki)
Assignee | ||
Updated•10 years ago
|
Attachment #8608014 -
Attachment is patch: true
Assignee | ||
Comment 22•10 years ago
|
||
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)
Comment 23•10 years ago
|
||
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 24•10 years ago
|
||
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 25•10 years ago
|
||
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.
Assignee | ||
Comment 26•10 years ago
|
||
(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•10 years ago
|
||
> 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.
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8606936 -
Attachment is obsolete: true
Assignee | ||
Comment 29•10 years ago
|
||
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)
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8608011 -
Attachment is obsolete: true
Assignee | ||
Comment 31•9 years ago
|
||
Attachment #8608014 -
Attachment is obsolete: true
Attachment #8609265 -
Attachment is obsolete: true
Attachment #8610037 -
Attachment is obsolete: true
Assignee | ||
Comment 32•9 years ago
|
||
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 33•9 years ago
|
||
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+
Comment 34•9 years ago
|
||
Comment 35•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 37•9 years ago
|
||
(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.
Assignee | ||
Comment 38•9 years ago
|
||
Yukawa-san, thanks.
You need to log in
before you can comment on or make changes to this bug.
Description
•