653741 - Scrolling back to the anchor in the url no longer works

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: Current m-c

STEPS TO REPRODUCE:
1)  Load http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.2.4
2)  Scroll down a bit
3)  Focus the url bar
4)  Hit enter

ACTUAL RESULTS: Nothing happens

EXPECTED RESULTS: Scroll back to the top of section 13.2.4.

This was broken in bug 640387 because the code was changed like so:

+            // We scroll the window precisely when we fire a hashchange event.
+            if (doHashchange) {
+                // Take the '#' off the hashes before passing them to
+                // ScrollToAnchor.
+                nsDependentCSubstring curHashName(curHash, 1);
+                nsDependentCSubstring newHashName(newHash, 1);
+                rv = ScrollToAnchor(curHashName, newHashName, aLoadType);

This has been driving me nuts for a few days now, but I finally had the time to track down what change broke the old behavior.

Is there any good reason to not scroll in this case?  If not, can we please restore the old user-visible behavior?  I don't think we should be constraining that on the hashchange semantics the spec happens to specify.

Comment 1

[Justin Lebar (not reading bugmail)]

Should pressing <enter> inside the location bar even trigger a short-circuited load?  Unless you're just changing the anchor, it seems like the right thing to do might be to do a refresh.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Worth testing behavior in other browsers, but the behavior we used to have was pretty explicitly to do the refresh only if the uri has no anchor.

Comment 3

[Justin Lebar (not reading bugmail)]

Chrome and Safari both refresh the page when you focus the location bar and press <enter>, even if there's a hash in the URL.

I'll try IE now...

Comment 4

[Justin Lebar (not reading bugmail)]

IE9 on Windows 7 does what we currently do: Refresh if there's no anchor, don't refresh if there is an anchor.

Personally, I think always refreshing is more sensible.  It's certainly more consistent and thus less surprising.  Of course, changing to that behavior would break your use case of "take me back to the anchor displayed in the URL bar."

Comment 5

[Boris Zbarsky [:bzbarsky]]

Indeed.  I think that we should just go back to the old behavior, personally.  ;)

Comment 6

[Justin Lebar (not reading bugmail)]

Well, I'll file a followup and we can bikeshed with some UI folks there.

Comment 7

[VeRtex]

Confirmed.
Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110502 Firefox/6.0a1

Comment 9

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Pushed to try.

Comment 10

[Justin Lebar (not reading bugmail)]

Comment on attachment 530422 [details] [diff] [review]
Patch v1

This looks green on try.  bz, what do you think?

Comment 11

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1.1

Removing dump()s from test.

Comment 12

[Justin Lebar (not reading bugmail)]

Comment on attachment 531102 [details] [diff] [review]
Patch v1.1

Hm; I apparently forgot to set the review flag on this.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 531102 [details] [diff] [review]
Patch v1.1

Why do we want to skip the scroll if aSHEntry?

Comment 14

[Justin Lebar (not reading bugmail)]

If aSHEntry presumably we have a saved scroll position which we want to use instead.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hmm.  We used to scroll in this situation, though.  Do we restore the scroll position after this point or something?

Comment 16

[Justin Lebar (not reading bugmail)]

hm...we do.  That's awfully strange, though.

This apparent double-scroll existed before pushstate, too (afff5c13df296).

I wonder if it's actually what we want, in some edge case, or just totally bogus.

Comment 17

[Justin Lebar (not reading bugmail)]

The original code would only scroll to anchor if:

 * !aSHEntry and !aPostData, or
 * aSHEntry and mOSHE.pageIdentifier = aSHEntry.pageIdentifier

The page identifiers (which we've since gotten rid of) told you whether two shentries were related by hash navigations.

This patch changes it so we scroll only if

 * !aSHEntry and !aPostData

It seems to me that this is fine iff we always save the scroll position.  We save it when we do a short-circuited load and pushState, and we don't save it on a normal navigation.

Maybe we should be saving the scroll position when you do a normal load.  Otherwise, if you visit

  A.html
  A.html#foo
  B.html

then go back 2, forward 1, we won't have a saved scroll position.  Scrolling to #foo is maybe more correct than doing nothing, but neither seems right.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Yeah, always saving scroll position would make sense...  I'm surprised we don't now: if I go from A.html to B.html and then go back, isn't scroll position on A.html restored?

Comment 19

[Justin Lebar (not reading bugmail)]

(In reply to comment #18)
> Yeah, always saving scroll position would make sense...  I'm surprised we
> don't now: if I go from A.html to B.html and then go back, isn't scroll
> position on A.html restored?

Hm, it is, even when I turn bfcache off.  Maybe it's in the layout history state?

It's still buggy when I go back multiple pages, though.  In trunk with or without bfcache, if I visit:

 * A.html
 * A.html#1
 * A.html#2
 * B.html

then go back 2, I'm taken to the top of the page.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Maybe it's in the layout history state?

Ah.  Yes, it is.

And in this case we'd be saving it in the layout history state when leaving A.html#2 not when leaving A.html, which would break.  Indeed.

Comment 21

[Justin Lebar (not reading bugmail)]

If we really want to persist scroll position for each shentry, can we just not store scroll position in the layout history state?  Bug 646641 already changes things so the layout history state is shared between all the shentries for a given document.

Comment 22

[Boris Zbarsky [:bzbarsky]]

We need to store scroll position in the layout history state to restore scroll position on root reframes.

Comment 23

[Justin Lebar (not reading bugmail)]

(In reply to comment #22)
> We need to store scroll position in the layout history state to restore
> scroll position on root reframes.

Can you elaborate?  I don't know what that is, or where to look to find out.

Comment 24

[Boris Zbarsky [:bzbarsky]]

1)  Load a page.
2)  Scroll down.
3)  javascript:var e = document.documentElement; e.style.display = "none"; e.offsetWidth; e.style.display = ""; void(0);

That's supposed to preserve the scroll position.  Looks slightly broken at the moment; I'm pretty sure we have a bug on that....

Comment 25

[Justin Lebar (not reading bugmail)]

Hm, that looks complicated.  :)

It seems to me that the right way forward is to file a new bug on the issue from comment 19 / comment 20.  In that bug, we'll make sure we always save and restore the scroll position into/out of the SHEntry.

Then the code in this bug should be correct.

Does that sound reasonable?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 531102 [details] [diff] [review]
Patch v1.1

Yeah, I think so.  But please update the comments here to explain why we're checking !aSHEntry, possibly even pointing to this other bug, ok?

Comment 28

[Justin Lebar (not reading bugmail)]

Just to check, I was going to wait to land this until we have the patch for bug 658448.  Do you also think I should wait, or were you expecting me to check this patch in before the imminent Aurora merge?

Comment 29

[Boris Zbarsky [:bzbarsky]]

I think we should get this in before merge; it's a pretty noticeable user-facing regression...

Comment 30

[Justin Lebar (not reading bugmail)]

Is this patch going to cause any regressions of its own?  I don't think it makes us more broken, based on my reasoning in comment 17, but I could be missing something.

Comment 31

[Boris Zbarsky [:bzbarsky]]

I think this is ok to take.

Comment 32

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/575362d9b92e

Comment 33

[JP Rosevear [:jpr]]

Go ahead and request approval for aurora landing.

Comment 34

[Justin Lebar (not reading bugmail)]

This got in to m-c right before the branch, so no need to land on aurora.

Comment 35

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:6.0a2) Gecko/20110525 Firefox/6.0a2

Bug was verified using the steps to reproduce presented in Comment 0 on Ubuntu 11.04 x86, Mac OS X 10.6, Win 7 x86 and WinXP.

Issue is no longer reproducible - setting resolution to Verified Fixed.