Bug 76317

Summary: REGRESSION(r103245): can't scroll left/up using scrollbar controls of overflowing elements
Product: WebKit Reporter: Tony Chang <tony>
Component: Layout and RenderingAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, darin, kling, tonikitoo, webkit.review.bot, xji
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Linux   
Bug Depends on: 76986    
Bug Blocks:    
Attachments:
Description Flags
test case
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Tony Chang
Reported 2012-01-13 15:22:47 PST
I'll attach a test case demonstrating the problem. It seems like internally we think that the scroll offset is the top left of the block.
Attachments
test case (382 bytes, text/html)
2012-01-13 15:23 PST, Tony Chang 		no flags
Details
Patch (5.58 KB, patch)
2012-01-23 11:36 PST, Tony Chang 		no flags
DetailsFormatted DiffDiff
Patch (6.11 KB, patch)
2012-01-23 13:06 PST, Tony Chang 		no flags
DetailsFormatted DiffDiff
Patch (6.24 KB, patch)
2012-01-24 13:20 PST, Tony Chang 		no flags
DetailsFormatted DiffDiff
Patch for landing (6.51 KB, patch)
2012-01-24 14:17 PST, Tony Chang 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Tony Chang
Comment 1 2012-01-13 15:23:15 PST
Created attachment 122507 [details] test case
Tony Chang
Comment 2 2012-01-13 15:26:25 PST
Huh, this only seems to not work on Linux. It works for me on Chromium Win, Chromium Mac and Safari Mac.
Tony Chang
Comment 3 2012-01-19 14:23:29 PST
Ah, wrong again. I was using an old build of the other browsers. I'm now able to repro in Chromium Win and Safari Mac using ToT WebKit. This is a regression caused by bug 74830. ScrollAnimators are always created with an offset of (0,0), but not all ScrollableAreas start with an offset of (0,0). Maybe we should always call scrollToOffsetWithoutAnimation if scrollOffset != 0, 0?
Tony Chang
Comment 4 2012-01-23 11:36:47 PST
Created attachment 123586 [details] Patch
Tony Chang
Comment 5 2012-01-23 11:37:19 PST
I verified that this patch does not create any additional ScrollAnimators on the html5 spec.
Darin Adler
Comment 6 2012-01-23 11:38:38 PST
Comment on attachment 123586 [details] Patch Is this the only practical way to fix this? Special-casing 0 does not seem right.
Tony Chang
Comment 7 2012-01-23 11:52:27 PST
(In reply to comment #6) > (From update of attachment 123586 [details]) > Is this the only practical way to fix this? Special-casing 0 does not seem right. We could also check to see if the scroll origin is 0, 0. Would that be better? I also considered passing the initial offset to ScrollAnimator::create. This is tricky because ScrollAnimator is created from lots of different code paths in ScrollableArea. Not all of them know the initial offset.
Tony Chang
Comment 8 2012-01-23 11:57:18 PST
We might also be able to move the check from RenderLayer::scrollToOffset to RenderLayer::updateScrollInfoAfterLayout, but I'm not sure if that will cause us to lose the optimization for other calls to RenderLayer::scrollToOffset.
Tony Chang
Comment 9 2012-01-23 13:06:05 PST
Created attachment 123605 [details] Patch
Tony Chang
Comment 10 2012-01-23 13:07:22 PST
(In reply to comment #9) > Created an attachment (id=123605) [details] > Patch This duplicates the checking logic in scrollToOffset, but avoids checking against 0, 0. Also, I'll do a follow up patch to change scrollOffset to a LayoutPoint.
Tony Chang
Comment 11 2012-01-24 13:20:30 PST
Created attachment 123791 [details] Patch
Tony Chang
Comment 12 2012-01-24 13:21:18 PST
(In reply to comment #11) > Created an attachment (id=123791) [details] > Patch Just changed the test to not output scroll offsets since it will likely be different on different platforms. Darin, can you take a look?
Darin Adler
Comment 13 2012-01-24 13:50:32 PST
Comment on attachment 123791 [details] Patch We often use a technique where the failing results show the values but passing omits them. That makes it easier to diagnose problems while staying platform-independent in our expected results.
Tony Chang
Comment 14 2012-01-24 14:17:42 PST
Created attachment 123807 [details] Patch for landing
Tony Chang
Comment 15 2012-01-24 14:18:02 PST
(In reply to comment #13) > (From update of attachment 123791 [details]) > We often use a technique where the failing results show the values but passing omits them. That makes it easier to diagnose problems while staying platform-independent in our expected results. Good idea; done.
WebKit Review Bot
Comment 16 2012-01-24 15:21:24 PST
Comment on attachment 123807 [details] Patch for landing Clearing flags on attachment: 123807 Committed r105820: <http://trac.webkit.org/changeset/105820>
WebKit Review Bot
Comment 17 2012-01-24 15:21:30 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.