Bug 89114 – REGRESSION (r112919): Setting scrollTop after setting display from none to block fails

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

<rdar://problem/11656050>

[Julien Chaffraix]

Simon, it looks like the test case didn't make it to the bug.

[Simon Fraser (smfr)]

Created attachment 147637 [details]
Testcase

[Julien Chaffraix]

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.

[Tim Horton]

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

[Rakesh]

I could not reproduce this issue on r120414.

[Simon Fraser (smfr)]

I can still reproduce in a TOT Mac build (the testcase should say "after: 0" at the end.

[Julien Chaffraix]

(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).

[Beth Dakin]

I think I understand what's going on here, and I have an idea for a fix. Taking the bug.

[Beth Dakin]

Created attachment 154247 [details]
Patch

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?

[Julien Chaffraix]

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.

[Beth Dakin]

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

[Beth Dakin]

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

[Beth Dakin]

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.

[Julien Chaffraix]

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.

[Beth Dakin]

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

[Julien Chaffraix]

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

[Kent Tamura]

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