| Summary: | [Qt] QtWebKit does not respond properly to QInputMethodEvents with replacement text in them. | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kristian Amlie <kristian.amlie> | ||||||||||||||||
| Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Major | CC: | abarth, ademar, ankur.baranwal, commit-queue, eric, hausmann, kling, koshuin, kristian.amlie, max.hong.shen, suresh.voruganti, webkit.review.bot | ||||||||||||||||
| Priority: | P1 | Keywords: | Qt, QtTriaged | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | All | ||||||||||||||||||
| OS: | All | ||||||||||||||||||
| Bug Depends on: | 49865 | ||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
Created attachment 74370 [details]
Patch for WebKit trunk
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.
Created attachment 74377 [details]
Patch for WebKit trunk
Created attachment 74378 [details]
Patch for old WebKit which is still in Qt
Comment on attachment 74378 [details]
Patch for old WebKit which is still in Qt
Janne Koskinen reviewed this.
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. Created attachment 74390 [details]
Patch for WebKit trunk after review
Marking as blocker for qtwebkit-2.1 since it's a must-have for the N8 (at least the Vietnamese version). 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 Committed r72471: <http://trac.webkit.org/changeset/72471> 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 I'll take a look at those tests. (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? (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 :-) (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. 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 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 on attachment 74643 [details] Patch for WebKit trunk, fixed test regression Clearing flags on attachment: 74643 Committed r72697: <http://trac.webkit.org/changeset/72697> Revision r72697 cherry-picked into qtwebkit-2.1 with commit 54ceb65 <http://gitorious.org/webkit/qtwebkit/commit/54ceb65> (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... (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. > 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.
Ademar, can you pls look in to the latest comment #22 I am re opening the error. 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? (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) (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. *** Bug 46801 has been marked as a duplicate of this bug. *** the new fix has been pushed to qtwebkit-2.1 as http://gitorious.org/webkit/qtwebkit/commit/78d2e8 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. fix added to qtwebkit-2.0 branch as well: c60775d44b56a65c60131b131c001ad3a465b74e <http://gitorious.org/webkit/qtwebkit/commit/c60775d> |
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.