Bug 89114 - REGRESSION (r112919): Setting scrollTop after setting display from none to block fails
Summary: REGRESSION (r112919): Setting scrollTop after setting display from none to bl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Beth Dakin
URL:
Keywords: InRadar
: 92239 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-06-14 11:56 PDT by Simon Fraser (smfr)
Modified: 2012-07-25 18:15 PDT (History)
9 users (show)

See Also:


Attachments
Testcase (573 bytes, text/html)
2012-06-14 13:17 PDT, Simon Fraser (smfr)
no flags Details
Patch (4.63 KB, patch)
2012-07-24 21:55 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 Simon Fraser (smfr) 2012-06-14 11:56:56 PDT
The attached testcase shows a bug in setting scrollTop after toggling display: from 'none' to 'block'. This regressed in http://trac.webkit.org/changeset/112919, bug 72852.
Comment 1 Simon Fraser (smfr) 2012-06-14 11:57:25 PDT
<rdar://problem/11656050>
Comment 2 Julien Chaffraix 2012-06-14 13:04:13 PDT
Simon, it looks like the test case didn't make it to the bug.
Comment 3 Simon Fraser (smfr) 2012-06-14 13:17:00 PDT
Created attachment 147637 [details]
Testcase
Comment 4 Julien Chaffraix 2012-06-14 13:31:47 PDT
I wonder if this is a regression from bug 72852.

Testing on a Chromium build from last week (WebKit revision 119830), I don't see the bug. Yet on today's Canary (WebKit r120277), I do.
Comment 5 Tim Horton 2012-06-14 13:33:45 PDT
(In reply to comment #4)
> I wonder if this is a regression from bug 72852.
> 
> Testing on a Chromium build from last week (WebKit revision 119830), I don't see the bug. Yet on today's Canary (WebKit r120277), I do.

I believe that has already been confirmed.
Comment 6 Rakesh 2012-06-15 02:56:17 PDT
I could not reproduce this issue on r120414.
Comment 7 Simon Fraser (smfr) 2012-06-19 11:03:10 PDT
I can still reproduce in a TOT Mac build (the testcase should say "after: 0" at the end.
Comment 8 Julien Chaffraix 2012-06-26 09:37:41 PDT
(In reply to comment #7)
> I can still reproduce in a TOT Mac build (the testcase should say "after: 0" at the end.

There seems to be some platform differences playing here. I couldn't reproduce the issue on Linux (using Qt or Chromium) but I saw it on Snow-Leopard (Chromium or Mac).
Comment 9 Beth Dakin 2012-07-24 19:00:47 PDT
I think I understand what's going on here, and I have an idea for a fix. Taking the bug.
Comment 10 Beth Dakin 2012-07-24 21:55:02 PDT
Created attachment 154247 [details]
Patch
Comment 11 mitz 2012-07-25 09:21:33 PDT
Comment on attachment 154247 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        ScrollAnimatorMac::immediateScrollTo() and ScrollAnimatorMac::immediateScrollTo() 

Did you mean a different function the second time?
Comment 12 Julien Chaffraix 2012-07-25 09:30:55 PDT
Comment on attachment 154247 [details]
Patch

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

Good catch!

> Source/WebCore/rendering/RenderLayer.cpp:200
>          m_scrollOffset = element->savedLayerScrollOffset();
> +        if (!m_scrollOffset.isZero())
> +            scrollAnimator()->setCurrentPosition(FloatPoint(m_scrollOffset.width(), m_scrollOffset.height()));

I think those lines should just be:

scrollToOffset(element->savedLayerScrollOffset());

The rationale being that we need to dispatch a 'scroll' event so that JS can also update its position. On top of that, it integrates more nicely in the existing code.

> LayoutTests/fast/overflow/setting-scrollTop-after-hide-show-expected.txt:1
> +before: 20

Could it be possible to dump more information about what is expected? The current result is difficult to read for people not familiar with this bug.

> LayoutTests/fast/overflow/setting-scrollTop-after-hide-show-expected.txt:3
> +after: 0

before / after don't really speak to me, especially since they refer to the final setting. Writting the following would be more readable:
* scrollTop after restoring it
* scrollTop after setting it back to 0

> LayoutTests/fast/overflow/setting-scrollTop-after-hide-show.html:23
> +    log('before: ' + a.scrollTop + '\n');

I think using our JS testing framework would make the expected output better (shouldBe("a.scrollTop", "20")) as it would document what you expect.
Comment 13 Beth Dakin 2012-07-25 10:53:38 PDT
(In reply to comment #11)
> (From update of attachment 154247 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154247&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        ScrollAnimatorMac::immediateScrollTo() and ScrollAnimatorMac::immediateScrollTo() 
> 
> Did you mean a different function the second time?

Oops! Yes, I meant immediateScrollBy for the second one. Will fix.
Comment 14 Beth Dakin 2012-07-25 11:03:15 PDT
(In reply to comment #12)
> (From update of attachment 154247 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154247&action=review
> 
> Good catch!
> 
> > Source/WebCore/rendering/RenderLayer.cpp:200
> >          m_scrollOffset = element->savedLayerScrollOffset();
> > +        if (!m_scrollOffset.isZero())
> > +            scrollAnimator()->setCurrentPosition(FloatPoint(m_scrollOffset.width(), m_scrollOffset.height()));
> 
> I think those lines should just be:
> 
> scrollToOffset(element->savedLayerScrollOffset());
> 
> The rationale being that we need to dispatch a 'scroll' event so that JS can also update its position. On top of that, it integrates more nicely in the existing code.
>

We definitely do NOT want to get rid of the if-statement. Otherwise will create ScrollAnimators for all layers with Element nodes. This has been a memory footprint issue that we have worked to control and reduce in the past. 

In terms of the JS argument…is there really a JS bug left here? I think my test case demonstrates that there is not a JS bug because when JS asks for the scrollTop, it will update layout, and everything will be right. I am happy to go with scrollToOffset() instead of setCurrentPosition() if there is actually a bug that would be fixed by that, but otherwise, scrollToOffset does a lot of work that does not seem necessary at this time.
 
> > LayoutTests/fast/overflow/setting-scrollTop-after-hide-show-expected.txt:1
> > +before: 20
> 
> Could it be possible to dump more information about what is expected? The current result is difficult to read for people not familiar with this bug.
> 
> > LayoutTests/fast/overflow/setting-scrollTop-after-hide-show-expected.txt:3
> > +after: 0
> 
> before / after don't really speak to me, especially since they refer to the final setting. Writting the following would be more readable:
> * scrollTop after restoring it
> * scrollTop after setting it back to 0
> 
> > LayoutTests/fast/overflow/setting-scrollTop-after-hide-show.html:23
> > +    log('before: ' + a.scrollTop + '\n');
> 

Yes, your suggestion is definitely better. Will fix.
Comment 15 Beth Dakin 2012-07-25 11:16:32 PDT
Committed with http://trac.webkit.org/changeset/123637

I will definitely change that setCurrentPosition() call to a scrollToOffset() if there is a bug leftover there. But I haven't found one yet.
Comment 16 Julien Chaffraix 2012-07-25 11:21:29 PDT
Comment on attachment 154247 [details]
Patch

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

>>> Source/WebCore/rendering/RenderLayer.cpp:200
>>> +            scrollAnimator()->setCurrentPosition(FloatPoint(m_scrollOffset.width(), m_scrollOffset.height()));
>> 
>> I think those lines should just be:
>> 
>> scrollToOffset(element->savedLayerScrollOffset());
>> 
>> The rationale being that we need to dispatch a 'scroll' event so that JS can also update its position. On top of that, it integrates more nicely in the existing code.
> 
> We definitely do NOT want to get rid of the if-statement. Otherwise will create ScrollAnimators for all layers with Element nodes. This has been a memory footprint issue that we have worked to control and reduce in the past. 
> 
> In terms of the JS argument…is there really a JS bug left here? I think my test case demonstrates that there is not a JS bug because when JS asks for the scrollTop, it will update layout, and everything will be right. I am happy to go with scrollToOffset() instead of setCurrentPosition() if there is actually a bug that would be fixed by that, but otherwise, scrollToOffset does a lot of work that does not seem necessary at this time.

You are totally right we don't want to create a ScrollAnimator for every layers. I would have expected the check in RenderLayer::scrollToOffset to catch the no-offset case and not calling into the ScrollableArea but I may have missed something.

The issue is not with the use case in this bug (ie changing the scrollOffset from JS). It's more about making the restore logic visible from JS through the 'scroll' event. Currently we don't dispatch a 'scroll' event when restoring the scrollOffset but I believe we should so that JS can update whatever representation it uses.
Comment 17 Beth Dakin 2012-07-25 11:37:53 PDT
(In reply to comment #16)
> 
> You are totally right we don't want to create a ScrollAnimator for every layers. I would have expected the check in RenderLayer::scrollToOffset to catch the no-offset case and not calling into the ScrollableArea but I may have missed something.
> 

Oh, you are right. scrollToOffset is smart about that. My bad!

> The issue is not with the use case in this bug (ie changing the scrollOffset from JS). It's more about making the restore logic visible from JS through the 'scroll' event. Currently we don't dispatch a 'scroll' event when restoring the scrollOffset but I believe we should so that JS can update whatever representation it uses.

I see. Hmm. But is this really necessary? It's kind of the JS's fault if it chose to change any of its internal scroll information. Because wasn't the whole point of the original change that caused this regression to make sure that that scroll information isn't lost so that it can be restored as it was? That implies to me that sites would have been broken BEFORE the original change, not NOW after these changes. 

I don't think it makes sense to send a scroll event here unless other browsers do or unless we find content that is actually broken by not sending it. I would rather not make guesses about what JavaScript implementations may or may not be doing here, because without actual broken sites, it seems just as likely that they would assume that that scroll position had never changed.
Comment 18 Julien Chaffraix 2012-07-25 12:26:40 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > 

> I see. Hmm. But is this really necessary? It's kind of the JS's fault if it chose to change any of its internal scroll information. Because wasn't the whole point of the original change that caused this regression to make sure that that scroll information isn't lost so that it can be restored as it was? That implies to me that sites would have been broken BEFORE the original change, not NOW after these changes. 
> 
> I don't think it makes sense to send a scroll event here unless other browsers do or unless we find content that is actually broken by not sending it. I would rather not make guesses about what JavaScript implementations may or may not be doing here, because without actual broken sites, it seems just as likely that they would assume that that scroll position had never changed.

It can be interpreted both ways. In order for us to restore the scroll offset you need to make the element non-scrollable at some point - which means that scrollOffset is (0, 0) at that time. AFAICT we don't send the 'scroll' event when killing the layer and since we restore it automatically when we are again scrollable, it may be fine to not to dispatch it in this case.

Deferring until we get more reports from the wild seems better indeed, sorry for the noise.
Comment 19 Kent Tamura 2012-07-25 18:15:37 PDT
*** Bug 92239 has been marked as a duplicate of this bug. ***