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

RESOLVED FIXED in Firefox 58

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: wjbillin, Assigned: bzbarsky)

Tracking

({regression})

55 Branch
mozilla59
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 wontfix, firefox58 fixed, firefox59 fixed)

Details

Attachments

(2 attachments)

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.
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Version: 57 Branch → 55 Branch
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Flags: needinfo?(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

You should probably drop the "to 0" in the commit message because
the actual value doesn't really matter, right?
Attachment #8936843 - Flags: review?(mats) → review+
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...
Pushed by bzbarsky@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/81174f568eaa
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Bz, do you think we should this fix ride the trains or uplift it in 58?
Flags: needinfo?(bzbarsky)
This isn't 100% risk-free, but it's probably OK to uplift to 58 if we do it soon....
Flags: needinfo?(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.
Attachment #8936843 - Flags: approval-mozilla-beta?
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+.
Attachment #8936843 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.