Bug 39625 – [Qt] Sending a QInputMethodEvent::Selection event forces the Editor to go into Composition mode

• Component: WebKit Qt
• Status: RESOLVED FIXED

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.

[Samuel Nevala]

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.

[Robert Hogan]

(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.

[Robert Hogan]

Created attachment 57267 [details]
Patch

[Robert Hogan]

(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.

[Robert Hogan]

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.

[Robert Hogan]

(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...

[Robert Hogan]

Created attachment 57408 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Robert Hogan]

(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.

[Simon Hausmann]

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.

[Robert Hogan]

(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'.

[Simon Hausmann]

(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.

[Eric Seidel (no email)]

Attachment 57408 [details] was posted by a committer and has review+, assigning to Robert Hogan for commit.

[Robert Hogan]

Created attachment 70964 [details]
Patch

[Andreas Kling]

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.

[Robert Hogan]

Created attachment 70976 [details]
Patch

[Robert Hogan]

(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.)

[Simon Hausmann]

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.

[Robert Hogan]

Created attachment 71466 [details]
Patch

[WebKit Commit Bot]

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

[Robert Hogan]

Created attachment 71472 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 71472 [details]
Patch

Clearing flags on attachment: 71472

Committed r70259: <http://trac.webkit.org/changeset/70259>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Suresh Voruganti]

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

[Ademar Reis]

Revision r70259 cherry-picked into qtwebkit-2.1 with commit 1d1745e <http://gitorious.org/webkit/qtwebkit/commit/1d1745e>

[Suresh Voruganti]

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