1425107 - Setting scrollTop on element does not work unless scrollTop is referenced first

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Josh Billingham]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

I can reproduce this on Firefox 57.0 (32 bit) on Windows 7 as well as Firefox 57.0.1 (64 bit) on Linux.

Here is a JS fiddle illustrating the problem:
https://jsfiddle.net/3nbwmcpc/13/

And a video of the behavior in question:
https://drive.google.com/file/d/1VcvL_IWE8cOQtsOC-sn4ha1PBVUJxDA7/view?usp=sharing

1. Create an container element that has contents overflowing in the y direction (overflow:auto).
2. Scroll the element to the bottom.
3. Hide the element by setting 'display: none'.
4. Show the element by setting 'display: block'.
5. Set the scrollTop of the element to 0.




Actual results:

The container does not scroll to the top. Instead, it remains at the same scroll position that it was scrolled to before it was hidden.

If scrollTop is referenced before it's set (i.e. via a console.log), the behavior works as expected (container is scrolled to top).


Expected results:

The container should scroll to the top when scrollTop is set to 0. This is the behavior for both Chrome and IE.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c2ff59dd31bce41bc9108939e86618017943b88d&tochange=0874cf4bb194d381a3afaa51276b6cee22f82211

Suspect: Bug 1364360

@:bz
Your bunch of patch seems to canse the regression, Can you look into this?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Standalone testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix setting scrollTop to 0 on an element with a pending scroll position restore to actually work correctly

MozReview-Commit-ID: 949eBXmKHlA

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 8936843 [details] [diff] [review]
Fix setting scrollTop to 0 on an element with a pending scroll position restore to actually work correctly

You should probably drop the "to 0" in the commit message because
the actual value doesn't really matter, right?

Comment 5

[Boris Zbarsky [:bzbarsky]]

It matters, because when setting to nonzero we'll flush layout, which will flush out the pending scroll position restore before we do the scroll for the scrollTop set...

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81174f568eaa
Fix setting scrollTop to 0 on an element with a pending scroll position restore to actually work correctly.  r=mats

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/81174f568eaa

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Bz, do you think we should this fix ride the trains or uplift it in 58?

Comment 9

[Boris Zbarsky [:bzbarsky]]

This isn't 100% risk-free, but it's probably OK to uplift to 58 if we do it soon....

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8936843 [details] [diff] [review]
Fix setting scrollTop to 0 on an element with a pending scroll position restore to actually work correctly

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1364360
[User impact if declined]: On some sites, things may not scroll as they should.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Somewhat.
[Why is the change risky/not risky?]:  We're changing how scroll state restoration interacts with explicit scrolls: with this change an explicit scroll cancels scroll state restoration.  The failure mode here would be if we generate spurious explicit scrolls somewhere and that causes scroll positions to be lost on reframe or some such.
[String changes made/needed]: None.

Comment 11

[Gerry Chang [:gchang]]

Comment on attachment 8936843 [details] [diff] [review]
Fix setting scrollTop to 0 on an element with a pending scroll position restore to actually work correctly

Fix a scrolling issue. Beta58+.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9e7435aeb7bd