297122 - [FIXr]Back and forward through fragment links reloads the page

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[u81239]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050608 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050608 Firefox/1.0+

When assigning a new URL to document.location.href where the URL is the same as
the previous one, except that the fragment identifier is changed (the #hash
part), the URL and location bar change, and the new URL is registered in the
history, but the page doesn’t reload. So far so good.

However, in current builds, when going back a page (one with a different
fragment identifier but the same URL), the page is reloaded. This wasn’t
previously the case, and this is thus a regression.

We (backbase.com) are using this method to support the back and forward buttons
in our Ajax application; therefore, support for this is quite important to us.

The problem occurs both with Fastback enabled and disabled. With Fastback
enabled however, the problem only occurs when when visiting a page in the
history for the first time - if you go back twice, then forward, the page you go
forward to doesn’t show the problem. This is only the case for 5 pages orso (the
value I have set in browser.sessionhistory.max_viewers).

All in all the current behaviour is very inconsistent. When navigating to an URL
with a different hash, it is not reloaded. When going back it is. But using
fastback, in some cases it is not.

I strongly believe it should go back to the way it worked before.

I think that this is a Fastback regression. I also think this should have some
priority as it also affects users who have not enabled fastback.


~Grauw

Reproducible: Always

Comment 1

[u81239]

Blocking fastback bug.

Comment 2

[u81239]

Attachment: Testcase

Attaching testcase

Comment 3

[Brian Ryner (not reading)]

Not a fastback regression; I see the bug in the 2005050407 build, fastback did
not land until later in that day.  This needs further regression analysis (which
I don't have time to do), but I'm removing it as a fastback blocker.

Comment 4

[u81239]

I did some ‘regression analysis’, and it turns out that this regression was
introduced between the 2005011917 and 2005012011 trunk Firefox builds.

Looking at bonsai for that time span, I am cc-ing bzbarsky, who might know more
about this.


~Grauw

Comment 5

[u81239]

I suspect bug 277224 of being the cause of this regression.

Also cc-ing reviewers of that bug, per neil’s suggestion.


~Grauw

Comment 6

[u81239]

I backed out bug 277224 and it works, so that is most certainly the cause.

Comment 7

[u81239]

Attachment: Patch

There are actually three ways to do this:

1. location.hash = "#bla"
2. location.href = "#bla"
3. location.href = location.href + "#bla"

This patch re-applies attachment 63748 [details] [diff] [review] from bug 114975, which was taken out by
the patch in bug 277224. That fixes 1.

It also introduces a check in SetHref which sees if the URL starts with #, and
if so, lets SetHash (the routine for 1.) handle it.

I have tested this in a local build, and it fixes the problem for those first
two cases. For the third case, it would have to compare the string before the #
with the current document’s string. As it happens, the attached testcase tests
the third method.

Please advise on the third case and perhaps a better more generic solution wrt.
URIs with hashes (in SetURI perhaps?), or apply this patch before 1.1 (which is
at least better than nothing at all).


~Grauw

Comment 8

[Boris Zbarsky [:bzbarsky]]

That code should no longer be needed; that was the whole point of the patch in
bug 277224.  Are we actually still calling Stop() somewhere?  If so, what's the
stack to it?

I'll try to look at this, but I have 1000+ messages to read first...

Comment 9

[u81239]

It apparantly is, as the removal introduced the described problem, and the
re-introduction solves it :).

Thanks for taking a look at it.


~Grauw

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: The right fix

Comment 11

[Boris Zbarsky [:bzbarsky]]

Requesting blocking for this regression; the fix is very simple...

Comment 12

[Bob Clary [:bc] (inactive)]

same problem as Bug 298377?

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 187943 [details] [diff] [review]
The right fix

I don't really understand this comment:
Note that the
+		 // loadType and allowScroll check above ensures that this only
+		 // happens when the load command is LOAD_CMD_NORMAL.

Isn't this what the code _right below_ is checking?
+		 if (aLoadType & LOAD_CMD_NORMAL) {

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 187943 [details] [diff] [review]
The right fix

sr=jst

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 187943 [details] [diff] [review]
The right fix

What I'm trying to say with that comment is that the LOAD_CMD_NORMAL check is
correct; the CMD_NORMAL loads that we wouldn't want to do this for never even
make it here.  I'll work on improving the comment to make that clearer.

Requesting 1.8b3 approval.  This is a pretty major regression with anchor
traversals, and a very safe patch.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.8b3

Comment 17

[u81239]

Verified fixed.

Testcase working, back-button on backbase.com now also working.


~Grauw