Bug 61023 – [Chromium] IME candidate window appears wrong position in an iframe

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Kenichi Ishibashi]

Download the attached two html files and open main.html with Chromium r83723 or later. Convert some Japanese or Chinese text in the textbox with IME (e.g. Kotoeri) to show the candiate window.  The candidate window should be located the below of the textbox, but appears somewhat upper-left position.

This problem was introduced by https://bugs.webkit.org/show_bug.cgi?id=54969. WebFrameImpl::firstRectForCharacterRange() returns coordinates relative to the focused frame when the selection is in an editable node.  RenderWidgetHostViewMac::firstRectForCharacterRange()  calls this method via IPC and converts the result to WebKit coordinates (uppler left origin) by assuming the result coordinates relative to the window.  As a result, the conversion gets incorrect.

[Kenichi Ishibashi]

Created attachment 93868 [details]
testcase (main)

[Kenichi Ishibashi]

Created attachment 93869 [details]
testcase (iframe)

[Kenichi Ishibashi]

I think WebFrameImpl::firstRectForCharacter() should always return window relative coordinates.  I checked the behavior and I couldn't find problems even it always returned window relative coordinates.  I'll send a patch.

[Kenichi Ishibashi]

Created attachment 93871 [details]
Patch V0

[Kenichi Ishibashi]

(In reply to comment #4)
> Created an attachment (id=93871) [details]
> Patch V0

In this patch, I modified TextInputController::firstRectForCharacterRange() of DRT to call the focused frame's firstRectForCharacterRange() so that DRT behaves the same manner of Chromium.

[WebKit Review Bot]

Comment on attachment 93871 [details]
Patch V0

Attachment 93871 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8709398

New failing tests:
platform/chromium-mac/editing/input/ime-candidate-window-position.html

[WebKit Review Bot]

Created attachment 93873 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kent Tamura]

Comment on attachment 93871 [details]
Patch V0

View in context: https://bugs.webkit.org/attachment.cgi?id=93871&action=review

Chromium bots runs all of tests even in LayoutTests/platform/.  You need to update test_expectations.txt.

> LayoutTests/platform/chromium-mac/editing/input/ime-candidate-window-position-expected.txt:7
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +rect[0] denotes x coodinate and rect[1] denotes y coodinate
> +PASS rect[0] is frame.offsetLeft + input.offsetLeft + 1
> +PASS rect[1] is frame.offsetTop + input.offsetTop

The output order looks very broken.
You should use jsTestIsAsync&finishJSTest().  See http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/websocket/tests/close-event.html?rev=86315

> LayoutTests/platform/chromium-mac/editing/input/ime-candidate-window-position.html:19
> +        textInputController.firstRectForCharacterRange(0, 0);
> +        rect = textInputController.firstRectForCharacterRange(0, 0);

The first line looks unneecssary.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1164
> -    // When inside an text control, don't adjust the range.
> -    if (!selectionRoot)
> -        rect = frame()->view()->contentsToWindow(rect);
> +    rect = frame()->view()->contentsToWindow(rect);

Is firstRectForCharacterRange() used only for DRT and IME? Won't his change make regressions for other usages?

[Kent Tamura]

(In reply to comment #8)
> Is firstRectForCharacterRange() used only for DRT and IME? Won't his change make regressions for other usages?

his -> this

[Kenichi Ishibashi]

Comment on attachment 93871 [details]
Patch V0

View in context: https://bugs.webkit.org/attachment.cgi?id=93871&action=review

Kent-san,

Thank you for review.  I'd like to ask for original author's comments.  Then, I'll revise the patch.

>> LayoutTests/platform/chromium-mac/editing/input/ime-candidate-window-position-expected.txt:7
>> +PASS rect[1] is frame.offsetTop + input.offsetTop
> 
> The output order looks very broken.
> You should use jsTestIsAsync&finishJSTest().  See http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/websocket/tests/close-event.html?rev=86315

Thank you for letting me know that.  I'll use these functions.

>> LayoutTests/platform/chromium-mac/editing/input/ime-candidate-window-position.html:19
>> +        rect = textInputController.firstRectForCharacterRange(0, 0);
> 
> The first line looks unneecssary.

I'll remove it.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1164
>> +    rect = frame()->view()->contentsToWindow(rect);
> 
> Is firstRectForCharacterRange() used only for DRT and IME? Won't his change make regressions for other usages?

Actually, I'm not sure this change won't make regressions so I'd like to ask original author and reviewers of their advices and suggestions.  If my understand correct, WebFrameImpl::firstRectForCharacterRange() is an implementation of Cocoa's NSTextInputClient protocol.  I think it could be called other components.  I looked for why the original code excludes editable elements for adjusting, but couldn't find the reason.  As far as I investigated,  we need to adjust coordinates including editable elements to place the candidate window on the right position.

[Robert Sesek]

Originally the IME window would be positioned incorrectly if the rect inside a text control was adjusted to window coordinates (I think the reason is that they have a different internal DOM and coordinate space).  But, I patched this in and everything appears to work fine now, so I have no objection to making this change. The only other component besides IME that uses the NSTextInput protocol is the system dictionary popup. If you hit/hold Cmd+Ctrl+D, either the currently highlighted word or the word under the mouse pointer will be defined with a little popup window.

And FYI both WebKit/mac and WebKit2 use the firstRectForRange() method for the same reasons.

[Kenichi Ishibashi]

Hi Robert,

Thank you for taking a look and comments.  I'll revise the patch then.

(In reply to comment #11)
> The only other component besides IME that uses the NSTextInput protocol is the system dictionary popup. If you hit/hold Cmd+Ctrl+D, either the currently highlighted word or the word under the mouse pointer will be defined with a little popup window.

Yeah, I noticed this feature.  This is awesome!  Thanks for implementing this feature:-)

[Kenichi Ishibashi]

Created attachment 94017 [details]
Patch V1

[Kent Tamura]

Comment on attachment 94017 [details]
Patch V1

ok

[WebKit Commit Bot]

Comment on attachment 94017 [details]
Patch V1

Clearing flags on attachment: 94017

Committed r86828: <http://trac.webkit.org/changeset/86828>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.