Bug 49787 - [Qt] QtWebKit does not respond properly to QInputMethodEvents with replacement text in them.
Summary: [Qt] QtWebKit does not respond properly to QInputMethodEvents with replacemen...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Major
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
: 46801 (view as bug list)
Depends on: 49865
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-18 23:37 PST by Kristian Amlie
Modified: 2011-02-17 06:46 PST (History)
12 users (show)

See Also:


Attachments
Test case (937 bytes, application/octet-stream)
2010-11-18 23:37 PST, Kristian Amlie
no flags Details
Patch for WebKit trunk (3.42 KB, patch)
2010-11-19 05:22 PST, Kristian Amlie
no flags Details | Formatted Diff | Diff
Patch for old WebKit which is still in Qt (2.90 KB, patch)
2010-11-19 05:24 PST, Kristian Amlie
no flags Details | Formatted Diff | Diff
Patch for WebKit trunk (3.67 KB, patch)
2010-11-19 05:58 PST, Kristian Amlie
akling: review-
Details | Formatted Diff | Diff
Patch for old WebKit which is still in Qt (2.90 KB, patch)
2010-11-19 05:59 PST, Kristian Amlie
kristian.amlie: review+
Details | Formatted Diff | Diff
Patch for WebKit trunk after review (3.68 KB, patch)
2010-11-19 08:01 PST, Kristian Amlie
no flags Details | Formatted Diff | Diff
Patch for WebKit trunk, fixed test regression (3.34 KB, patch)
2010-11-23 04:32 PST, Kristian Amlie
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kristian Amlie 2010-11-18 23:37:12 PST
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.
Comment 1 Kristian Amlie 2010-11-19 05:22:59 PST
Created attachment 74370 [details]
Patch for WebKit trunk
Comment 2 Kristian Amlie 2010-11-19 05:24:42 PST
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.
Comment 3 Kristian Amlie 2010-11-19 05:58:44 PST
Created attachment 74377 [details]
Patch for WebKit trunk
Comment 4 Kristian Amlie 2010-11-19 05:59:24 PST
Created attachment 74378 [details]
Patch for old WebKit which is still in Qt
Comment 5 Kristian Amlie 2010-11-19 06:00:12 PST
Comment on attachment 74378 [details]
Patch for old WebKit which is still in Qt

Janne Koskinen reviewed this.
Comment 6 Andreas Kling 2010-11-19 06:43:14 PST
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.
Comment 7 Kristian Amlie 2010-11-19 08:01:55 PST
Created attachment 74390 [details]
Patch for WebKit trunk after review
Comment 8 Ademar Reis 2010-11-19 08:23:01 PST
Marking as blocker for qtwebkit-2.1 since it's a must-have for the N8 (at least the Vietnamese version).
Comment 9 WebKit Commit Bot 2010-11-19 23:26:52 PST
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
Comment 10 Andreas Kling 2010-11-20 00:15:29 PST
Committed r72471: <http://trac.webkit.org/changeset/72471>
Comment 11 WebKit Review Bot 2010-11-20 02:23:06 PST
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
Comment 12 Kristian Amlie 2010-11-22 01:19:56 PST
I'll take a look at those tests.
Comment 13 Ademar Reis 2010-11-22 05:18:27 PST
(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?
Comment 14 Ademar Reis 2010-11-22 05:55:07 PST
(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 :-)
Comment 15 Ademar Reis 2010-11-22 06:07:15 PST
(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.
Comment 16 Kristian Amlie 2010-11-23 04:32:20 PST
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".
Comment 17 Eric Seidel 2010-11-24 00:23:48 PST
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.
Comment 18 WebKit Commit Bot 2010-11-24 13:29:40 PST
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>
Comment 19 Ademar Reis 2010-11-24 13:38:36 PST
Revision r72697 cherry-picked into qtwebkit-2.1 with commit 54ceb65 <http://gitorious.org/webkit/qtwebkit/commit/54ceb65>
Comment 20 Ademar Reis 2010-11-24 13:54:51 PST
(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...
Comment 21 Ademar Reis 2010-11-24 14:13:19 PST
(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.
Comment 22 Janne Koskinen 2010-12-02 05:08:29 PST
> 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.
Comment 23 Suresh Voruganti 2010-12-02 06:08:47 PST
Ademar, can you pls look in to the latest comment #22

I am re opening the error.
Comment 24 Yi Shen 2010-12-02 07:21:34 PST
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?
Comment 25 Ademar Reis 2010-12-02 07:34:45 PST
(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)
Comment 26 Ademar Reis 2010-12-03 06:48:38 PST
(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.
Comment 27 Suresh Voruganti 2010-12-07 08:26:39 PST
*** Bug 46801 has been marked as a duplicate of this bug. ***
Comment 28 Ademar Reis 2010-12-07 11:23:24 PST
the new fix has been pushed to qtwebkit-2.1 as http://gitorious.org/webkit/qtwebkit/commit/78d2e8
Comment 29 Ademar Reis 2011-02-10 13:56:22 PST
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.
Comment 30 Ademar Reis 2011-02-17 06:45:34 PST
fix added to qtwebkit-2.0 branch as well: c60775d44b56a65c60131b131c001ad3a465b74e <http://gitorious.org/webkit/qtwebkit/commit/c60775d>