Bug 138525 – REGRESSION (r173484): Reducing content of scrollable region does not reset scroll position

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
If you scroll a scrollable region, then change the content of the scrollable region to very little content that is no longer scrollable, the region should scroll itself back to make the new content visible. This is no longer happening.

* STEPS
1. Open Attached reduction
2. Scroll the content area a bit
3. Click the button to change the content in the scrollable region
  => No new content is visible, it should be

* NOTES
- Checking in the inspector, $0.scrollTop is some >0 value. Where before it used to be 0 after clicking the button.

[Joseph Pecoraro]

I was at r175758.

[Joseph Pecoraro]

Created attachment 241215 [details]
[TEST] Reduction

[Radar WebKit Bug Importer]

<rdar://problem/18914565>

[Radar WebKit Bug Importer]

<rdar://problem/18914560>

[Simon Fraser (smfr)]

I wasn't able to reproduce this.

[Joseph Pecoraro]

I can reproduce 100%. It seems best to scroll almost all the way down but not quite all the way.

[Simon Fraser (smfr)]

I tried on Mavericks and Yosemite and couldn't repro on either.

[Simon Fraser (smfr)]

Ah, it doesn't reproduce if you've used a mouse wheel to scroll, only if you've trackpad-scrolled.

[Simon Fraser (smfr)]

Regressed with http://trac.webkit.org/changeset/173484

[Beth Dakin]

This doesn't repro for me anymore. Does it for you, Joe?

[Beth Dakin]

Ah ha! It does reproduce if you flick enough to allow the gesture to decelerate.

[Joseph Pecoraro]

In Web Inspector this happened in the "Computed" section of the Styles sidebar. Have "Show All" properties checked, scroll down in this fashion, then uncheck "Show All" and the sidebar is blank instead of showing something.

[Simon Fraser (smfr)]

We think we're rubber banding all the time, because RenderLayer::overhangAmount() returns a non-zero size because we're asking it before we apply any clamping.

[Nikita Vasilyev]

*** Bug 140923 has been marked as a duplicate of this bug. ***

[Timothy Hatcher]

*** Bug 140924 has been marked as a duplicate of this bug. ***

[Beth Dakin]

If I remove the call to isRubberBandInProgress() from RenderLayer::updateScrollInfoAfterLayout(), which was added by https://bugs.webkit.org/show_bug.cgi?id=136650 then this bug goes away. The tricky thing is that it seems like that original bug (also about scrolling in the inspector) does not reproduce anymore with this fix taken out, which probably means that either the inspector is no longer updating layout on scroll, or maybe Hyatt's recent offsetWidth optimizations are preventing those layouts entirely.

So I do have a simple fix for this bug, but I am worried that it might cause the symptoms described in https://bugs.webkit.org/show_bug.cgi?id=136650 to reappear in some content. I just haven't found that content yet…

[Beth Dakin]

Created attachment 249616 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 249616 [details]
Patch

I think this should be easily testable, no?

[Beth Dakin]

(In reply to comment #18)
> Comment on attachment 249616 [details]
> Patch
> 
> I think this should be easily testable, no?

The bug does not reproduce with a programatic scroll. Brent did add a way to momentum scroll in tests, so I might be able to make one of those tests reproduce the bug. I'll take a look.

[Beth Dakin]

Created attachment 249625 [details]
Patch with test

[Simon Fraser (smfr)]

Comment on attachment 249625 [details]
Patch with test

View in context: https://bugs.webkit.org/attachment.cgi?id=249625&action=review

> Source/WebCore/ChangeLog:4
> +        REGRESSION (r173484): Reducing content of scrollable region does not reset scroll 
> +        position

Unfortunate wrapping.

> LayoutTests/ChangeLog:4
> +        REGRESSION (r173484): Reducing content of scrollable region does not reset scroll 
> +        position

Unfortunate wrapping.

[Beth Dakin]

Created attachment 249782 [details]
Patch

Thanks Simon, but I did end up finding a regression with that patch. This patch takes a totally new approach and adds an additional test.

[Simon Fraser (smfr)]

Comment on attachment 249782 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249782&action=review

> LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/overflow-scroll-reduced-content.html:84
> +            <h1>Lots and lots of content</h1>
> +            <p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nulla tincidunt vehicula lectus, in blandit nisl tempus eget. Suspendisse dapibus, nibh vitae scelerisque aliquet, elit leo pharetra risus, in convallis turpis magna at lorem. In placerat porta justo, eu euismod ex. Donec ultricies velit sed sollicitudin iaculis. Proin congue venenatis ipsum, iaculis bibendum felis elementum ac. Curabitur sed rhoncus elit. Aenean sit amet posuere arcu. Etiam eget tellus erat. Aliquam erat volutpat. Aliquam ornare ex et sollicitudin pulvinar. Cras elit orci, fermentum nec augue ac, viverra ultricies mi. Nam at erat aliquet, dapibus nibh non, interdum massa. Nam ullamcorper, sapien id dictum efficitur, leo ante porttitor nisl, sit amet consequat lectus velit quis nibh.</p>
> +            <p>Vestibulum dapibus ante sit amet mauris vulputate, non congue sem pretium. Integer nec gravida risus. Suspendisse quis nibh ultricies, ultricies tellus in, interdum dolor. Nunc feugiat, eros nec elementum porttitor, lorem enim dictum ligula, vitae volutpat elit mi ut diam. Donec at lectus quis risus dapibus sollicitudin. Morbi porta, nisi vel pellentesque fermentum, nisl felis placerat velit, id tempor nisi lectus eget dolor. Interdum et malesuada fames ac ante ipsum primis in faucibus. Curabitur condimentum nisl id libero cursus, at egestas elit semper. Mauris lorem dui, rutrum id eros eu, cursus vestibulum odio. Etiam vel libero et nunc sodales commodo feugiat gravida tortor. Sed nisi leo, feugiat ac luctus et, faucibus non magna.</p>
> +            <p>Proin viverra cursus magna, aliquam auctor mauris mollis ac. Sed varius felis ex, et tempor nisl lobortis vitae. Suspendisse augue ante, pharetra quis purus et, pulvinar pharetra nibh. Etiam aliquet cursus sapien, at viverra dolor venenatis ac. Mauris semper bibendum urna sed tempor. Suspendisse placerat, mauris at tristique ultricies, risus libero congue nunc, id vulputate ligula arcu id eros. Vestibulum et porta ante. Mauris non dolor justo. Curabitur tincidunt diam id ipsum rhoncus tempor vel eget augue.</p>
> +            <p>Curabitur id tempus neque, ac facilisis risus. Aenean aliquam elementum tellus, in imperdiet mauris maximus sed. Morbi ac mi id nisl tincidunt viverra ac sed magna. Aliquam erat volutpat. Nulla efficitur risus sapien, sit amet vehicula felis bibendum sed. Nunc eget venenatis lectus. Pellentesque id tempor enim, sit amet suscipit dui. Suspendisse tempus tellus mi, mattis posuere ante pharetra quis. Cras pharetra quis massa id sollicitudin. Nunc vel nisi tempus, efficitur lacus at, molestie sem. Nulla elit ante, malesuada nec ligula ac, iaculis auctor risus. Donec hendrerit mauris sem, eu pellentesque mi sagittis id. Aliquam erat volutpat. Fusce ac tincidunt quam. Sed eget lacus eu erat ultrices pretium.</p>
> +            <p>In finibus elit nec ligula eleifend tincidunt. Nunc vel sapien posuere, venenatis erat vel, pulvinar metus. Vestibulum at augue elit. Nunc vehicula vel nunc nec fringilla. Quisque vel quam magna. Aenean scelerisque, mauris sed mollis commodo, lacus mi congue sem, pretium elementum dui odio non mauris. Quisque sed vestibulum lectus. Nullam ante ante, vulputate vitae mattis eu, ultrices non odio. Cras faucibus sodales ligula, id rutrum neque dictum sed. Sed molestie risus in metus dictum ornare. Maecenas consectetur elit mi, at suscipit tellus iaculis ut. Ut tempus leo eget dui viverra vehicula.</p>
> +        </div>

Could this just be style="height: 1000px"?

> LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/overflow-scroll-zero-delta-wheel-events.html:54
> +    setTimeout(checkForZero, 500);

Please don't add a test that takes a half second!

[Beth Dakin]

(In reply to comment #23)
> Comment on attachment 249782 [details]
> Patch
> Could this just be style="height: 1000px"?
> 

Yes, fixed!

> > LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/overflow-scroll-zero-delta-wheel-events.html:54
> > +    setTimeout(checkForZero, 500);
> 
> Please don't add a test that takes a half second!

I was able to reduce it to 100, which is not great either. We should have a different mechanism I suppose. Hopefully the test won't be flaky.

http://trac.webkit.org/changeset/182191

Thanks!!