Bug 28023 - Scrolling with middle mouse button doesn't work in Expanded view on reader.google.com
Summary: Scrolling with middle mouse button doesn't work in Expanded view on reader.go...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Brian Weinstein
URL:
Keywords: InRadar, PlatformOnly
Depends on: 28166
Blocks: 27850
  Show dependency treegraph
 
Reported: 2009-08-05 11:38 PDT by Brian Weinstein
Modified: 2009-08-12 16:57 PDT (History)
2 users (show)

See Also:


Attachments
Another way to Scroll (2.63 KB, patch)
2009-08-05 11:44 PDT, Brian Weinstein
adele: review-
Details | Formatted Diff | Diff
Scroll Fixes (6.97 KB, patch)
2009-08-06 11:23 PDT, Brian Weinstein
eric: review-
Details | Formatted Diff | Diff
Scroll Fixes + Test (9.70 KB, patch)
2009-08-07 14:17 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
Scroll Fixes + Test + Newline (9.68 KB, patch)
2009-08-07 14:21 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
Scroll Fixes + Attempted Automated Test (11.43 KB, patch)
2009-08-10 11:52 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
Scroll Fixes + Automated Test (12.43 KB, patch)
2009-08-11 10:05 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
Fixed ChangeLogs (12.72 KB, patch)
2009-08-11 10:11 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
Scroll Fixes + Automated Test (12.72 KB, patch)
2009-08-11 10:13 PDT, Brian Weinstein
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.