Closed Bug 653741 Opened 13 years ago Closed 13 years ago

Scrolling back to the anchor in the url no longer works


(Core :: DOM: Navigation, defect)

Not set



Tracking Status
firefox6 + fixed


(Reporter: bzbarsky, Assigned: justin.lebar+bug)


(Depends on 1 open bug)


(Keywords: regression)


(1 file, 1 obsolete file)

BUILD: Current m-c

1)  Load
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.
Assignee: nobody → justin.lebar+bug
Keywords: regression
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.
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.
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...
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."
Indeed.  I think that we should just go back to the old behavior, personally.  ;)
Well, I'll file a followup and we can bikeshed with some UI folks there.
Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110502 Firefox/6.0a1
Blocks: 654387
No longer blocks: 654387
Attached patch Patch v1 (obsolete) — Splinter Review
Pushed to try.
Comment on attachment 530422 [details] [diff] [review]
Patch v1

This looks green on try.  bz, what do you think?
Attachment #530422 - Flags: review?(bzbarsky)
Attached patch Patch v1.1Splinter Review
Removing dump()s from test.
Attachment #530422 - Attachment is obsolete: true
Attachment #530422 - Flags: review?(bzbarsky)
Comment on attachment 531102 [details] [diff] [review]
Patch v1.1

Hm; I apparently forgot to set the review flag on this.
Attachment #531102 - Flags: review?(bzbarsky)
Comment on attachment 531102 [details] [diff] [review]
Patch v1.1

Why do we want to skip the scroll if aSHEntry?
If aSHEntry presumably we have a saved scroll position which we want to use instead.
Hmm.  We used to scroll in this situation, though.  Do we restore the scroll position after this point or something?
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.
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


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.
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?
(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.
> 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.
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.
We need to store scroll position in the layout history state to restore scroll position on root reframes.
(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.
1)  Load a page.
2)  Scroll down.
3)  javascript:var e = document.documentElement; = "none"; e.offsetWidth; = ""; 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....
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 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?
Attachment #531102 - Flags: review?(bzbarsky) → review+
Depends on: 658448
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?
I think we should get this in before merge; it's a pretty noticeable user-facing regression...
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.
I think this is ok to take.
Closed: 13 years ago
Resolution: --- → FIXED
Go ahead and request approval for aurora landing.
This got in to m-c right before the branch, so no need to land on aurora.
Target Milestone: --- → mozilla6
OS: Mac OS X → All
Hardware: x86 → All
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.
Depends on: 680257
You need to log in before you can comment on or make changes to this bug.