Bug 28023

Summary: Scrolling with middle mouse button doesn't work in Expanded view on reader.google.com
Product: WebKit Reporter: Brian Weinstein <bweinstein>
Component: WebCore Misc.Assignee: Brian Weinstein <bweinstein>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, aroben
Priority: P2 Keywords: InRadar, PlatformOnly
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
Bug Depends on: 28166    
Bug Blocks: 27850    
Attachments:
Description Flags
Another way to Scroll
adele: review-
Scroll Fixes
eric: review-
Scroll Fixes + Test
none
Scroll Fixes + Test + Newline
none
Scroll Fixes + Attempted Automated Test
none
Scroll Fixes + Automated Test
none
Fixed ChangeLogs
none
Scroll Fixes + Automated Test eric: review+, eric: commit-queue-

Description Brian Weinstein 2009-08-05 11:38:55 PDT
- 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>
Comment 1 Brian Weinstein 2009-08-05 11:44:47 PDT
Created attachment 34157 [details]
Another way to Scroll
Comment 2 Adele Peterson 2009-08-05 11:54:46 PDT
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.
Comment 3 Adele Peterson 2009-08-05 11:55:18 PDT
meant to say "isn't getting called on the right renderer"
Comment 4 Brian Weinstein 2009-08-05 11:57:22 PDT
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.
Comment 5 Adele Peterson 2009-08-05 12:04:36 PDT
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.
Comment 6 Brian Weinstein 2009-08-06 11:23:33 PDT
Created attachment 34214 [details]
Scroll Fixes
Comment 7 Adele Peterson 2009-08-06 12:45:12 PDT
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?
Comment 8 Brian Weinstein 2009-08-06 13:52:19 PDT
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 9 Eric Seidel (no email) 2009-08-07 08:47:27 PDT
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!
Comment 10 Brian Weinstein 2009-08-07 14:17:32 PDT
Created attachment 34328 [details]
Scroll Fixes + Test
Comment 11 Brian Weinstein 2009-08-07 14:21:40 PDT
Created attachment 34330 [details]
Scroll Fixes + Test + Newline
Comment 12 Eric Seidel (no email) 2009-08-08 08:56:32 PDT
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.
Comment 13 Brian Weinstein 2009-08-08 14:06:58 PDT
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.
Comment 14 Brian Weinstein 2009-08-10 11:52:12 PDT
Created attachment 34495 [details]
Scroll Fixes + Attempted Automated Test
Comment 15 Adam Roben (:aroben) 2009-08-10 12:01:30 PDT
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!
Comment 16 Brian Weinstein 2009-08-11 10:05:22 PDT
Created attachment 34571 [details]
Scroll Fixes + Automated Test
Comment 17 Brian Weinstein 2009-08-11 10:11:26 PDT
Created attachment 34572 [details]
Fixed ChangeLogs
Comment 18 Brian Weinstein 2009-08-11 10:13:53 PDT
Created attachment 34574 [details]
Scroll Fixes + Automated Test
Comment 19 Eric Seidel (no email) 2009-08-12 15:54:08 PDT
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 20 Eric Seidel (no email) 2009-08-12 16:11:16 PDT
Comment on attachment 34574 [details]
Scroll Fixes + Automated Test

Removing cq+ per request.
Comment 21 Brian Weinstein 2009-08-12 16:57:37 PDT
Committed in http://trac.webkit.org/changeset/47163.