Bug 39625 - [Qt] Sending a QInputMethodEvent::Selection event forces the Editor to go into Composition mode
Summary: [Qt] Sending a QInputMethodEvent::Selection event forces the Editor to go int...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords: Qt, QtTriaged
: 47273 (view as bug list)
Depends on:
Blocks: 47273
  Show dependency treegraph
 
Reported: 2010-05-24 16:03 PDT by rajiv.ramanasankaran
Modified: 2010-11-01 06:36 PDT (History)
11 users (show)

See Also:


Attachments
Patch (6.05 KB, patch)
2010-05-27 12:48 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (9.39 KB, patch)
2010-05-27 14:22 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (9.40 KB, patch)
2010-05-29 01:28 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (5.58 KB, patch)
2010-10-16 13:52 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (5.96 KB, patch)
2010-10-17 10:16 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (6.31 KB, patch)
2010-10-21 11:53 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (6.31 KB, patch)
2010-10-21 12:29 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description rajiv.ramanasankaran 2010-05-24 16:03:03 PDT
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.
Comment 1 Samuel Nevala 2010-05-26 23:37:14 PDT
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.
Comment 2 Robert Hogan 2010-05-27 10:29:35 PDT
(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.
Comment 3 rajiv.ramanasankaran 2010-05-27 11:33:12 PDT
(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.
Comment 4 Robert Hogan 2010-05-27 12:48:54 PDT
Created attachment 57267 [details]
Patch
Comment 5 Robert Hogan 2010-05-27 12:49:41 PDT
(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.
Comment 6 Robert Hogan 2010-05-27 14:22:39 PDT
Created attachment 57275 [details]
Patch
Comment 7 rajiv.ramanasankaran 2010-05-28 09:59:48 PDT
(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.
Comment 8 Robert Hogan 2010-05-28 13:22:04 PDT
(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!
Comment 9 rajiv.ramanasankaran 2010-05-28 13:33:45 PDT
(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...
Comment 10 Robert Hogan 2010-05-29 01:28:00 PDT
Created attachment 57408 [details]
Patch
Comment 11 Kenneth Rohde Christiansen 2010-07-03 07:40:05 PDT
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
Comment 12 Robert Hogan 2010-07-05 11:41:16 PDT
(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 13 Simon Hausmann 2010-07-12 13:06:32 PDT
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.
Comment 14 Robert Hogan 2010-07-12 14:14:35 PDT
(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'.
Comment 15 Simon Hausmann 2010-07-12 14:55:56 PDT
(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.
Comment 16 Eric Seidel 2010-10-13 12:25:00 PDT
Attachment 57408 [details] was posted by a committer and has review+, assigning to Robert Hogan for commit.
Comment 17 Robert Hogan 2010-10-16 13:52:06 PDT
Created attachment 70964 [details]
Patch
Comment 18 Andreas Kling 2010-10-17 08:46:36 PDT
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.
Comment 19 Robert Hogan 2010-10-17 10:16:03 PDT
Created attachment 70976 [details]
Patch
Comment 20 Robert Hogan 2010-10-20 12:18:30 PDT
(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 21 Simon Hausmann 2010-10-21 03:46:31 PDT
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.
Comment 22 Robert Hogan 2010-10-21 11:53:47 PDT
Created attachment 71466 [details]
Patch
Comment 23 WebKit Commit Bot 2010-10-21 12:17:48 PDT
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
Comment 24 Robert Hogan 2010-10-21 12:29:21 PDT
Created attachment 71472 [details]
Patch
Comment 25 WebKit Commit Bot 2010-10-21 12:53:32 PDT
Comment on attachment 71472 [details]
Patch

Clearing flags on attachment: 71472

Committed r70259: <http://trac.webkit.org/changeset/70259>
Comment 26 WebKit Commit Bot 2010-10-21 12:53:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 Suresh Voruganti 2010-10-22 07:37:20 PDT
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
Comment 28 Ademar Reis 2010-10-22 12:26:06 PDT
Revision r70259 cherry-picked into qtwebkit-2.1 with commit 1d1745e <http://gitorious.org/webkit/qtwebkit/commit/1d1745e>
Comment 29 Suresh Voruganti 2010-11-01 06:36:15 PDT
*** Bug 47273 has been marked as a duplicate of this bug. ***