Bug 51170 – [Qt] The IME composition is strangely affected by another text field

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Dongseong Hwang]

The IME composition is strangely affected by another text field when click this text field from another text field by mouse.

For example,
 1. Typed on #1 text field,  "가난"
 2. Clicked #2 text field, and typed on #2 text field, "ㅏ".
 3. "ㅏ" can not be typed in #2 text field, and #1 text filed's value is changed to "가나나" as well as #1 text field receives focus.

[Dongseong Hwang]

Created attachment 76741 [details]
Amended EditorClientQt::textFieldDidEndEditing(Element*)

Amended EditorClientQt::textFieldDidEndEditing(Element*) in order to complete composition.
It is because there is no chance to confirm text composition when user click another text field during text composition on this text field.

In detail, FocusController::setFocusedNode calls EditorClientQt::setInputMethodState(false) only when there is no new node.
It makes composition complete when user clicks the background, not elements, by mouse.

[Eric Seidel (no email)]

Comment on attachment 76741 [details]
Amended EditorClientQt::textFieldDidEndEditing(Element*)

How do we test this?

[Andreas Kling]

Comment on attachment 76741 [details]
Amended EditorClientQt::textFieldDidEndEditing(Element*)

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

> WebKit/qt/ChangeLog:13
> +        Need a short description and bug URL (OOPS!)

This line needs to be removed.

> WebKit/qt/WebCoreSupport/EditorClientQt.cpp:502
> +        // Notify qwidget to complete the text composition.

qwidget -> QWidget

> WebKit/qt/WebCoreSupport/EditorClientQt.cpp:505
> +        // Notify WebCore::Editor to complete the text composition.
> +        frame->editor()->confirmComposition();

The comment is redundant.

[Dongseong Hwang]

Thank you for comment.
It is necessary to check whether this patch is vaild because It is very old patch.

If this is vaild, I'll update the patch with layout test.

[Dongseong Hwang]

Created attachment 91899 [details]
patch

[Dongseong Hwang]

I attached the first Korean IME Layout Test for this bug.
I think EditorClient::setInputMethod is more place to confirm composition than EditorClient::textFieldDidEndEditing.

[Dongseong Hwang]

I am waiting that someone reviews this patch.

[Chang Shu]

I am trying to make some comments here: :)
The layout tests look great. But the code change is Qt only and tests are cross-platform. So my question is: Will these tests pass for all platforms? If not, you may have to fix other platforms, too. If they do, did you figure out why? I briefly looked the setInputMethodState function in other platforms and didn't see much implementation in it. Then why Qt needs the confirmComposition handling?

[Dongseong Hwang]

confirmComposition is not platform specific and all ports need to use confirmComposition for compositing some language like Korean.
This issue is ok on gtk and chromium.
I will test on gtk and chromium to verify my layout test. I wonder whether it is enough to test on 3 ports.
If not, should contributers test on all ports if they add new layout test?

[Eric Seidel (no email)]

CCing Qt reviewers and various folks who may have IME interest.

[Robert Hogan]

(In reply to comment #8)
> I am trying to make some comments here: :)
> The layout tests look great. But the code change is Qt only and tests are cross-platform. So my question is: Will these tests pass for all platforms? If not, you may have to fix other platforms, too. If they do, did you figure out why? I briefly looked the setInputMethodState function in other platforms and didn't see much implementation in it. Then why Qt needs the confirmComposition handling?

I think this may be working for GTK because it confirms the composition in EditorClientGTK::respondToChangedSelection(), and in chromium because it does the same when the text is unmarked.

IME implementations seem so many and varied it looks hard to follow another port's practice. When messing with IME stuff in the past I found it hard to keep one or other of the generic WebKit IME layout tests and the QtWebKit unit tests for QWebPage's inputMethodEvent() from breaking. You should make sure both pass before you land this when someone r+'s it. 

It would be great to have someone fix all the broken input method stuff in QtWebKit so hopefully this is the first of many!

[WebKit Review Bot]

Comment on attachment 91899 [details]
patch

Rejecting attachment 91899 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
cceeded at 1 with fuzz 3.
patching file Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp
Hunk #1 succeeded at 598 (offset -45 lines).
Hunk #2 succeeded at 642 (offset -45 lines).
patching file Source/WebKit/qt/WebCoreSupport/EditorClientQt.h
Hunk #1 FAILED at 118.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/qt/WebCoreSupport/EditorClientQt.h.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Oliver Hunt', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/9967009

[Eric Seidel (no email)]

Comment on attachment 91899 [details]
patch

Huang is not a committer, so this will need a new patch in order to land through the commit-queue.

[Dongseong Hwang]

I feel sorry to everyone about this rotten patch.
But I have not sufficient ability to finish this problem that Robert Hogan suggested.
How do I help this problem?

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.