Bug 37163 - [Qt] inputMethodQuery returns coordinates in web page coordinates rather than in item coordinates.
Summary: [Qt] inputMethodQuery returns coordinates in web page coordinates rather than...
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2010-04-06 12:02 PDT by John Pavan
Modified: 2010-04-26 03:36 PDT (History)
8 users (show)

See Also:


Attachments
Proposed fix and unit test. (4.89 KB, patch)
2010-04-14 14:57 PDT, John Pavan
no flags Details | Formatted Diff | Diff
Patch round 2 (4.91 KB, patch)
2010-04-19 12:20 PDT, John Pavan
hausmann: review-
Details | Formatted Diff | Diff
Fix applkied to QWebPage. Tests added for QGraphicsWebView and (4.84 KB, patch)
2010-04-22 10:29 PDT, John Pavan
no flags Details | Formatted Diff | Diff
Fix applied to QWebPage. Tests added for QGraphicsWebView and QWebView (4.88 KB, patch)
2010-04-22 11:26 PDT, John Pavan
laszlo.gombos: review-
Details | Formatted Diff | Diff
Updated changelog file, removed wait (5.03 KB, patch)
2010-04-22 14:50 PDT, John Pavan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Pavan 2010-04-06 12:02:52 PDT
If QGraphicsWebView::inputMethodQuery is asked to return the position of something on the web page, the values returned are in web page coordinates.  As a QGraphicsWidget, QGraphicsWebView should be returning item coordinates (relative to it's position in the QGraphicsScene).

For example, if the QGraphicsWebView contains a web page that has a cursor in an editable field at pixel 20,1200 and the web page has been scrolled so that the cursor is at 20,300.  When the microfocus is queried, QGraphicsWebView::inputMethodQuery(Qt::ImMicroFocus) returns 20,1200, not 20,300.
Comment 1 John Pavan 2010-04-06 12:04:14 PDT
I am working on getting a proposed fix ready for submission.
Comment 2 John Pavan 2010-04-14 14:57:00 PDT
Created attachment 53372 [details]
Proposed fix and unit test.

Bug is confirmed on linux.  Attached patch file should fix the problem, unit test is included.
Comment 3 WebKit Review Bot 2010-04-19 11:42:17 PDT
Attachment 53372 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qgraphicswebview.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp"
WebKit/qt/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 John Pavan 2010-04-19 12:09:33 PDT
Comment on attachment 53372 [details]
Proposed fix and unit test.

>Index: WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp
>===================================================================
>--- WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp	(revision 57531)
>+++ WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp	(working copy)
>@@ -31,6 +31,7 @@
> private slots:
>     void qgraphicswebview();
>     void crashOnViewlessWebPages();
>+    void inputMethodQueryItemCoordinates();
> };
> 
> void tst_QGraphicsWebView::qgraphicswebview()
>@@ -102,6 +103,43 @@
>     QVERIFY(waitForSignal(page, SIGNAL(loadFinished(bool))));
> }
> 
>+void tst_QGraphicsWebView::inputMethodQueryItemCoordinates()
>+{
>+    QGraphicsScene scene;
>+    QGraphicsView view(&scene);
>+
>+    QGraphicsWebView* webView = new QGraphicsWebView;
>+    QWebPage* page = new QWebPage;
>+    webView->setPage(page);
>+
>+    // set the size of the view to a known size
>+    scene.addItem(webView);
>+    webView->setGeometry(QRect(0, 0, 500, 500));
>+
>+    // set up a web page that's bigger than the size of the view
>+    page->mainFrame()->setHtml(QString("data:text/html,"
>+        "<canvas id=\"testCanvas\" width=\"1000\" height=\"1000\">"
>+        "This is a test canvas"
>+        "</canvas>"));
>+    page->mainFrame()->setFocus();
>+
>+    QTest::qWait(200);
>+    view.show();
>+    
>+    // get the initial microfocus
>+    QVariant initialMicroFocus = webView->inputMethodQuery(Qt::ImMicroFocus);
>+    QVERIFY(initialMicroFocus.isValid());
>+
>+    page->mainFrame()->scroll(300,300);
>+
>+    QVariant currentMicroFocus = webView->inputMethodQuery(Qt::ImMicroFocus);
>+    QVERIFY(currentMicroFocus.isValid());
>+
>+    QCOMPARE(initialMicroFocus.toRect().translated(-300,-300),currentMicroFocus.toRect());
>+
>+    delete webView;
>+}
>+
> QTEST_MAIN(tst_QGraphicsWebView)
> 
> #include "tst_qgraphicswebview.moc"
>Index: WebKit/qt/ChangeLog
>===================================================================
>--- WebKit/qt/ChangeLog	(revision 57608)
>+++ WebKit/qt/ChangeLog	(working copy)
>@@ -1,3 +1,17 @@
>+2010-04-14  John Pavan  <john.pavan@nokia.com>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        inputMethodQuery returns coordinates in web page coordinates rather than in item coordinates.
>+        To fix this problem if the inputMethodQuery to the underlying QWebPage returns a QPoint, QPointF, QRect, or QRectF it is transformed to reflect the difference between the web page coordinates and the widget coordinates.
>+        Added a unit test to reflect this as well.
>+        https://bugs.webkit.org/show_bug.cgi?id=37163
>+
>+        * Api/qgraphicswebview.cpp:
>+        (QGraphicsWebView::inputMethodQuery):
>+        * tests/qgraphicswebview/tst_qgraphicswebview.cpp:
>+        (tst_QGraphicsWebView::inputMethodQueryItemCoordinates):
>+
> 2010-04-14  Aaron Boodman  <aa@chromium.org>
> 
>         Reviewed by David Levin.
>Index: WebKit/qt/Api/qgraphicswebview.cpp
>===================================================================
>--- WebKit/qt/Api/qgraphicswebview.cpp	(revision 57531)
>+++ WebKit/qt/Api/qgraphicswebview.cpp	(working copy)
>@@ -668,9 +668,55 @@
> */
> QVariant QGraphicsWebView::inputMethodQuery(Qt::InputMethodQuery query) const
> {
>-    if (d->page)
>-        return d->page->inputMethodQuery(query);
>-    return QVariant();
>+    // john.pavan@nokia.com:
>+    // in cases of coordinates the QGraphicsWebView (as a QGraphicsWidget)
>+    // should be returning coordinates in the reference frame of the
>+    // item (qgraphicswebview), not the coordinates on the web page.
>+    // To do this we need the viewport.
>+
>+    QVariant retVal;
>+    if (d->page) {
>+        retVal = d->page->inputMethodQuery(query);
>+        if (retVal.type() == QVariant::RectF
>+            || retVal.type() == QVariant::PointF
>+            || retVal.type() == QVariant::Rect
>+            || retVal.type() == QVariant::Point) {
>+            // get the scroll position of the current frame.
>+                QPoint scrollPosition;
>+                if (d->page->currentFrame())
>+                    scrollPosition = d->page->currentFrame()->scrollPosition();
>+                else
>+                    scrollPosition = d->page->mainFrame()->scrollPosition();
>+                
>+
>+            // now apply the appropriate transform
>+                switch (retVal.type()) {
>+                case QVariant::RectF:
>+                    retVal = retVal.toRectF().translated(-scrollPosition);
>+                    break;
>+
>+                case QVariant::PointF:
>+                    retVal = retVal.toPointF() - scrollPosition;
>+                    break;
>+
>+                case QVariant::Rect:
>+                    retVal = retVal.toRect().translated(-scrollPosition);
>+                    break;
>+
>+                case QVariant::Point:
>+                    retVal = retVal.toPoint() - scrollPosition;
>+                    break;
>+
>+                default:
>+                    // do nothing
>+                    qt_noop();
>+                    break;
>+                };
>+        }
>+
>+
>+    }
>+    return retVal;
> }
> 
> /*! \reimp
Comment 5 John Pavan 2010-04-19 12:20:24 PDT
Created attachment 53703 [details]
Patch round 2
Comment 6 Simon Hausmann 2010-04-19 16:33:47 PDT
Comment on attachment 53703 [details]
Patch round 2


Good observation with the bug and good that you made a unit test!

I don't agree about your approach though. I think the scroll position of the frame should be taken into account
right in QWebPage::inputMethodQuery, so that it works for QWebView and QGraphicsWebView. The code also becomes
a lot simpler.

Joe, Janne: What do you think?


> ===================================================================
> --- WebKit/qt/ChangeLog	(revision 57608)
> +++ WebKit/qt/ChangeLog	(working copy)
> @@ -1,3 +1,17 @@
> +2010-04-14  John Pavan  <john.pavan@nokia.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        inputMethodQuery returns coordinates in web page coordinates rather than in item coordinates.

Please put a newline between the title of the bug and the description of the fix.
It's also good practice to prefix the title with [Qt] if it's a qt specific issue.


> +    // john.pavan@nokia.com:
> +    // in cases of coordinates the QGraphicsWebView (as a QGraphicsWidget)
> +    // should be returning coordinates in the reference frame of the
> +    // item (qgraphicswebview), not the coordinates on the web page.
> +    // To do this we need the viewport.

You don't have to put your email address as a comment into the
code. svn annotate / git blame will find out :-)
Comment 7 Joseph Ligman 2010-04-20 07:06:19 PDT
(In reply to comment #6)
> (From update of attachment 53703 [details])
> 
> Good observation with the bug and good that you made a unit test!
> 
> I don't agree about your approach though. I think the scroll position of the
> frame should be taken into account
> right in QWebPage::inputMethodQuery, so that it works for QWebView and
> QGraphicsWebView. The code also becomes
> a lot simpler.
> 
> Joe, Janne: What do you think?
> 

I agree, it does seem simpler to do this in QWebPage::inputMethodQuery. For another simplification, you might also look at ScrollView's contentsToWindow to do the coordinates conversion.
Comment 8 John Pavan 2010-04-22 10:29:53 PDT
Created attachment 54070 [details]
Fix applkied to QWebPage.  Tests added for QGraphicsWebView and
Comment 9 WebKit Review Bot 2010-04-22 11:17:37 PDT
Attachment 54070 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/qt/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/tests/qwebview/tst_qwebview.cpp"
Total errors found: 7 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 John Pavan 2010-04-22 11:26:36 PDT
Created attachment 54078 [details]
Fix applied to QWebPage.  Tests added for QGraphicsWebView and QWebView

Tabs were in the changelog, removed them.
Comment 11 Laszlo Gombos 2010-04-22 14:37:57 PDT
Comment on attachment 54078 [details]
Fix applied to QWebPage.  Tests added for QGraphicsWebView and QWebView

WebKit/qt/tests/qwebview/tst_qwebview.cpp:227
 +  
This qWait does not seems to be needed, and will slow down running the automated tests.

WebKit/qt/ChangeLog:7
 +          
We need a reference here (link) to the bugzilla bugreport - this is automatically generated if you use the prepare-ChangeLog script.



Overall the patch looks great, I would give an r+ if you would be a committer. Let's fix this 2 small issues in a new patch.
Comment 12 John Pavan 2010-04-22 14:50:19 PDT
Created attachment 54099 [details]
Updated changelog file, removed wait
Comment 13 Laszlo Gombos 2010-04-22 14:59:46 PDT
Comment on attachment 54099 [details]
Updated changelog file, removed wait

lgtm, r+. Thanks.
Comment 14 WebKit Commit Bot 2010-04-22 19:52:31 PDT
Comment on attachment 54099 [details]
Updated changelog file, removed wait

Clearing flags on attachment: 54099

Committed r58141: <http://trac.webkit.org/changeset/58141>
Comment 15 WebKit Commit Bot 2010-04-22 19:52:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Simon Hausmann 2010-04-26 03:36:18 PDT
Revision r58141 cherry-picked into qtwebkit-2.0 with commit a7515d0cfe63634a2a983bbb2f5316bccdc9c4f8