Closed Bug 1425107 Opened 4 years ago Closed 4 years ago
Top on element does not work unless scroll Top is referenced first
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.
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?
Attachment #8936843 - Flags: review?(mats)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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 email@example.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
Bz, do you think we should this fix ride the trains or uplift it in 58?
This isn't 100% risk-free, but it's probably OK to uplift to 58 if we do it soon....
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.