Bug 76317 – REGRESSION(r103245): can't scroll left/up using scrollbar controls of overflowing elements

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Tony Chang]

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.

[Tony Chang]

Created attachment 122507 [details]
test case

[Tony Chang]

Huh, this only seems to not work on Linux.  It works for me on Chromium Win, Chromium Mac and Safari Mac.

[Tony Chang]

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]

Created attachment 123586 [details]
Patch

[Tony Chang]

I verified that this patch does not create any additional ScrollAnimators on the html5 spec.

[Darin Adler]

Comment on attachment 123586 [details]
Patch

Is this the only practical way to fix this? Special-casing 0 does not seem right.

[Tony Chang]

(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]

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]

Created attachment 123605 [details]
Patch

[Tony Chang]

(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]

Created attachment 123791 [details]
Patch

[Tony Chang]

(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 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]

Created attachment 123807 [details]
Patch for landing

[Tony Chang]

(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 on attachment 123807 [details]
Patch for landing

Clearing flags on attachment: 123807

Committed r105820: <http://trac.webkit.org/changeset/105820>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.