Bug 28023

Created attachment 34157 [details] Another way to Scroll

Comment on attachment 34157 [details] Another way to Scroll Why are you scrolling the enclosing box here? This makes me think that scrollByRecursively isn't getting on the right renderer.

meant to say "isn't getting called on the right renderer"

I was testing on Google Reader, and this was the case that caused the mouse wheel to scroll, I figured this might be a useful case to add. I got this code from EventHandler::scrollAndAcceptEvent.

scrollAndAcceptEvent gets called after the first node with a renderer is found. So it makes sense that that renderer might not be scrollable. But scrollByRecursively is called on a RenderLayer and is supposed to scroll that layer and be recursively called on other layers in the chain. Adding this case is not correct. Either we should be calling scrollByRecursively on a different starting layer, or its not being called recursively on the right layers.

Created attachment 34214 [details] Scroll Fixes

Comment on attachment 34214 [details] Scroll Fixes How does the addition of canBeScrolledAndHasScrollableArea work for frames? I mentioned offline to you that the recursion logic in scrollByRecursively should match the recursion logic in scrollRectToVisible. Is that true now?

For frames, this won't change the current behavior. It will scroll in frames, but then won't jump out of them (making it do that would be a second patch). I need to clean up the end of the recursion, and will be posting another patch for that, but from what I can tell, the recursion that we have works very similarly to scrollRectToVisible, except that we don't jump out of frames like they do.

Comment on attachment 34214 [details] Scroll Fixes LGTM, except this needs tests. Layout tests would be *strongly* preferred (and should be possible with eventSender!). But a WebCore/manual-test would be fine too. Thanks for the change. Please re-post with tests and I (or anyone else) will be happy to r+ this!

Created attachment 34328 [details] Scroll Fixes + Test

Created attachment 34330 [details] Scroll Fixes + Test + Newline

Comment on attachment 34330 [details] Scroll Fixes + Test + Newline Did you investigate making an automated test? I really wouldn't think it would be very hard (but I may just be naive). You just add a little <script> tag to your existing test, which calls eventSender.mouseDown(2) and then eventSender.mouseMoveTo(x, y). But maybe pan scroll events wouldn't work in DRT? Either way it seems we should have an explanation in a bug of why we can't test pan scrolling with the intention of fixing it in DRT some day.

I saw there was a pan-scrolling test in manual-tests, so I was following that idea. I see a few autoscroll tests, so I will try to make mine match that, and work as an automated test.

Created attachment 34495 [details] Scroll Fixes + Attempted Automated Test

Comment on attachment 34495 [details] Scroll Fixes + Attempted Automated Test I think the test isn't working because Windows DRT doesn't support the button number parameter to eventSender.mouseDown/mouseUp. See <http://trac.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/win/EventSender.cpp?rev=43762#L147>. This would be easy to fix!

Created attachment 34571 [details] Scroll Fixes + Automated Test

Created attachment 34572 [details] Fixed ChangeLogs

Created attachment 34574 [details] Scroll Fixes + Automated Test

Comment on attachment 34574 [details] Scroll Fixes + Automated Test Looks fine. We need a bug report for: 1033 // FIXME: If we didn't scroll the whole way, do we want to try looking at the frames ownerElement?

Comment on attachment 34574 [details] Scroll Fixes + Automated Test Removing cq+ per request.

Committed in http://trac.webkit.org/changeset/47163.