Bug 138525 - REGRESSION (r173484): Reducing content of scrollable region does not reset scroll position
Summary: REGRESSION (r173484): Reducing content of scrollable region does not reset sc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
: 140923 140924 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-11-07 15:30 PST by Joseph Pecoraro
Modified: 2021-08-07 11:06 PDT (History)
12 users (show)

See Also:


Attachments
[TEST] Reduction (4.00 KB, text/html)
2014-11-07 15:31 PST, Joseph Pecoraro
no flags Details
Patch (1.65 KB, patch)
2015-03-27 15:51 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch with test (9.13 KB, patch)
2015-03-27 17:36 PDT, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff
Patch (13.69 KB, patch)
2015-03-30 16:54 PDT, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2014-11-07 15:30:37 PST
* 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.
Comment 1 Joseph Pecoraro 2014-11-07 15:30:55 PST
I was at r175758.
Comment 2 Joseph Pecoraro 2014-11-07 15:31:07 PST
Created attachment 241215 [details]
[TEST] Reduction
Comment 3 Radar WebKit Bug Importer 2014-11-07 15:31:11 PST
<rdar://problem/18914565>
Comment 4 Radar WebKit Bug Importer 2014-11-07 15:31:12 PST
<rdar://problem/18914560>
Comment 5 Simon Fraser (smfr) 2014-11-09 11:14:51 PST
I wasn't able to reproduce this.
Comment 6 Joseph Pecoraro 2014-11-10 19:00:03 PST
I can reproduce 100%. It seems best to scroll almost all the way down but not quite all the way.
Comment 7 Simon Fraser (smfr) 2014-11-10 20:26:41 PST
I tried on Mavericks and Yosemite and couldn't repro on either.
Comment 8 Simon Fraser (smfr) 2014-11-10 20:29:01 PST
Ah, it doesn't reproduce if you've used a mouse wheel to scroll, only if you've trackpad-scrolled.
Comment 9 Simon Fraser (smfr) 2014-11-10 20:36:26 PST
Regressed with http://trac.webkit.org/changeset/173484
Comment 10 Beth Dakin 2015-01-14 15:24:50 PST
This doesn't repro for me anymore. Does it for you, Joe?
Comment 11 Beth Dakin 2015-01-14 15:51:45 PST
Ah ha! It does reproduce if you flick enough to allow the gesture to decelerate.
Comment 12 Joseph Pecoraro 2015-01-14 15:53:38 PST
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.
Comment 13 Simon Fraser (smfr) 2015-01-21 17:30:25 PST
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.
Comment 14 Nikita Vasilyev 2015-02-08 19:39:08 PST
*** Bug 140923 has been marked as a duplicate of this bug. ***
Comment 15 Timothy Hatcher 2015-02-28 15:18:10 PST
*** Bug 140924 has been marked as a duplicate of this bug. ***
Comment 16 Beth Dakin 2015-03-27 14:28:05 PDT
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…
Comment 17 Beth Dakin 2015-03-27 15:51:00 PDT
Created attachment 249616 [details]
Patch
Comment 18 Simon Fraser (smfr) 2015-03-27 15:59:04 PDT
Comment on attachment 249616 [details]
Patch

I think this should be easily testable, no?
Comment 19 Beth Dakin 2015-03-27 16:02:13 PDT
(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.
Comment 20 Beth Dakin 2015-03-27 17:36:25 PDT
Created attachment 249625 [details]
Patch with test
Comment 21 Simon Fraser (smfr) 2015-03-30 11:49:21 PDT
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.
Comment 22 Beth Dakin 2015-03-30 16:54:44 PDT
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.
Comment 23 Simon Fraser (smfr) 2015-03-30 17:06:16 PDT
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!
Comment 24 Beth Dakin 2015-03-31 11:28:13 PDT
(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!!