Bug 50419 – [Qt] QWebPage sends out a RequestSoftwareInputPanel event incorrectly in particular case

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

[Yi Shen]

For a page like,
<input type='text' id='input'/>
<div onclick="i=document.getElementById('input'); i.focus();">abc</div>

If user click on the div, the javascript sets the focus on the text input field. Then QWebPage sends out a RequestSoftwareInputPanel event, which invokes a VKB on symbian device.

void QWebPagePrivate::mouseReleaseEvent(T *ev) {
    ... ...
    if (mev.button() != NoButton)
        accepted = frame->eventHandler()->handleMouseReleaseEvent(mev); // set focus on the text input field which enable the inputmethod
    ... ...
    handleSoftwareInputPanel(ev->button()); // emit RequestSoftwareInputPanel event since the inputmethod has been enabled
}

[Yi Shen]

Created attachment 75429 [details]
Add HitTestResult check before firing the RequestSoftwareInputPanel event

[Eric Seidel (no email)]

Comment on attachment 75429 [details]
Add HitTestResult check before firing the RequestSoftwareInputPanel event

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

> WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2022
> +        QMouseEvent evpres(QEvent::MouseButtonPress, inputElement.geometry().center(), Qt::LeftButton, Qt::NoButton, Qt::NoModifier);
> +        page->event(&evpres);
> +        QMouseEvent evrel(QEvent::MouseButtonRelease, inputElement.geometry().center(), Qt::LeftButton, Qt::NoButton, Qt::NoModifier);
> +        page->event(&evrel);

Seems the testing infrastructure should have some helper methods for doing clicks like this.  This is insanely verbose. :)

[Eric Seidel (no email)]

Comment on attachment 75429 [details]
Add HitTestResult check before firing the RequestSoftwareInputPanel event

r- for insane verbosity.  Please add a helper method.

[Yi Shen]

(In reply to comment #3)
> (From update of attachment 75429 [details])
> r- for insane verbosity.  Please add a helper method.
Thanks for reviewing, eric :) I was wondering whether I should add the helper method in the patch. Do I need to make a new bug for adding the helper method, which can service all the qt api tests under the webkit/qt/tests folder?

[Yi Shen]

Created attachment 76289 [details]
add help function for testing

[Andreas Kling]

Kenneth is our new expert on Input Method stuff, CC'ing him. :)

[Kenneth Rohde Christiansen]

Comment on attachment 76289 [details]
add help function for testing

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

> WebKit/qt/Api/qwebpage.cpp:762
> +    handleSoftwareInputPanel(ev->button(), QPointF(ev->pos()).toPoint());

I think this should be the point where the mouse was pressed, not released. You can click, hold and select text and then release and it should popup with a selection.

Probably needs a comment for that. Anyway, I guess instead of doing a hit test what you want to do is to check if the selection (caret or range) is editable. You should look at the inputMethodQuery methods. Something like frame->selection()->isContentEditable().

[Yi Shen]

(In reply to comment #7)
> (From update of attachment 76289 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76289&action=review
> 
> > WebKit/qt/Api/qwebpage.cpp:762
> > +    handleSoftwareInputPanel(ev->button(), QPointF(ev->pos()).toPoint());
> 
> I think this should be the point where the mouse was pressed, not released. You can click, hold and select text and then release and it should popup with a selection.
> 
> Probably needs a comment for that. Anyway, I guess instead of doing a hit test what you want to do is to check if the selection (caret or range) is editable. You should look at the inputMethodQuery methods. Something like frame->selection()->isContentEditable().

Thanks Kenneth. For following page, 
<input type='text' id='input'/>
<div onclick="i=document.getElementById('input');i.focus(); i.select();">abc</div>

The VKB still pops up when user click on the div, if we only check the frame->selection()->isContentEditable(), right? Correct me if I am wrong.

[Yi Shen]

(In reply to comment #7)
> (From update of attachment 76289 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76289&action=review
> 
> > WebKit/qt/Api/qwebpage.cpp:762
> > +    handleSoftwareInputPanel(ev->button(), QPointF(ev->pos()).toPoint());
> 
> I think this should be the point where the mouse was pressed, not released. You can click, hold and select text and then release and it should popup with a selection.
> 
> Probably needs a comment for that. Anyway, I guess instead of doing a hit test what you want to do is to check if the selection (caret or range) is editable. You should look at the inputMethodQuery methods. Something like frame->selection()->isContentEditable().

Kenneth, I just tried to use frame->selection()->isContentEditable() to determine whether or not send requestsoftwareinputpanel event, but it failed my test case. See my changed code below,
void QWebPagePrivate::handleSoftwareInputPanel(...) {
...
    if (frame->selection()->isContentEditable()) {
                QEvent event(QEvent::RequestSoftwareInputPanel);
                QApplication::sendEvent(client->ownerWidget(), &event);
            }
}

But I am agree with you about using the mouse press position, instead of release position (This is how iphone safari does). I think we should have a separated bug for that since it may involve more discussion, e.g. platform behavior.

For this bug, let's just focus on fixing this particular issue :)

[WebKit Commit Bot]

Comment on attachment 76289 [details]
add help function for testing

Rejecting attachment 76289 [details] from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-attachment', '--non-interactive', 76289]" exit_code: 2
Last 500 characters of output:
ailed to merge in the changes.
Patch failed at 0001 2010-12-16  Yury Semikhatsky  <[e-mail]>

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at WebKitTools/Scripts/update-webkit line 132.

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2

Full output: http://queues.webkit.org/results/7115064

[Yi Shen]

(In reply to comment #10)
> (From update of attachment 76289 [details])
> Rejecting attachment 76289 [details] from commit-queue.
> 
> Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-attachment', '--non-interactive', 76289]" exit_code: 2
> Last 500 characters of output:
> ailed to merge in the changes.
> Patch failed at 0001 2010-12-16  Yury Semikhatsky  <[e-mail]>
> 
> When you have resolved this problem run "git rebase --continue".
> If you would prefer to skip this patch, instead run "git rebase --skip".
> To restore the original branch and stop rebasing run "git rebase --abort".
> 
> rebase refs/remotes/origin/master: command returned error: 1
> 
> Died at WebKitTools/Scripts/update-webkit line 132.
> 
> Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
> 
> Full output: http://queues.webkit.org/results/7115064

Kenneth, is this a buildbot issue? I have another patch got the same error output...

[Eric Seidel (no email)]

Comment on attachment 76289 [details]
add help function for testing

The git repo on one of the commit-queues got corrupted.  I'mf ixing it now.  Sorry for the noise!

[WebKit Commit Bot]

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

fast/loader/recursive-before-unload-crash.html bug 50880 (authors: [e-mail] and [e-mail])
fast/preloader/script.html bug 50879 (author: [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 76289 [details]:

editing/selection/caret-mode-paragraph-keys-navigation.html bug 51234 (author: [e-mail])
compositing/reflections/animation-inside-reflection.html bug 50875 (author: [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 76289 [details]:

fast/images/load-img-with-empty-src.html bug 50855 (author: [e-mail])
http/tests/misc/redirect.php bug 51238 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 76289 [details]
add help function for testing

Clearing flags on attachment: 76289

Committed r74243: <http://trac.webkit.org/changeset/74243>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r74243 cherry-picked into qtwebkit-2.2 with commit 01fd783 <http://gitorious.org/webkit/qtwebkit/commit/01fd783>