Bug 50419 - [Qt] QWebPage sends out a RequestSoftwareInputPanel event incorrectly in particular case
Summary: [Qt] QWebPage sends out a RequestSoftwareInputPanel event incorrectly in part...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Yi Shen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-02 16:31 PST by Yi Shen
Modified: 2011-01-24 13:13 PST (History)
6 users (show)

See Also:


Attachments
Add HitTestResult check before firing the RequestSoftwareInputPanel event (4.18 KB, patch)
2010-12-02 16:40 PST, Yi Shen
eric: review-
Details | Formatted Diff | Diff
add help function for testing (7.74 KB, patch)
2010-12-10 19:30 PST, Yi Shen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yi Shen 2010-12-02 16:31:23 PST
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
}
Comment 1 Yi Shen 2010-12-02 16:40:35 PST
Created attachment 75429 [details]
Add HitTestResult check before firing the RequestSoftwareInputPanel event
Comment 2 Eric Seidel 2010-12-02 17:29:51 PST
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. :)
Comment 3 Eric Seidel 2010-12-09 23:39:21 PST
Comment on attachment 75429 [details]
Add HitTestResult check before firing the RequestSoftwareInputPanel event

r- for insane verbosity.  Please add a helper method.
Comment 4 Yi Shen 2010-12-10 06:29:50 PST
(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?
Comment 5 Yi Shen 2010-12-10 19:30:38 PST
Created attachment 76289 [details]
add help function for testing
Comment 6 Andreas Kling 2010-12-16 08:27:40 PST
Kenneth is our new expert on Input Method stuff, CC'ing him. :)
Comment 7 Kenneth Rohde Christiansen 2010-12-16 08:36:20 PST
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().
Comment 8 Yi Shen 2010-12-16 10:56:28 PST
(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.
Comment 9 Yi Shen 2010-12-16 13:34:41 PST
(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 :)
Comment 10 WebKit Commit Bot 2010-12-16 13:42:32 PST
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  <yurys@chromium.org>

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
Comment 11 Yi Shen 2010-12-16 14:18:29 PST
(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  <yurys@chromium.org>
> 
> 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...
Comment 12 Eric Seidel 2010-12-16 14:34:36 PST
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!
Comment 13 WebKit Commit Bot 2010-12-16 19:21:25 PST
The commit-queue encountered the following flaky tests while processing attachment 76289 [details]:

fast/loader/recursive-before-unload-crash.html bug 50880 (authors: beidson@apple.com and eric@webkit.org)
fast/preloader/script.html bug 50879 (author: abarth@webkit.org)
The commit-queue is continuing to process your patch.
Comment 14 WebKit Commit Bot 2010-12-16 22:56:34 PST
The commit-queue encountered the following flaky tests while processing attachment 76289 [details]:

editing/selection/caret-mode-paragraph-keys-navigation.html bug 51234 (author: tonikitoo@webkit.org)
compositing/reflections/animation-inside-reflection.html bug 50875 (author: simon.fraser@apple.com)
The commit-queue is continuing to process your patch.
Comment 15 WebKit Commit Bot 2010-12-17 01:26:29 PST
The commit-queue encountered the following flaky tests while processing attachment 76289 [details]:

fast/images/load-img-with-empty-src.html bug 50855 (author: mitz@webkit.org)
http/tests/misc/redirect.php bug 51238 (author: mjs@apple.com)
The commit-queue is continuing to process your patch.
Comment 16 WebKit Commit Bot 2010-12-17 02:51:48 PST
Comment on attachment 76289 [details]
add help function for testing

Clearing flags on attachment: 76289

Committed r74243: <http://trac.webkit.org/changeset/74243>
Comment 17 WebKit Commit Bot 2010-12-17 02:51:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Ademar Reis 2011-01-24 13:13:38 PST
Revision r74243 cherry-picked into qtwebkit-2.2 with commit 01fd783 <http://gitorious.org/webkit/qtwebkit/commit/01fd783>