Bug 76317 - REGRESSION(r103245): can't scroll left/up using scrollbar controls of overflowing elements
Summary: REGRESSION(r103245): can't scroll left/up using scrollbar controls of overflo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on: 76986
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-13 15:22 PST by Tony Chang
Modified: 2012-01-24 23:55 PST (History)
6 users (show)

See Also:


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 Details | Formatted Diff | Diff
Patch (6.11 KB, patch)
2012-01-23 13:06 PST, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (6.24 KB, patch)
2012-01-24 13:20 PST, Tony Chang
no flags Details | Formatted Diff | Diff
Patch for landing (6.51 KB, patch)
2012-01-24 14:17 PST, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 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.
Comment 1 Tony Chang 2012-01-13 15:23:15 PST
Created attachment 122507 [details]
test case
Comment 2 Tony Chang 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.
Comment 3 Tony Chang 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?
Comment 4 Tony Chang 2012-01-23 11:36:47 PST
Created attachment 123586 [details]
Patch
Comment 5 Tony Chang 2012-01-23 11:37:19 PST
I verified that this patch does not create any additional ScrollAnimators on the html5 spec.
Comment 6 Darin Adler 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.
Comment 7 Tony Chang 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.
Comment 8 Tony Chang 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.
Comment 9 Tony Chang 2012-01-23 13:06:05 PST
Created attachment 123605 [details]
Patch
Comment 10 Tony Chang 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.
Comment 11 Tony Chang 2012-01-24 13:20:30 PST
Created attachment 123791 [details]
Patch
Comment 12 Tony Chang 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?
Comment 13 Darin Adler 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.
Comment 14 Tony Chang 2012-01-24 14:17:42 PST
Created attachment 123807 [details]
Patch for landing
Comment 15 Tony Chang 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.
Comment 16 WebKit Review Bot 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>
Comment 17 WebKit Review Bot 2012-01-24 15:21:30 PST
All reviewed patches have been landed.  Closing bug.