Going forward to a history entry created by pushState doesn't update scroll position

VERIFIED FIXED

Status

()

Core
DOM
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: jbalogh, Assigned: Justin Lebar (not reading bugmail))

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
Putting this in Core::DOM since I'm playing with history.pushState.

I have a page that's using pushState: http://z.jbalogh.me/history.html.

If I scroll down the page and then hit back I get taken to the top of the page.  There doesn't seem to be a way to prevent this, even when catching the popState event.
Scroll position is a function of the history entry, no?  In particular, if you pushState a url with an anchor ref we should scroll to it, I should think.

That said, the fact that going forward again doesn't scroll back down seems like a bug.
(Assignee)

Comment 2

8 years ago
(In reply to comment #1)
> In particular, if you pushState a url with an anchor ref we should scroll to 
> it, I should think.

I don't believe this is implied by the spec as it exists now, although we could of course change it if we wanted to.

http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#dom-history-pushstate

> That said, the fact that going forward again doesn't scroll back down seems
> like a bug.

I agree.
(Reporter)

Comment 3

8 years ago
You can also see this on the tabs under "Browse Add-ons" on https://preview.addons.mozilla.org/z/en-US/firefox/.  We try to use push/popState when you switch tabs, but going back jumps to the top of the page.
Again, scroll position is session-history-entry-specific.  If you pushstate, then scroll, then pop, I would expect us to scroll back to where we were when you pushed...
(Assignee)

Updated

8 years ago
Summary: Going back in history moves the viewport to the top of the page → Going forward to a history entry created by pushState doesn't update scroll position
(Assignee)

Comment 5

8 years ago
This should block, I think.

I have a fix that seems to work; just writing a test now.
blocking2.0: --- → ?
Here's a screencast showing the problem going backwards: http://screencast.com/t/YTU5ZDc1.
(Reporter)

Comment 7

8 years ago
(In reply to comment #6)
> Here's a screencast showing the problem going backwards:
> http://screencast.com/t/YTU5ZDc1.

I asked stephen to make one of his videos to show my original issue: he's scrolled halfway down the page clicking on the tabs (which is when we pushState).  When he hits Back it jumps to the top of the page.
Yes; the point is that this is expected behavior...
(Assignee)

Comment 9

8 years ago
(In reply to comment #8)
> Yes; the point is that this is expected behavior...

It should scroll you back to where you were before the pushState.  I think his point is that it scrolls you back to the top regardless.  At least, that's what I see in the video.
(Reporter)

Comment 10

8 years ago
(In reply to comment #9)
> (In reply to comment #8)
> > Yes; the point is that this is expected behavior...
> 
> It should scroll you back to where you were before the pushState.  I think his
> point is that it scrolls you back to the top regardless.  At least, that's what
> I see in the video.

Yes.  If we're updating scroll position for going forward, it seems like the same should be done for going back.
(Assignee)

Comment 11

8 years ago
There are actually two issues here.

One is in the spec: Since a pushState doesn't cause a history traversal, you never save the scroll position.  Hixie agrees this is a spec bug; we'll get it fixed.

The other is that going back and forward between history entries which are related by a pushState doesn't respect the saved scroll state for either of them.
(Assignee)

Comment 12

8 years ago
Created attachment 469185 [details] [diff] [review]
Patch v1

This patch fixes both issues from my previous comment.
Assignee: nobody → justin.lebar+bug
Status: NEW → ASSIGNED
Attachment #469185 - Flags: review?(bzbarsky)
> It should scroll you back to where you were before the pushState.

Yes, absolutely.
This seems like bad behavior in a new FF4 feature.
blocking2.0: ? → betaN+
Hmm.  So normal navigations didn't use to save the scroll pos in the history entry?  Where did it get saved then?  And can we remove that other saving location now, perhaps as a followup?
Comment on attachment 469185 [details] [diff] [review]
Patch v1

r=bzbarsky as long as you test scroll pos restoration on normal page navigations well.
Attachment #469185 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 18

8 years ago
(In reply to comment #16)
> Hmm.  So normal navigations didn't use to save the scroll pos in the history
> entry?  Where did it get saved then?

Is this owned by the presentation shell?  See nsDocShell::GetCurScrollPos() and nsDocShell::GetRootScrollFrame().
Blocks: 580066
(Assignee)

Comment 19

8 years ago
Created attachment 469313 [details] [diff] [review]
Patch v1.5 (WIP, broken test)

This patch attempts to test scroll pos restoration on a normal load.  It fails, sadly, and I'm not sure why.  I've tried doing what I think are the same steps outside mochitest, and that works as i expect.

I'll look at it in the morning, I guess.
Attachment #469185 - Attachment is obsolete: true
> Is this owned by the presentation shell? 

Could be, sure.
(Assignee)

Comment 21

8 years ago
Created attachment 469522 [details] [diff] [review]
Patch v2 (working test)

All right; I managed to get it to work by spinning the event loop more.  The behavior is pretty strange if you don't spin the event loop enough, but in my experience that's true in general for history stuff, so it doesn't concern me too much.
Attachment #469313 - Attachment is obsolete: true
(Assignee)

Comment 22

8 years ago
http://hg.mozilla.org/mozilla-central/rev/44e28539ef9b
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 23

8 years ago
(In reply to comment #3)
> You can also see this on the tabs under "Browse Add-ons" on
> https://preview.addons.mozilla.org/z/en-US/firefox/.  We try to use
> push/popState when you switch tabs, but going back jumps to the top of the
> page.

Jeff, that page isn't currently using pushState, is it?  I get the same wonky scroll behavior there as I did before this fix, although http://z.jbalogh.me/history.html works fine for me.
(Reporter)

Comment 24

8 years ago
Yes, we pushState whenever you click on a tab.  The code is at [1].  We pushState as soon as you hit the page and then again on each click.

[1]: http://github.com/jbalogh/zamboni/blob/master/media/js/zamboni/homepage.js
(Assignee)

Comment 25

8 years ago
Oh, I see what's going on.  When the page loads, it pushStates to ?browse=featured.  So then when I go back, I jump up to the top of the page.

I'd recommend using replaceState for that initial load.  Does the scrolling look good to you in the latest nightly?
(Reporter)

Comment 26

8 years ago
(In reply to comment #25) 
> I'd recommend using replaceState for that initial load.  Does the scrolling
> look good to you in the latest nightly?

Looks great to me, thanks!
Status: RESOLVED → VERIFIED
(Assignee)

Updated

8 years ago
Blocks: 593144
(Spec has been updated accordingly.)
(Assignee)

Updated

8 years ago
No longer blocks: 580066
You need to log in before you can comment on or make changes to this bug.