Bug 37163 – [Qt] inputMethodQuery returns coordinates in web page coordinates rather than in item coordinates.

• Component: WebKit Qt
• Status: CLOSED FIXED

[John Pavan]

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.

[John Pavan]

I am working on getting a proposed fix ready for submission.

[John Pavan]

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.

[WebKit Review Bot]

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.

[John Pavan]

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 @@
>[phone]  John Pavan  <[e-mail]>
>+
>+        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  <[e-mail]>
> 
>         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();
>+    // [e-mail]:
>+    // 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

[John Pavan]

Created attachment 53703 [details]
Patch round 2

[Simon Hausmann]

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 @@
> [phone]  John Pavan  <[e-mail]>
> +
> +        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.


> +    // [e-mail]:
> +    // 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 :-)

[Joseph Ligman]

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

[John Pavan]

Created attachment 54070 [details]
Fix applkied to QWebPage.  Tests added for QGraphicsWebView and

[WebKit Review Bot]

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.

[John Pavan]

Created attachment 54078 [details]
Fix applied to QWebPage.  Tests added for QGraphicsWebView and QWebView

Tabs were in the changelog, removed them.

[Laszlo Gombos]

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.

[John Pavan]

Created attachment 54099 [details]
Updated changelog file, removed wait

[Laszlo Gombos]

Comment on attachment 54099 [details]
Updated changelog file, removed wait

lgtm, r+. Thanks.

[WebKit Commit Bot]

Comment on attachment 54099 [details]
Updated changelog file, removed wait

Clearing flags on attachment: 54099

Committed r58141: <http://trac.webkit.org/changeset/58141>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Revision r58141 cherry-picked into qtwebkit-2.0 with commit a7515d0cfe63634a2a983bbb2f5316bccdc9c4f8