Bug 32668

Created attachment 45080 [details] Patch to add new QWebFrame API scrollRecursively I took the concept from the EventHandler::scrollRecursively

Attachment 45080 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2822: Missing space after , [whitespace/comma] [3] WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2832: Missing space after , [whitespace/comma] [3] WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2845: Missing space after , [whitespace/comma] [3] WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2855: Missing space after , [whitespace/comma] [3] WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2858: Missing space after , [whitespace/comma] [3] Total errors found: 5

Created attachment 45081 [details] Patch to add new QWebFrame API scrollRecursively

style-queue ran check-webkit-style on attachment 45081 [details] without any errors.

(In reply to comment #2) > Attachment 45080 [details] did not pass style-queue: > > Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 > WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2822: Missing space after , > [whitespace/comma] [3] > WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2832: Missing space after , > [whitespace/comma] [3] > WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2845: Missing space after , > [whitespace/comma] [3] > WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2855: Missing space after , > [whitespace/comma] [3] > WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2858: Missing space after , > [whitespace/comma] [3] > Total errors found: 5 at least I'm consistent :)

Comment on attachment 45081 [details] Patch to add new QWebFrame API scrollRecursively > Index: WebKit/qt/ChangeLog > =================================================================== > --- WebKit/qt/ChangeLog (revision 52259) > +++ WebKit/qt/ChangeLog (working copy) > @@ -1,3 +1,21 @@ > +2009-12-17 Joe Ligman <joseph.ligman@nokia.com> > + > + Reviewed by NOBODY (OOPS!). > + > + [Qt] Add new API to QWebFrame to scrollRecursively starting with any css overflow > + then checking current frame and then ancestors > + https://bugs.webkit.org/show_bug.cgi?id=32668 > + > + * Api/qwebframe.cpp: > + (QWebFramePrivate::scrollOverflow): > + (QWebFrame::scrollRecursively): > + * Api/qwebframe.h: > + * Api/qwebframe_p.h: > + * tests/qwebframe/qwebframe.qrc: > + * tests/qwebframe/testiframe.html: Added. > + * tests/qwebframe/testiframe2.html: Added. > + * tests/qwebframe/tst_qwebframe.cpp: > + > 2009-12-17 Benjamin Poulain <benjamin.poulain@nokia.com> > > Reviewed by Simon Hausmann. > Index: WebKit/qt/Api/qwebframe.cpp > =================================================================== > --- WebKit/qt/Api/qwebframe.cpp (revision 52259) > +++ WebKit/qt/Api/qwebframe.cpp (working copy) > @@ -361,6 +361,45 @@ void QWebFramePrivate::renderRelativeCoo > } > } > > +bool QWebFramePrivate::scrollOverflow(int dx, int dy) > +{ > + if (!frame || !frame->document() || !frame->eventHandler()) > + return false; > + > + Node* node = frame->document()->focusedNode(); > + if (!node) > + node = frame->document()->elementFromPoint(frame->eventHandler()->currentMousePosition().x(), > + frame->eventHandler()->currentMousePosition().y()); Remove the space before frame-> > + if (!node) > + return false; > + > + RenderObject* renderer = node->renderer(); > + if (!renderer) > + return false; > + > + if (renderer->isListBox()) > + return false; > + > + RenderLayer* renderLayer = renderer->enclosingLayer(); > + if (!renderLayer) > + return false; > + > + bool scrolledHorizontal = false; > + bool scrolledVertical = false; > + > + if (dx > 0) > + scrolledHorizontal = renderLayer->scroll(ScrollRight, ScrollByPixel, dx); > + else if (dx < 0) > + scrolledHorizontal = renderLayer->scroll(ScrollLeft, ScrollByPixel, qAbs(dx)); > + > + if (dy > 0) > + scrolledVertical = renderLayer->scroll(ScrollDown, ScrollByPixel, dy); > + else if (dy < 0) > + scrolledVertical = renderLayer->scroll(ScrollUp, ScrollByPixel, qAbs(dy)); > + > + return (scrolledHorizontal || scrolledVertical); > +} > + > /*! > \class QWebFrame > \since 4.4 > @@ -1000,6 +1039,50 @@ void QWebFrame::scroll(int dx, int dy) > } > > /*! > + \since 4.7 > + Scrolls nested frames starting at this frame, \a dx pixels to the right > + and \a dy pixels downward. Both \a dx and \a dy may be negative. First attempts > + to scroll elements with css overflow followed by this frame. If this It is called CSS > + frame doesn't scroll, attempts to scroll the parent > + > + \sa QWebFrame::scroll, QWebFramePrivate::scrollOverflow Dont add references to the private! > +*/ > +bool QWebFrame::scrollRecursively(int dx, int dy) > +{ > + bool scrolledHorizontal = false; > + bool scrolledVertical = false; > + bool scrolledOverflow = d->scrollOverflow(dx, dy); > + > + if (!scrolledOverflow) { > + Frame* frame = d->frame; > + if (!frame || !frame->view()) > + return false; > + > + do { > + IntSize scrollOffset = frame->view()->scrollOffset(); > + IntPoint maxScrollOffset = frame->view()->maximumScrollPosition(); > + > + if (dx > 0) // scroll right > + scrolledHorizontal = scrollOffset.width() < maxScrollOffset.x(); > + else if (dx < 0) // scroll left > + scrolledHorizontal = scrollOffset.width() > 0; > + > + if (dy > 0) // scroll down > + scrolledVertical = scrollOffset.height() < maxScrollOffset.y(); > + else if (dy < 0) //scroll up > + scrolledVertical = scrollOffset.height() > 0; > + > + if (scrolledHorizontal || scrolledVertical) { > + frame->view()->scrollBy(IntSize(dx, dy)); > + return true; > + } > + frame = frame->tree()->parent(); > + } while (frame && frame->view()); > + } > + return (scrolledHorizontal || scrolledVertical || scrolledOverflow); > +} > + > +/*! > \property QWebFrame::scrollPosition > \since 4.5 > \brief the position the frame is currently scrolled to. > Index: WebKit/qt/Api/qwebframe.h > =================================================================== > --- WebKit/qt/Api/qwebframe.h (revision 52259) > +++ WebKit/qt/Api/qwebframe.h (working copy) > @@ -156,6 +156,7 @@ public: > QRect scrollBarGeometry(Qt::Orientation orientation) const; > > void scroll(int, int); > + bool scrollRecursively(int, int); > QPoint scrollPosition() const; > void setScrollPosition(const QPoint &pos); > > Index: WebKit/qt/Api/qwebframe_p.h > =================================================================== > --- WebKit/qt/Api/qwebframe_p.h (revision 52259) > +++ WebKit/qt/Api/qwebframe_p.h (working copy) > @@ -85,6 +85,8 @@ public: > void renderRelativeCoords(WebCore::GraphicsContext*, QWebFrame::RenderLayer, const QRegion& clip); > void renderContentsLayerAbsoluteCoords(WebCore::GraphicsContext*, const QRegion& clip); > > + bool scrollOverflow(int dx, int dy); > + > QWebFrame *q; > Qt::ScrollBarPolicy horizontalScrollBarPolicy; > Qt::ScrollBarPolicy verticalScrollBarPolicy; > Index: WebKit/qt/tests/qwebframe/qwebframe.qrc > =================================================================== > --- WebKit/qt/tests/qwebframe/qwebframe.qrc (revision 52259) > +++ WebKit/qt/tests/qwebframe/qwebframe.qrc (working copy) > @@ -4,5 +4,7 @@ > <file>style.css</file> > <file>test1.html</file> > <file>test2.html</file> > +<file>testiframe.html</file> > +<file>testiframe2.html</file> > </qresource> > </RCC> > Index: WebKit/qt/tests/qwebframe/testiframe2.html > =================================================================== > --- WebKit/qt/tests/qwebframe/testiframe2.html (revision 0) > +++ WebKit/qt/tests/qwebframe/testiframe2.html (revision 0) > @@ -0,0 +1,21 @@ > +</html> > +<html> > +<head> > +<title></title> > +<style type="text/css"> > +<!-- > +#content { > + background: #fff; > + position: absolute; > + top: 0px; > + left: 0px; > + width: 800px; > + height: 800px; > +} > +--> > +</style> > +</head> > +<body> > +<div id="content"> </div> > +</body> > +</html> > \ No newline at end of file > Index: WebKit/qt/tests/qwebframe/testiframe.html > =================================================================== > --- WebKit/qt/tests/qwebframe/testiframe.html (revision 0) > +++ WebKit/qt/tests/qwebframe/testiframe.html (revision 0) > @@ -0,0 +1,54 @@ > +</html> > +<html> > +<head> > +<title></title> > +<style type="text/css"> > +<!-- > +#header { > + background: #0f0; > + position: absolute; > + top: 0px; > + left: 0px; > + width: 800px; > + height: 100px; > +} > +#content1 { > + background: #ff0; > + position: absolute; > + top: 101px; > + left: 0px; > + width: 400px; > + height: 400px; > + overflow: scroll; > +} > +#content2 { > + background: #ff7; > + position: absolute; > + top: 101px; > + left: 401px; > + width: 400px; > + height: 400px; > +} > +#footer { > + background: #0f0; > + position: absolute; > + top: 502px; > + left: 0px; > + width: 800px; > + height: 200px; > +} > +--> > +</style> > +</head> > +<body> > +<div id="header"></div> > +<div id="content1">You can use the overflow property when you want to have better control of the layout. Try to change the overflow property to: visible, hidden, auto, or inherit and see what happens. The default value is visible. > +You can use the overflow property when you want to have better control of the layout. Try to change the overflow property to: visible, hidden, auto, or inherit and see what happens. The default value is visible. > +You can use the overflow property when you want to have better control of the layout. Try to change the overflow property to: visible, hidden, auto, or inherit and see what happens. The default value is visible. > +You can use the overflow property when you want to have better control of the layout. Try to change the overflow property to: visible, hidden, auto, or inherit and see what happens. The default value is visible. > +You can use the overflow property when you want to have better control of the layout. Try to change the overflow property to: visible, hidden, auto, or inherit and see what happens. The default value is visible. > +You can use the overflow property when you want to have better control of the layout. Try to change the overflow property to: visible, hidden, auto, or inherit and see what happens. The default value is visible.</div> > +<iframe id="content2" name="control" src="testiframe2.html"> </iframe> > +<div id="footer"></div> > +</body> > +</html> > \ No newline at end of file > Index: WebKit/qt/tests/qwebframe/tst_qwebframe.cpp > =================================================================== > --- WebKit/qt/tests/qwebframe/tst_qwebframe.cpp (revision 52259) > +++ WebKit/qt/tests/qwebframe/tst_qwebframe.cpp (working copy) > @@ -576,6 +576,7 @@ private slots: > void scrollPosition(); > void evaluateWillCauseRepaint(); > void qObjectWrapperWithSameIdentity(); > + void scrollRecursively(); > > private: > QString evalJS(const QString&s) { > @@ -2795,5 +2796,69 @@ void tst_QWebFrame::qObjectWrapperWithSa > QCOMPARE(mainFrame->toPlainText(), QString("test2")); > } > > +void tst_QWebFrame::scrollRecursively() > +{ > + // The test content is > + // a nested frame set > + // The main frame scrolls > + // and has two children > + // an iframe and a div overflow > + // both scroll > + QWebView webView; > + QWebPage* webPage = webView.page(); > + QSignalSpy loadSpy(webPage, SIGNAL(loadFinished(bool))); > + QUrl url = QUrl("qrc:///testiframe.html"); > + webPage->mainFrame()->load(url); > + QTRY_COMPARE(loadSpy.count(), 1); > + > + QList<QWebFrame*> children = webPage->mainFrame()->childFrames(); > + QVERIFY(children.count() == 1); > + > + // 1st test > + // call scrollRecursively over mainframe > + // verify scrolled > + // verify scroll postion changed > + QPoint scrollPosition(webPage->mainFrame()->scrollPosition()); > + QVERIFY(webPage->mainFrame()->scrollRecursively(10, 10)); > + QVERIFY(scrollPosition != webPage->mainFrame()->scrollPosition()); > + > + // 2nd test > + // call scrollRecursively over child iframe > + // verify scrolled > + // verify child scroll position changed > + // verify parent's scroll position did not change > + scrollPosition = webPage->mainFrame()->scrollPosition(); > + QPoint childScrollPosition = children.at(0)->scrollPosition(); > + QVERIFY(children.at(0)->scrollRecursively(10, 10)); > + QVERIFY(scrollPosition == webPage->mainFrame()->scrollPosition()); > + QVERIFY(childScrollPosition != children.at(0)->scrollPosition()); > + > + // 3rd test > + // call scrollRecursively over div overflow > + // verify scrolled == true > + // verify parent and child frame's scroll postion did not change > + QWebElement div = webPage->mainFrame()->documentElement().findFirst("#content1"); > + QMouseEvent evpres(QEvent::MouseMove, div.geometry().center(), Qt::NoButton, Qt::NoButton, Qt::NoModifier); > + webPage->event(&evpres); > + scrollPosition = webPage->mainFrame()->scrollPosition(); > + childScrollPosition = children.at(0)->scrollPosition(); > + QVERIFY(webPage->mainFrame()->scrollRecursively(5, 5)); > + QVERIFY(childScrollPosition == children.at(0)->scrollPosition()); > + QVERIFY(scrollPosition == webPage->mainFrame()->scrollPosition()); > + > + // 4th test > + // call scrollRecursively twice over childs iframe > + // verify scrolled == true first time > + // verify parent's scroll == true second time > + // verify parent and childs scroll position changed > + childScrollPosition = children.at(0)->scrollPosition(); > + QVERIFY(children.at(0)->scrollRecursively(-10, -10)); > + QVERIFY(childScrollPosition != children.at(0)->scrollPosition()); > + scrollPosition = webPage->mainFrame()->scrollPosition(); > + QVERIFY(children.at(0)->scrollRecursively(-10, -10)); > + QVERIFY(scrollPosition != webPage->mainFrame()->scrollPosition()); > + > +} > + > QTEST_MAIN(tst_QWebFrame) > #include "tst_qwebframe.moc"

Created attachment 45086 [details] Patch to add new QWebFrame API scrollRecursively

style-queue ran check-webkit-style on attachment 45086 [details] without any errors.

> at least I'm consistent :) Thanks for fixing the style nits. :)

*** Bug 26934 has been marked as a duplicate of this bug. ***

Comment on attachment 45086 [details] Patch to add new QWebFrame API scrollRecursively Clearing flags on attachment: 45086 Committed r52311: <http://trac.webkit.org/changeset/52311>

All reviewed patches have been landed. Closing bug.

i am late here ( vacations) nits: joe, i would had added a break point (".") at the end of "+ \since 4.7 + Scrolls nested frames starting at this frame, \a dx pixels to the right + and \a dy pixels downward. Both \a dx and \a dy may be negative. First attempts + to scroll elements with CSS overflow followed by this frame. If this + frame doesn't scroll, attempts to scroll the parent" and documented the what true/false means in terms of returned value.

Joe, would you mind prepare a version of this patch for 4.6 branch with private APIs (qt_scrollRecursively, qt_scrollOverflow) ? Thanks.

Created attachment 46140 [details] private API prepared to merge to 4.6

(In reply to comment #15) > Created an attachment (id=46140) [details] > private API prepared to merge to 4.6 The auto test included in this patch fails for me: FAIL! : tst_QWebFrame::qtscrollRecursively() 'children.count() == 1' returned FALSE. () Also I don't think we should use qt_ as prefix for the private API as that might conflict with private API that qt exports. I would refer qtwebkit_webframe_scrollRecursively(). Also the qt_privateScrollOverflow function should be marked as static and it does not need a qt_ prefix.

Created attachment 46274 [details] Patch for private API

Added commit 236bb65bb77eac6f668fd9fbf53d85e58b7d291a that cherry-picks 52311 and then commit 39b1a3d826dce489d2f5c99282ef0c30b5dbb9f6 on top that fixes the private API.