Open
Bug 1442958
Opened 6 years ago
Updated 11 months ago
Attempt to restore scroll position after popstate doesn't really do the right thing due to lack of layout flush
Categories
(Core :: DOM: Navigation, enhancement, P3)
Core
DOM: Navigation
Tracking
()
NEW
People
(Reporter: bzbarsky, Unassigned, NeedInfo)
Details
Attachments
(1 file)
678 bytes,
text/html
|
Details |
See attached testcase. When executed, it does not scroll to the right place on the back() navigation. If I introduce a layout flush into the testcase before returning from the popstate handler, or manually put one in nsDocShell::SetCurScrollPosEx, the problem goes away. The issue, of course, is that when we try to restore the old scroll position by calling nsDocShell::SetCurScrollPosEx in the popstate case in nsDocShell::InternalLoad the layout hasn't yet been updated for the newly-generated DOM, so the scroll attempt gets clamped to the actual height of the content at that point (which is 0). There are a few options here, I think. We could add a layout flush after firing popstate and before attempting SetCurScrollPosEx. We could add a flush to SetCurScrollPosEx itself (but the only other caller scrolls to (0,0), which never needs a flush). We could change this code to look more like normal pageload and set a "position to restore" on the root scrollframe, then have it restore it during the next reflow. Not sure whether we'd need to rejigger the logic in there that's checking for the page load state, because the page load state in this case is "loaded" all the time... Maybe that's OK because in practice we'll only end up needing to try to restore once. Not having a sync layout flush here would be nice, so I'm somewhat interested in the "make it more like a normal scroll position restore" approach. Timothy, it looks like you've been involved in most of the recent changes to ScrollFrameHelper::ScrollToRestoredPosition. How does that idea sound?
Flags: needinfo?(tnikkel)
Comment 1•6 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #0) > We could change this code to look more > like normal pageload and set a "position to restore" on the root > scrollframe, then have it restore it during the next reflow. Not sure > whether we'd need to rejigger the logic in there that's checking for the > page load state, because the page load state in this case is "loaded" all > the time... Maybe that's OK because in practice we'll only end up needing > to try to restore once. I think that would be fine because there isn't any way we could define a period of time where more content is coming and so to keep trying to restore, correct? > Not having a sync layout flush here would be nice, so I'm somewhat > interested in the "make it more like a normal scroll position restore" > approach. Timothy, it looks like you've been involved in most of the recent > changes to ScrollFrameHelper::ScrollToRestoredPosition. How does that idea > sound? Sounds reasonable. All those changes have tests so should be pretty easy to make sure we don't regress something.
Flags: needinfo?(tnikkel)
Updated•6 years ago
|
Priority: -- → P3
Reporter | ||
Comment 2•4 years ago
|
||
Emilio, you've been looking into scroll state restoration bits, right?
Flags: needinfo?(emilio)
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•