| Summary: | REGRESSION(r103245): can't scroll left/up using scrollbar controls of overflowing elements | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tony Chang <tony> | ||||||||||||
| Component: | Layout and Rendering | Assignee: | 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
Tony Chang
2012-01-13 15:22:47 PST
Created attachment 122507 [details]
test case
Huh, this only seems to not work on Linux. It works for me on Chromium Win, Chromium Mac and Safari Mac. 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? Created attachment 123586 [details]
Patch
I verified that this patch does not create any additional ScrollAnimators on the html5 spec. Comment on attachment 123586 [details]
Patch
Is this the only practical way to fix this? Special-casing 0 does not seem right.
(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. 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. Created attachment 123605 [details]
Patch
(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. Created attachment 123791 [details]
Patch
(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 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.
Created attachment 123807 [details]
Patch for landing
(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 on attachment 123807 [details] Patch for landing Clearing flags on attachment: 123807 Committed r105820: <http://trac.webkit.org/changeset/105820> All reviewed patches have been landed. Closing bug. |