Bug 49787 – [Qt] QtWebKit does not respond properly to QInputMethodEvents with replacement text in them.

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Kristian Amlie]

Created attachment 74358 [details]
Test case

QtWebKit does not properly respond to QInputMethodEvents that have replacement text in them. QInputMethodEvent::setCommitString() takes two integers, the start position (relative to the current cursor position) and the length which should be replaced. The attached test case displays the problem:

1. Launch the application.
2. After five seconds (giving Google time to load), a timer launches which produces events.
3. The text in the input field after 5 seconds should be "ok", not "okX".

If you change to a QLineEdit by flipping the #if 1, the program works.

This is a very important fix for Nokia N8 when using Vietnamese language pack, because replacement text is used extensively in their input methods.

[Kristian Amlie]

Created attachment 74370 [details]
Patch for WebKit trunk

[Kristian Amlie]

Created attachment 74371 [details]
Patch for old WebKit which is still in Qt

The patch-old-webkit.diff patch will not go into trunk, but please review it so that it can be used in the versions of Qt that haven't updated WebKit.

[Kristian Amlie]

Created attachment 74377 [details]
Patch for WebKit trunk

[Kristian Amlie]

Created attachment 74378 [details]
Patch for old WebKit which is still in Qt

[Kristian Amlie]

Comment on attachment 74378 [details]
Patch for old WebKit which is still in Qt

Janne Koskinen reviewed this.

[Andreas Kling]

Comment on attachment 74377 [details]
Patch for WebKit trunk

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

> WebKit/qt/Api/qwebpage.cpp:1091
> +    } else if (!ev->commitString().isEmpty()) {

Coding style, no { } for single-line blocks.

> WebKit/qt/ChangeLog:11
> +	Also made sure that the preeditString is always respected, even if
> +	there is committed text. This is how QLineEdit does it.

Use spaces for indentation.

[Kristian Amlie]

Created attachment 74390 [details]
Patch for WebKit trunk after review

[Ademar Reis]

Marking as blocker for qtwebkit-2.1 since it's a must-have for the N8 (at least the Vietnamese version).

[WebKit Commit Bot]

Comment on attachment 74390 [details]
Patch for WebKit trunk after review

Rejecting patch 74390 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 74390]" exit_code: 2
Last 500 characters of output:
', u'--reviewer', u'Andreas Kling', u'--force']" exit_code: 1
Parsed 3 diffs from patch file(s).
patching file WebKit/qt/Api/qwebpage.cpp
Hunk #1 FAILED at 1082.
1 out of 1 hunk FAILED -- saving rejects to file WebKit/qt/Api/qwebpage.cpp.rej
patching file WebKit/qt/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKit/qt/tests/qwebpage/tst_qwebpage.cpp

Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Andreas Kling', u'--force']" exit_code: 1

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

[Andreas Kling]

Committed r72471: <http://trac.webkit.org/changeset/72471>

[WebKit Review Bot]

http://trac.webkit.org/changeset/72471 might have broken Qt Linux Release
The following tests are not passing:
fast/events/ime-composition-events-001.html
fast/forms/input-maxlength-ime-completed.html

[Kristian Amlie]

I'll take a look at those tests.

[Ademar Reis]

(In reply to comment #12)
> I'll take a look at those tests.

The tests have been fixed already: bug 49865, see previous comments. Or am I missing something?

[Ademar Reis]

(In reply to comment #13)
> (In reply to comment #12)
> > I'll take a look at those tests.
> 
> The tests have been fixed already: bug 49865, see previous comments. Or am I missing something?

re-closing :-)

[Ademar Reis]

(In reply to comment #13)
> (In reply to comment #12)
> > I'll take a look at those tests.
> 
> The tests have been fixed already: bug 49865, see previous comments. Or am I missing something?

I was missing something. :-) Bug 49865 is a rollout, not a fix for the tests. Sorry for the noise.

[Kristian Amlie]

Created attachment 74643 [details]
Patch for WebKit trunk, fixed test regression

The previous fix included a patch for respecting preedit text even when there is committed text. This is correct behavior if you compare with Qt, but it seems that WebKit doesn't like it. Since this is a noncritical part of the fix, I simply reverted that to "else if" instead of "if".

[Eric Seidel (no email)]

Comment on attachment 74390 [details]
Patch for WebKit trunk after review

Cleared Andreas Kling's review+ from obsolete attachment 74390 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 74643 [details]
Patch for WebKit trunk, fixed test regression

Clearing flags on attachment: 74643

Committed r72697: <http://trac.webkit.org/changeset/72697>

[Ademar Reis]

Revision r72697 cherry-picked into qtwebkit-2.1 with commit 54ceb65 <http://gitorious.org/webkit/qtwebkit/commit/54ceb65>

[Ademar Reis]

(In reply to comment #19)
> Revision r72697 cherry-picked into qtwebkit-2.1 with commit 54ceb65 <http://gitorious.org/webkit/qtwebkit/commit/54ceb65>

Nevermind, the cherry-pick applies cleanly but doesn't build, so I didn't push it. The patch requires the changes from bug 48700, which on the other hand requires changes from bug 47870 (which obviously can't be cherry-picked because of a fair ammount of conflicts).

So yet again we need a backport for qtwebkit-2.1... :-(

I'll try to prepare a new patch based on the diffs... let's see...

[Ademar Reis]

(In reply to comment #20)
> So yet again we need a backport for qtwebkit-2.1... :-(
> 
> I'll try to prepare a new patch based on the diffs... let's see...

Fortunately that was trivial... :-) I merged the differences between the trunk and 2.0 patches and cooked a new patch. It builds fine and thanks to the testcase I see it works.

Pushed to qtwebkit-2.1 as http://gitorious.org/webkit/qtwebkit/commit/85a22e12280f290c4eb0ccb4aec2ec9f13ffb7af

For some reason the Commit Bot didn't close this bug, but I see no reason to keep it open since all patches have landed and been cherry-picked. Please reopen it if necessary.

[Janne Koskinen]

> Pushed to qtwebkit-2.1 as http://gitorious.org/webkit/qtwebkit/commit/85a22e12280f290c4eb0ccb4aec2ec9f13ffb7af

Unfortunately this commit breaks text inputting on QtWebkit2.1 (least) on Symbian. Each character entered gets deleted right after entering. Commit needs to be reverted and backport redone and tested.

[Suresh Voruganti]

Ademar, can you pls look in to the latest comment #22

I am re opening the error.

[Yi Shen]

For this patch, one thing I am confusing is about the use of the parameter replaceFrom.

void QInputMethodEvent::setCommitString ( const QString & commitString, int replaceFrom = 0, int replaceLength = 0 )

In qt doc, replaceFrom specifies the position at which characters are to be replaced relative from the start of the preedit string.

while in the patch, replaceFrom is the start position relative to the current cursor position.

Any thoughts about it?

[Ademar Reis]

(In reply to comment #23)
> Ademar, can you pls look in to the latest comment #22
> 
> I am re opening the error.

open/closed relates to trunk, not qtwebkit-2.1... If the bug happens on trunk, please open a new bug for the regression. For releases (like 2.1) we use the dependency list to control if a fix has been cherry-picked or not.

So I'm closing it again, but keeping it blocking the 2.1 release (bug 39121)

[Ademar Reis]

(In reply to comment #22)
> > Pushed to qtwebkit-2.1 as http://gitorious.org/webkit/qtwebkit/commit/85a22e12280f290c4eb0ccb4aec2ec9f13ffb7af
> 
> Unfortunately this commit breaks text inputting on QtWebkit2.1 (least) on Symbian. Each character entered gets deleted right after entering. Commit needs to be reverted and backport redone and tested.

Anybody knows if it affects trunk as well? We need a new bug open if that's the case. Looking at the patch I see no reason why it would be exclusive to 2.1, but I don't have the environment to test.

[Suresh Voruganti]

*** Bug 46801 has been marked as a duplicate of this bug. ***

[Ademar Reis]

the new fix has been pushed to qtwebkit-2.1 as http://gitorious.org/webkit/qtwebkit/commit/78d2e8

[Ademar Reis]

Patch has been pushed to Qt (via src/3rdparty/webkit) master and 4.7 branches, so we need to include it in the qtwebkit-2.0 branch as well.

[Ademar Reis]

fix added to qtwebkit-2.0 branch as well: c60775d44b56a65c60131b131c001ad3a465b74e <http://gitorious.org/webkit/qtwebkit/commit/c60775d>