Bug 26934

Created attachment 32197 [details] Proposed patch, for adding scrolling to QWebElement I'm not sure what is the correct API, but something like this would be useful for me anyway.

@joseph: patch looks ok to me (not a reviewer). some comments: 1) NIT: remove dup code. + if (!m_element) + return false; + + if (!m_element) + return false; 2) About the API, in my opinion exposing just a *ByPixel method is weak. IMO, patch should add methods for: *DoScroll(direction) -> where is would scroll "one unit" in the given direction ... e.g. it simulates arrow keypress * ScrollByPage (direction) -> it simulates a pageUp/Down as well as the already done *ByPixel. (...) + QVERIFY(div.scrollByPixel(10,0)); + QVERIFY(div.scrollByPixel(0,10)); + QVERIFY(div.scrollByPixel(-10,-10)); + QVERIFY(!div.scrollByPixel(-10,-10)); + QVERIFY(!p.scrollByPixel(10,10)); (...) e.g. code above looks like a panning implementation (which is ok), but limits API to this. 3) what about content w/ overflow:hidden like gmail main page. Does your scroll method work ?

@tonikitoo: I dont like making so many functions, it would be better with an enum, but actually this API should be the same as for QWebFrame etc. I think the function there is just called ScrollBy, but you can have a look.

I looked again at the scrolling methods RenderLayer. The options are: panScrollFromPoint scrollByRecursively scrollXOffset scrollYOffset scrollToOffset scrollToXOffset scrollToYOffset scrollRectToVisible scroll: this method takes the granularity enum and is what the patch is using. It looks like adding scrollToOffset and panScrollFromPoint to WebElement would make sense. Adding the enum to the APi might be difficult to maintain in the future.

(In reply to comment #3) > @tonikitoo: I dont like making so many functions, it would be better with an > enum, but actually this API should be the same as for QWebFrame etc. I think > the function there is just called ScrollBy, but you can have a look. QWebFrame has a scroll(int, int) method. I guess this is the same thing as scrollToOffset.

Joe, this looks interesting. Can you explain your use-case a bit?

(In reply to comment #6) > Joe, this looks interesting. Can you explain your use-case a bit? Thanks for your comments. This new api provides clients with a method to pan scroll elements with the overflow scroll style set. One live use case can be seen if you navigate to the gmail - create a account page, scroll down to the terms of service div.

Comment on attachment 32197 [details] Proposed patch, for adding scrolling to QWebElement And how would a client use this exactly? I'm not convinced that it is necessary. At Torch we have a client that uses QtWebKit that has automatic panning on a touch screen. We didn't need this API to accomplish it. More than happy to have this if a real use-case can be demonstrated. r- to get it out of review queue until such use case is understood and because of the concerns with scrollByPixel API from other comments.

(In reply to comment #8) > (From update of attachment 32197 [details]) > And how would a client use this exactly? I'm not convinced that it is > necessary. At Torch we have a client that uses QtWebKit that has automatic > panning on a touch screen. We didn't need this API to accomplish it. > > More than happy to have this if a real use-case can be demonstrated. r- to get > it out of review queue until such use case is understood and because of the > concerns with scrollByPixel API from other comments. This is the element in question: .content { position: absolute; top: 50px; overflow-x: hidden; overflow-y: auto; height: 532px; width: 360px; } Also fails if only one "overflow: auto;" -property is used instead of -x and -y, no big difference there.

Created attachment 44167 [details] Proposed patch, for adding scrolling to QWebElement Modified scroll API to be the same as QWebFrame. Use case in comment #9

Comment on attachment 44167 [details] Proposed patch, for adding scrolling to QWebElement > + Scrollbar* hsb = layer->horizontalScrollbar(); > + Scrollbar* vsb = layer->verticalScrollbar(); > + return ( vsb || hsb ); What is scrollbars were disabled via the settings? > + > + if (dx > 0) > + scrolled = layer->scroll(ScrollRight, ScrollByPixel, qAbs(dx)); qAbs doesn't seem needed in this case > + else if (dx < 0) > + scrolled = layer->scroll(ScrollLeft, ScrollByPixel, qAbs(dx)); maybe do scrolled = layer->scroll((dx < 0) ? ScrollLeft : ScrollRight, ScrollByPixel, qAbs(dx)); ?

Our other api's don't have isScrollable() methods. Would it be possible to add a contentsSize instead? you can compare it with the geometry().size()

Comment on attachment 44167 [details] Proposed patch, for adding scrolling to QWebElement Hmm, does this really tell that the element itself is scrollbar or its enclosing layer? I wonder if it may be a good idea to make it a bit clearer in the API that this isn't really a feature of the element itself but it's encoding layer. This could be reflected in the method name for example.

Created attachment 44612 [details] Proposed patch, for adding scrolling to QWebElement I removed the methods isScrollable() and contentsSize from the patch. The scroll method informs the client if the QWebElement can scroll and for my use case that is all that is needed. If it's important to add contentsSize I can propose another patch as well. Thanks.

This general idea of this use case is resolved in https://bugs.webkit.org/show_bug.cgi?id=32668 *** This bug has been marked as a duplicate of bug 32668 ***