Bug 50419

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

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 on attachment 75429 [details] Add HitTestResult check before firing the RequestSoftwareInputPanel event r- for insane verbosity. Please add a helper method.

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

Created attachment 76289 [details] add help function for testing

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

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

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

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

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

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.

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.

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 on attachment 76289 [details] add help function for testing Clearing flags on attachment: 76289 Committed r74243: <http://trac.webkit.org/changeset/74243>

All reviewed patches have been landed. Closing bug.

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