Bug 28023 – Scrolling with middle mouse button doesn't work in Expanded view on reader.google.com

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brian Weinstein]

- Click any article under RSS subscriptions
- Click Expanded-List in the right top corner
- try pan-scrolling

Expected: The page pan scrolls.
Actual: It shows the icon, but doesn't.

<rdar://6622771>

[Brian Weinstein]

Created attachment 34157 [details]
Another way to Scroll

[Adele Peterson]

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.

[Adele Peterson]

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

[Brian Weinstein]

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.

[Adele Peterson]

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.

[Brian Weinstein]

Created attachment 34214 [details]
Scroll Fixes

[Adele Peterson]

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?

[Brian Weinstein]

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.

[Eric Seidel (no email)]

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!

[Brian Weinstein]

Created attachment 34328 [details]
Scroll Fixes + Test

[Brian Weinstein]

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

[Eric Seidel (no email)]

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.

[Brian Weinstein]

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.

[Brian Weinstein]

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

[Adam Roben (:aroben)]

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!

[Brian Weinstein]

Created attachment 34571 [details]
Scroll Fixes + Automated Test

[Brian Weinstein]

Created attachment 34572 [details]
Fixed ChangeLogs

[Brian Weinstein]

Created attachment 34574 [details]
Scroll Fixes + Automated Test

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

Comment on attachment 34574 [details]
Scroll Fixes + Automated Test

Removing cq+ per request.

[Brian Weinstein]

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