Sending a QInputMethodEvent::Selection to the Editor causes it to create a composition even though it was just a selection event. In QWebPagePrivate::inputMethodEvent(), the code handling the case for a QInputMethodEvent::Selection checks to see if the preedit text is empty. If it was empty (which is going to be the case when the text is being selected normally by the user inside a text control), it copies the text being rendered and assigns that as the preedit text. This causes the Editor to create a Composition when it may not be required. This has some side effects and screws up the internal book-keeping of the base and extent positions inside Webkit.
QWebPagePrivate::inputMethodEvent() is changed as part of https://bugs.webkit.org/show_bug.cgi?id=35702. Rajiv Ramanasankaran: What exactly goes wrong? Would it be possible to come up with layout test or auto test to catch the bug.
(In reply to comment #1) > QWebPagePrivate::inputMethodEvent() is changed as part of https://bugs.webkit.org/show_bug.cgi?id=35702. > > Rajiv Ramanasankaran: What exactly goes wrong? Would it be possible to come up with layout test or auto test to catch the bug. I am the culprit here. Looks like I need to use editor->hasComposition() in the right place. Will have a go this evening and post results.
(In reply to comment #1) > QWebPagePrivate::inputMethodEvent() is changed as part of https://bugs.webkit.org/show_bug.cgi?id=35702. > Rajiv Ramanasankaran: What exactly goes wrong? Would it be possible to come up with layout test or auto test to catch the bug. Samuel, The current auto test with negative extent passes only because the cursor position and the anchor positions are calculated as deltas (which gives correct results just by chance). I noticed that the internal bookkeeping (base and extent) inside Webkit was inconsistent though. A simple test can be to start with the Editor in non-composition mode and note if the Editor goes into composition mode immediately after sending it a QInputMethodEvent::Selection event.
Created attachment 57267 [details] Patch
(In reply to comment #4) > Created an attachment (id=57267) [details] > Patch Hi Guys, Can you take a look before a reviewer please? There's a couple of things (noted in the Changelog) that you may or may not have opinions on.
Created attachment 57275 [details] Patch
(In reply to comment #6) > Created an attachment (id=57275) [details] > Patch Couple of points: In QwebPage.cpp: 1) If the preedit string was not empty and this wasn't a selection event, we are changing the present composition to go from 0 to preedit.length. It seems wrong and unneccessary to touch the composition in this case. 2) If either the preedit string is empty or the editor doesn't have an ongoing composition, it doesn't seem necessary to change the start and end positions in this case either. These should only be changed if we had received a selection event.
(In reply to comment #7) > (In reply to comment #6) > > Created an attachment (id=57275) [details] [details] > > Patch > > Couple of points: > > In QwebPage.cpp: > 1) If the preedit string was not empty and this wasn't a selection event, we are changing the present > composition to go from 0 to preedit.length. It seems wrong and unneccessary to touch the composition in this case. Removing it doesn't appear to impact any layout tests, and the unit test I removed from tst_qwebpage was based on the assumption that an event with a preedit would not affect composition. But if we remove it we will only call setComposition() on selection events. You sure that's correct? Shouldn't a populated preedit put us into composition mode and update the current composition? > 2) If either the preedit string is empty or the editor doesn't have an ongoing composition, it doesn't > seem necessary to change the start and end positions in this case either. These should only be changed > if we had received a selection event. Of course, thanks for that!
(In reply to comment #8) > Removing it doesn't appear to impact any layout tests, and the unit test I removed from tst_qwebpage was based on the assumption that an event with a preedit would not affect composition. But if we remove it we will only call setComposition() on selection events. You sure that's correct? Shouldn't a populated preedit put us into composition mode and update the current composition? I am no InputMethod expert :-). So you may be right that the composition needs to be updated. But should it range from 0 to the length of the preedit text? Perhaps someone on IRC or mailing list can help...
Created attachment 57408 [details] Patch
Comment on attachment 57408 [details] Patch Your current patch is an improvement on the current situation so r=me. If Simon has more comments we can fix that in a follow up. A bit of comments in the code would have been nice though WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1479 + QCOMPARE(value, QString("Qit")); Is this a right change? Seems like an error
(In reply to comment #11) > (From update of attachment 57408 [details]) > Your current patch is an improvement on the current situation so r=me. If Simon has more comments we can fix that in a follow up. > > A bit of comments in the code would have been nice though > > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1479 > + QCOMPARE(value, QString("Qit")); > Is this a right change? Seems like an error Good question! My note in the changelog is about this: "Remove the test in tst_qwebpage that performs a pre-edit composition and checks that surrounding text is unaffected. If we're in composition mode and change the composition, even without committing it, we will return the surrounding the text of the latest composition. So the expectations of this test seems wrong." That's my two cents - so hopefully Simon can say if it's right or not.
Comment on attachment 57408 [details] Patch I agree that the editor should not go into composition mode if just a selection is set, but I don't think ImSurroundingText should exclude the selection. That doesn't sound correct to me and in fact it's not what QTextEdit or QLineEdit do in response to that input method query.
(In reply to comment #13) > (From update of attachment 57408 [details]) > I agree that the editor should not go into composition mode if just a selection is set, but I don't think ImSurroundingText should exclude the selection. That doesn't sound correct to me and in fact it's not what QTextEdit or QLineEdit do in response to that input method query. OK, need some guidance on what is considered surrounding text in the following sequence then: //Set selection with negative length inputAttributes << QInputMethodEvent::Attribute(QInputMethodEvent::Selection, 6, -5, QVariant()); QInputMethodEvent eventSelection3("",inputAttributes); page->event(&eventSelection3); //ImCursorPosition variant = page->inputMethodQuery(Qt::ImCursorPosition); cursorPosition = variant.toInt(); QCOMPARE(cursorPosition, 6); //ImCurrentSelection variant = page->inputMethodQuery(Qt::ImCurrentSelection); selectionValue = variant.value<QString>(); QCOMPARE(selectionValue, QString("tWebK")); //ImSurroundingText variant = page->inputMethodQuery(Qt::ImSurroundingText); QString value = variant.value<QString>(); QCOMPARE(value, QString("Qit")); My understanding was that we set the selected text into composition mode with the first call. This makes the selected text the input area, so the subsequent call to ImSurroundingText should return the text that surrounds the text in composition mode, i.e. 'Qit'.
(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 57408 [details] [details]) > > I agree that the editor should not go into composition mode if just a selection is set, but I don't think ImSurroundingText should exclude the selection. That doesn't sound correct to me and in fact it's not what QTextEdit or QLineEdit do in response to that input method query. > > OK, need some guidance on what is considered surrounding text in the following sequence then: > > //Set selection with negative length > inputAttributes << QInputMethodEvent::Attribute(QInputMethodEvent::Selection, 6, -5, QVariant()); > QInputMethodEvent eventSelection3("",inputAttributes); > page->event(&eventSelection3); > > //ImCursorPosition > variant = page->inputMethodQuery(Qt::ImCursorPosition); > cursorPosition = variant.toInt(); > QCOMPARE(cursorPosition, 6); > > //ImCurrentSelection > variant = page->inputMethodQuery(Qt::ImCurrentSelection); > selectionValue = variant.value<QString>(); > QCOMPARE(selectionValue, QString("tWebK")); > > //ImSurroundingText > variant = page->inputMethodQuery(Qt::ImSurroundingText); > QString value = variant.value<QString>(); > QCOMPARE(value, QString("Qit")); > > My understanding was that we set the selected text into composition mode with the first call. This makes the selected text the input area, so the subsequent call to ImSurroundingText should return the text that surrounds the text in composition mode, i.e. 'Qit'. The way it works in QTextEdit/QLineEdit - which is also how the input methods on Symbian and Maemo are expecting it - is that the selection is literally the visible text selection and it is distinct from the surrounding text and the pre-edit string. In other words: 1) The surrounding text is the surrounding paragraph or block of text. It does not include the pre-edit string. 2) The selection attribute in the input method events allows creating selections, as if the user created them using the mouse/keyboard. Therefore it is included in the ImSurroundingText, as it does not modify the text itself. ImCurrentSelection returns only the selected text. 3) The pre-edit string is neither visible in the selection nor in the surrounding text. So when receiving an input method event we should proceed like this (roughly, see QTextControl::inputMethodEvent for more details): 1) Determine if we are receiving text (pre-edit string, commit string or replacement length) 2) If we are receiving text, then the current selected text should be removed (if editable) 3) The replacementStart+length are is replaced with the commit string (even if empty) 4) New text selections from the attributes are applied. 5) Pre-edit text and formats are applied.
Attachment 57408 [details] was posted by a committer and has review+, assigning to Robert Hogan for commit.
Created attachment 70964 [details] Patch
Comment on attachment 70964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70964&action=review > WebKit/qt/Api/qwebpage.cpp:1059 > +#if QT_VERSION >= 0x040600 Not necessary as we don't support Qt versions older than 4.6 on WebKit trunk. > WebKit/qt/ChangeLog:8 > + Improve QWebPage handling of input method events. Please elaborate. > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:-1522 > - //Cancel current composition first > - inputAttributes << QInputMethodEvent::Attribute(QInputMethodEvent::Selection, 0, 0, QVariant()); > - QInputMethodEvent eventSelection2("",inputAttributes); > - page->event(&eventSelection2); Why are you removing this? I'm not saying it's wrong, it's just not stated anywhere why it's being removed.
Created attachment 70976 [details] Patch
(In reply to comment #19) > Created an attachment (id=70976) [details] > Patch Guys, anyone want to review? (Thanks Andreas for comments, addressed in updated patch.)
Comment on attachment 70976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70976&action=review > WebKit/qt/Api/qwebpage.cpp:-1051 > - selection = a; I think you can also remove the function-local selection variable altogether with your change.
Created attachment 71466 [details] Patch
Comment on attachment 71466 [details] Patch Rejecting patch 71466 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 71466]" exit_code: 1 Last 500 characters of output: ?id=71466&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=39625&ctype=xml Processing 1 patch from 1 bug. Cleaning working directory Updating working directory Processing patch 71466 from bug 39625. NOBODY (OOPS!) found in /Projects/CommitQueue/WebKit/qt/ChangeLog does not appear to be a valid reviewer according to committers.py. ERROR: /Projects/CommitQueue/WebKit/qt/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/4722011
Created attachment 71472 [details] Patch
Comment on attachment 71472 [details] Patch Clearing flags on attachment: 71472 Committed r70259: <http://trac.webkit.org/changeset/70259>
All reviewed patches have been landed. Closing bug.
Can you pls integrate the fix for Qtwebkit 2.1 branch? This error is important for services. The fix for this error should fix the Bug 47273
Revision r70259 cherry-picked into qtwebkit-2.1 with commit 1d1745e <http://gitorious.org/webkit/qtwebkit/commit/1d1745e>
*** Bug 47273 has been marked as a duplicate of this bug. ***