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)

enhancement

Tracking

()

People

(Reporter: bzbarsky, Unassigned, NeedInfo)

Details

Attachments

(1 file)

Attached file Testcase
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)
(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)
Priority: -- → P3

Emilio, you've been looking into scroll state restoration bits, right?

Flags: needinfo?(emilio)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: