Bug 37163

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

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.

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

Created attachment 53703 [details] Patch round 2

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 :-)

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

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

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.

Created attachment 54078 [details] Fix applied to QWebPage. Tests added for QGraphicsWebView and QWebView Tabs were in the changelog, removed them.

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.

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

Comment on attachment 54099 [details] Updated changelog file, removed wait lgtm, r+. Thanks.

Comment on attachment 54099 [details] Updated changelog file, removed wait Clearing flags on attachment: 54099 Committed r58141: <http://trac.webkit.org/changeset/58141>

All reviewed patches have been landed. Closing bug.

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