The default bug view has changed. See this FAQ.

Scrolling back to the anchor in the url no longer works

VERIFIED FIXED in Firefox 6

Status

()

Core
Document Navigation
VERIFIED FIXED
6 years ago
6 years ago

People

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

Tracking

(Depends on: 1 bug, {regression})

Trunk
mozilla6
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox6+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
(Assignee)

Updated

6 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Updated

6 years ago
Keywords: regression
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 3

6 years ago
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...
(Assignee)

Comment 4

6 years ago
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.  ;)
(Assignee)

Comment 6

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

Comment 7

6 years ago
Confirmed.
Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110502 Firefox/6.0a1
Blocks: 654387
(Assignee)

Updated

6 years ago
Duplicate of this bug: 654387
(Assignee)

Updated

6 years ago
No longer blocks: 654387
(Assignee)

Comment 9

6 years ago
Created attachment 530422 [details] [diff] [review]
Patch v1

Pushed to try.
(Assignee)

Comment 10

6 years ago
Comment on attachment 530422 [details] [diff] [review]
Patch v1

This looks green on try.  bz, what do you think?
Attachment #530422 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

6 years ago
Created attachment 531102 [details] [diff] [review]
Patch v1.1

Removing dump()s from test.
(Assignee)

Updated

6 years ago
Attachment #530422 - Attachment is obsolete: true
Attachment #530422 - Flags: review?(bzbarsky)
(Assignee)

Comment 12

6 years ago
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?
(Assignee)

Comment 14

6 years ago
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?
(Assignee)

Comment 16

6 years ago
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.
(Assignee)

Comment 17

6 years ago
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.
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?
(Assignee)

Comment 19

6 years ago
(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.
(Assignee)

Comment 21

6 years ago
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.
(Assignee)

Comment 23

6 years ago
(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; 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....
(Assignee)

Comment 25

6 years ago
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+
(Assignee)

Updated

6 years ago
Depends on: 658448
Duplicate of this bug: 657628
(Assignee)

Comment 28

6 years ago
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...
tracking-firefox6: --- → ?
(Assignee)

Comment 30

6 years ago
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.
tracking-firefox6: ? → ---
I think this is ok to take.
tracking-firefox6: --- → ?
(Assignee)

Comment 32

6 years ago
http://hg.mozilla.org/mozilla-central/rev/575362d9b92e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 33

6 years ago
Go ahead and request approval for aurora landing.
tracking-firefox6: ? → +
(Assignee)

Comment 34

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

Updated

6 years ago
Target Milestone: --- → mozilla6

Updated

6 years ago
status-firefox6: --- → fixed
OS: Mac OS X → All
Hardware: x86 → All

Comment 35

6 years ago
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.
Status: RESOLVED → VERIFIED

Updated

6 years ago
Depends on: 680257
You need to log in before you can comment on or make changes to this bug.