Bug 61023

Summary: [Chromium] IME candidate window appears wrong position in an iframe
Product: WebKit Reporter: Kenichi Ishibashi <bashi>
Component: TextAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, dglazkov, rsesek, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
testcase (main)
none
testcase (iframe)
none
Patch V0
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch V1 none

Description Kenichi Ishibashi 2011-05-17 21:55:50 PDT
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.
Comment 1 Kenichi Ishibashi 2011-05-17 21:56:21 PDT
Created attachment 93868 [details]
testcase (main)
Comment 2 Kenichi Ishibashi 2011-05-17 21:56:57 PDT
Created attachment 93869 [details]
testcase (iframe)
Comment 3 Kenichi Ishibashi 2011-05-17 21:57:49 PDT
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.
Comment 4 Kenichi Ishibashi 2011-05-17 22:13:51 PDT
Created attachment 93871 [details]
Patch V0
Comment 5 Kenichi Ishibashi 2011-05-17 22:14:31 PDT
(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.
Comment 6 WebKit Review Bot 2011-05-17 22:39:21 PDT
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
Comment 7 WebKit Review Bot 2011-05-17 22:39:25 PDT
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
Comment 8 Kent Tamura 2011-05-17 23:06:27 PDT
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?
Comment 9 Kent Tamura 2011-05-17 23:07:40 PDT
(In reply to comment #8)
> Is firstRectForCharacterRange() used only for DRT and IME? Won't his change make regressions for other usages?

his -> this
Comment 10 Kenichi Ishibashi 2011-05-17 23:27:16 PDT
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.
Comment 11 Robert Sesek 2011-05-18 07:31:02 PDT
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.
Comment 12 Kenichi Ishibashi 2011-05-18 09:16:25 PDT
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:-)
Comment 13 Kenichi Ishibashi 2011-05-18 18:02:37 PDT
Created attachment 94017 [details]
Patch V1
Comment 14 Kent Tamura 2011-05-18 18:44:55 PDT
Comment on attachment 94017 [details]
Patch V1

ok
Comment 15 WebKit Commit Bot 2011-05-19 02:22:58 PDT
Comment on attachment 94017 [details]
Patch V1

Clearing flags on attachment: 94017

Committed r86828: <http://trac.webkit.org/changeset/86828>
Comment 16 WebKit Commit Bot 2011-05-19 02:23:07 PDT
All reviewed patches have been landed.  Closing bug.