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.
Created attachment 93868 [details] testcase (main)
Created attachment 93869 [details] testcase (iframe)
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.
Created attachment 93871 [details] Patch V0
(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 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
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 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?
(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 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.
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.
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:-)
Created attachment 94017 [details] Patch V1
Comment on attachment 94017 [details] Patch V1 ok
Comment on attachment 94017 [details] Patch V1 Clearing flags on attachment: 94017 Committed r86828: <http://trac.webkit.org/changeset/86828>
All reviewed patches have been landed. Closing bug.