Closed
Bug 1425107
Opened 5 years ago
Closed 5 years ago
Setting scrollTop on element does not work unless scrollTop is referenced first
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox57 | --- | wontfix |
firefox58 | --- | fixed |
firefox59 | --- | fixed |
People
(Reporter: wjbillin, Assigned: bzbarsky)
References
Details
(Keywords: regression)
Attachments
(2 files)
369 bytes,
text/html
|
Details | |
5.69 KB,
patch
|
MatsPalmgren_bugz
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
![]() |
||
Updated•5 years ago
|
Status: UNCONFIRMED → NEW
status-firefox57:
--- → unaffected
status-firefox58:
--- → unaffected
status-firefox59:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Component: Untriaged → Layout
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Version: 57 Branch → 55 Branch
![]() |
||
Comment 1•5 years ago
|
||
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?
Blocks: 1364360
Flags: needinfo?(bzbarsky)
![]() |
Assignee | |
Comment 2•5 years ago
|
||
![]() |
Assignee | |
Comment 3•5 years ago
|
||
MozReview-Commit-ID: 949eBXmKHlA
Attachment #8936843 -
Flags: review?(mats)
![]() |
Assignee | |
Updated•5 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
![]() |
Assignee | |
Updated•5 years ago
|
Flags: needinfo?(bzbarsky)
Comment 4•5 years ago
|
||
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+
![]() |
Assignee | |
Comment 5•5 years ago
|
||
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
Comment 7•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/81174f568eaa
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 8•5 years ago
|
||
Bz, do you think we should this fix ride the trains or uplift it in 58?
Flags: needinfo?(bzbarsky)
![]() |
Assignee | |
Comment 9•5 years ago
|
||
This isn't 100% risk-free, but it's probably OK to uplift to 58 if we do it soon....
Flags: needinfo?(bzbarsky)
![]() |
Assignee | |
Comment 10•5 years ago
|
||
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 11•5 years ago
|
||
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+
Comment 12•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/9e7435aeb7bd
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•