Bug 49200 – [Qt] bugs in Composition mode for QWebPage::inputMethodEvent & inputMethodQuery()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Yi Shen]

When editor is in composition mode, the Qt::ImCursorPosition, Qt::ImAnchorPosition, Qt::ImCurrentSelection seems return wrong value, which produce some bugs on S60 VKB, e.g. entering a character which gets highlighted for a second (in composition mode), then the highlight disappeared.

Also, in my tests cases, the value of ImCursorPosition & ImAnchorostion can be negative sometimes, which is incorrect.

So, I modified QWebPage::inputMethodEvent & inputMethodQuery() based on qcoefepinputcontext_s60.cpp and now it works as following when in composition mode,

1. anchor position and cursor position are the same and always >= 0
2. current selection is always Null when in composition mode

[Yi Shen]

Created attachment 73271 [details]
tested on both linux & s60

[Andreas Kling]

Comment on attachment 73271 [details]
tested on both linux & s60

View in context: https://bugs.webkit.org/attachment.cgi?id=73271&action=review

Very nice test coverage here!
Unfortunately I have to r- because of lacking ChangeLog.

> WebKit/qt/ChangeLog:7
> +

Insufficient ChangeLog- please explain what's being changed and why.

> WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1695
> +    // clear selection, also cancel the ongoing composition if there is one

Comments should be capitalized and end with a period.

> WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1704
> +    //ImCurrentSelection

Style violation, space after //

[Yi Shen]

Created attachment 73374 [details]
fix style and changelog

[Andreas Kling]

Comment on attachment 73374 [details]
fix style and changelog

View in context: https://bugs.webkit.org/attachment.cgi?id=73374&action=review

Oops,

> WebKitTools/ChangeLog:6
> +        [Qt][QtTestBrowser] Don't erase incorrect url in the Url Bar
> +        https://bugs.webkit.org/show_bug.cgi?id=49047

This stuff belongs to another bug report :)

> WebKitTools/QtTestBrowser/mainwindow.cpp:119
> -    urlEdit->setText(url.toString(QUrl::RemoveUserInfo));
> +    setAddressUrl(url.toString(QUrl::RemoveUserInfo));

Ditto.

> WebKitTools/QtTestBrowser/mainwindow.cpp:125
> -    urlEdit->setText(url);
> +    if (!url.contains("about:"))
> +        urlEdit->setText(url);

Ditto.

[Yi Shen]

Created attachment 73375 [details]
try again

[Yi Shen]

(In reply to comment #4)
> (From update of attachment 73374 [details])
> This stuff belongs to another bug report :)
Weird, I updated both? Anyway, please review it again, thanks :)

[Yi Shen]

Created attachment 73891 [details]
fix style

[Simon Hausmann]

Comment on attachment 73891 [details]
fix style

View in context: https://bugs.webkit.org/attachment.cgi?id=73891&action=review

In general the change looks good to me, but I'd like to hear Robert's opinion on the patch, too. Nice test coverage, btw :)

> WebKit/qt/Api/qwebpage.cpp:1086
> -        editor->setComposition(ev->preeditString(), underlines, 0, ev->preeditString().length());
> +        editor->setComposition(ev->preeditString(), underlines, 0, 0);

That looks correct to me. Robert, can you remember why the pre-edit string was automatically selected before?

> WebKit/qt/Api/qwebpage.cpp:1372
> -            if (editor->hasComposition()) {
> -                RefPtr<Range> range = editor->compositionRange();
> -                return QVariant(frame->selection()->end().offsetInContainerNode() - TextIterator::rangeLength(range.get()));
> -            }
> +            if (editor->hasComposition())
> +                return QVariant(frame->selection()->end().offsetInContainerNode());

Ah, does this fix the negative positioning you mentioned in the bugreport?

> WebKit/qt/Api/qwebpage.cpp:1386
> -            if (renderTextControl) {
> +            if (!editor->hasComposition() && renderTextControl) {

Why is this check needed? When you enter composition, you clear the selection anyway, don't you?

> WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1755
> +    // Send temporary text, which makes the editor has composition 'l'.
> +    {
> +        QList<QInputMethodEvent::Attribute> attributes;
> +        QInputMethodEvent event("n", attributes);
> +        page->event(&event);

Shouldn't the comment say "has composition 'n'" ?

> WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1810
> +    // Send temporary text, which makes the editor has composition 'm'.
> +    {
> +        QList<QInputMethodEvent::Attribute> attributes;
> +        QInputMethodEvent event("d", attributes);

Same here, isn't the comment incorrect and it should say 'd' instead?

> WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1869
> +    // Send temporary text, which makes the editor has composition 'm'.
> +    {
> +        QList<QInputMethodEvent::Attribute> attributes;
> +        QInputMethodEvent event("t", attributes);
> +        page->event(&event);

Same here...

[Yi Shen]

(In reply to comment #8)
> (From update of attachment 73891 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73891&action=review
> 
> In general the change looks good to me, but I'd like to hear Robert's opinion on the patch, too. Nice test coverage, btw :)
> 
> > WebKit/qt/Api/qwebpage.cpp:1086
> > -        editor->setComposition(ev->preeditString(), underlines, 0, ev->preeditString().length());
> > +        editor->setComposition(ev->preeditString(), underlines, 0, 0);
> 
> That looks correct to me. Robert, can you remember why the pre-edit string was automatically selected before?
> 
> > WebKit/qt/Api/qwebpage.cpp:1372
> > -            if (editor->hasComposition()) {
> > -                RefPtr<Range> range = editor->compositionRange();
> > -                return QVariant(frame->selection()->end().offsetInContainerNode() - TextIterator::rangeLength(range.get()));
> > -            }
> > +            if (editor->hasComposition())
> > +                return QVariant(frame->selection()->end().offsetInContainerNode());
> 
> Ah, does this fix the negative positioning you mentioned in the bugreport?
Yes, it does.
> 
> > WebKit/qt/Api/qwebpage.cpp:1386
> > -            if (renderTextControl) {
> > +            if (!editor->hasComposition() && renderTextControl) {
> 
> Why is this check needed? When you enter composition, you clear the selection anyway, don't you?
No, the editor doesn't clear the selection when enter composition. So I add this check to make sure it returns empty string when in composition.
> 
> > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1755
> > +    // Send temporary text, which makes the editor has composition 'l'.
> > +    {
> > +        QList<QInputMethodEvent::Attribute> attributes;
> > +        QInputMethodEvent event("n", attributes);
> > +        page->event(&event);
> 
> Shouldn't the comment say "has composition 'n'" ?
I will fix it, thanks
> 
> > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1810
> > +    // Send temporary text, which makes the editor has composition 'm'.
> > +    {
> > +        QList<QInputMethodEvent::Attribute> attributes;
> > +        QInputMethodEvent event("d", attributes);
> 
> Same here, isn't the comment incorrect and it should say 'd' instead?
> 
> > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1869
> > +    // Send temporary text, which makes the editor has composition 'm'.
> > +    {
> > +        QList<QInputMethodEvent::Attribute> attributes;
> > +        QInputMethodEvent event("t", attributes);
> > +        page->event(&event);
> 
> Same here...

[Yi Shen]

Created attachment 74112 [details]
fix typo

[Robert Hogan]

(In reply to comment #8)
> 
> > WebKit/qt/Api/qwebpage.cpp:1086
> > -        editor->setComposition(ev->preeditString(), underlines, 0, ev->preeditString().length());
> > +        editor->setComposition(ev->preeditString(), underlines, 0, 0);
> 
> That looks correct to me. Robert, can you remember why the pre-edit string was automatically selected before?

That looks like a regression from http://trac.webkit.org/changeset/70259 that wasn't caught by the unit tests.

Yi, could you make sure the changes don't cause any regressions on :

fast/forms/input-maxlength-ime-preedit.html
fast/forms/input-maxlength-ime-completed.html
fast/text/international/thai-cursor-position.html
fast/events/ime-composition-events-001.html

[Yi Shen]

(In reply to comment #11)
> (In reply to comment #8)
> > 
> > > WebKit/qt/Api/qwebpage.cpp:1086
> > > -        editor->setComposition(ev->preeditString(), underlines, 0, ev->preeditString().length());
> > > +        editor->setComposition(ev->preeditString(), underlines, 0, 0);
> > 
> > That looks correct to me. Robert, can you remember why the pre-edit string was automatically selected before?
> 
> That looks like a regression from http://trac.webkit.org/changeset/70259 that wasn't caught by the unit tests.
> 
> Yi, could you make sure the changes don't cause any regressions on :
> 
> fast/forms/input-maxlength-ime-preedit.html
> fast/forms/input-maxlength-ime-completed.html
> fast/text/international/thai-cursor-position.html
> fast/events/ime-composition-events-001.html

Robert Hogan, I run the test on linux and all above tests passed.

[Robert Hogan]

(In reply to comment #12)
> 
> Robert Hogan, I run the test on linux and all above tests passed.

Great stuff. Looks good to me so!

[Andreas Kling]

Comment on attachment 74112 [details]
fix typo

LGTM, too. Thanks Simon & Robert for weighing in!

[Yi Shen]

(In reply to comment #14)
> (From update of attachment 74112 [details])
> LGTM, too. Thanks Simon & Robert for weighing in!

Thanks all you guys :)

[Kenneth Rohde Christiansen]

Comment on attachment 74112 [details]
fix typo

View in context: https://bugs.webkit.org/attachment.cgi?id=74112&action=review

> WebKit/qt/ChangeLog:10
> +        2. current selection is always NULL

We normally write "null" as we do not use a define for 0.

[Yi Shen]

Created attachment 74150 [details]
fix changelog

[Eric Seidel (no email)]

Comment on attachment 74112 [details]
fix typo

Cleared Andreas Kling's review+ from obsolete attachment 74112 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74150 [details]:

compositing/iframes/overlapped-nested-iframes.html
fast/frames/flattening/frameset-flattening-advanced.html

Please file bugs against the tests.  These tests were authored by [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.

[Yi Shen]

(In reply to comment #19)
> The commit-queue encountered the following flaky tests while processing attachment 74150 [details]:
> 
> compositing/iframes/overlapped-nested-iframes.html
> fast/frames/flattening/frameset-flattening-advanced.html
> 
> Please file bugs against the tests.  These tests were authored by [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.

Tested on Linux with r72289,

compositing/iframes/overlapped-nested-iframes.html failed without/with my patch.

fast/frames/flattening/frameset-flattening-advanced.html passed without/with my patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74150 [details]:

compositing/iframes/overlapped-nested-iframes.html
fast/dom/Document/CaretRangeFromPoint/hittest-relative-to-viewport.html

Please file bugs against the tests.  These tests were authored by [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74150 [details]:

compositing/iframes/overlapped-nested-iframes.html
fast/workers/storage/use-same-database-in-page-and-workers.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 74150 [details]
fix changelog

Clearing flags on attachment: 74150

Committed r72370: <http://trac.webkit.org/changeset/72370>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74150 [details]:

compositing/iframes/overlapped-nested-iframes.html
fast/preloader/script.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[Yi Shen]

(In reply to comment #25)
> The commit-queue encountered the following flaky tests while processing attachment 74150 [details]:
> 
> compositing/iframes/overlapped-nested-iframes.html
> fast/preloader/script.html
> 
> Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

Tested on Linux with r72337,

fast/dom/Document/CaretRangeFromPoint/hittest-relative-to-viewport.html
fast/workers/storage/use-same-database-in-page-and-workers.html
fast/preloader/script.html

all passed with this patch.