Last Comment Bug 297122 - [FIXr]Back and forward through fragment links reloads the page
: [FIXr]Back and forward through fragment links reloads the page
Status: VERIFIED FIXED
: testcase
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.8beta3
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks: 277224
  Show dependency treegraph
 
Reported: 2005-06-08 13:56 PDT by Laurens Holst
Modified: 2008-07-31 02:47 PDT (History)
9 users (show)
cbeard: blocking1.8b3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (1.04 KB, text/html)
2005-06-08 13:57 PDT, Laurens Holst
no flags Details
Patch (1.40 KB, patch)
2005-06-20 09:24 PDT, Laurens Holst
bzbarsky: review-
Details | Diff | Review
The right fix (1.66 KB, patch)
2005-07-01 08:33 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
cbiesinger: review+
jst: superreview+
benjamin: approval1.8b3+
Details | Diff | Review

Description Laurens Holst 2005-06-08 13:56:34 PDT
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 Laurens Holst 2005-06-08 13:57:16 PDT
Blocking fastback bug.
Comment 2 Laurens Holst 2005-06-08 13:57:50 PDT
Created attachment 185712 [details]
Testcase

Attaching testcase
Comment 3 Brian Ryner (not reading) 2005-06-19 21:17:24 PDT
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 Laurens Holst 2005-06-20 04:29:53 PDT
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 Laurens Holst 2005-06-20 05:29:06 PDT
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 Laurens Holst 2005-06-20 06:50:18 PDT
I backed out bug 277224 and it works, so that is most certainly the cause.
Comment 7 Laurens Holst 2005-06-20 09:24:53 PDT
Created attachment 186811 [details] [diff] [review]
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 [:bz] (Out June 25-July 6) 2005-07-01 06:59:33 PDT
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 Laurens Holst 2005-07-01 08:07:16 PDT
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 [:bz] (Out June 25-July 6) 2005-07-01 08:33:07 PDT
Created attachment 187943 [details] [diff] [review]
The right fix
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-07-01 08:38:13 PDT
Requesting blocking for this regression; the fix is very simple...
Comment 12 Bob Clary [:bc:] 2005-07-01 08:44:03 PDT
same problem as Bug 298377?
Comment 13 Christian :Biesinger (don't email me, ping me on IRC) 2005-07-01 13:43:31 PDT
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, jst@mozilla.com) 2005-07-01 17:11:09 PDT
Comment on attachment 187943 [details] [diff] [review]
The right fix

sr=jst
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-07-01 22:49:32 PDT
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 [:bz] (Out June 25-July 6) 2005-07-03 10:20:41 PDT
Fixed for 1.8b3
Comment 17 Laurens Holst 2005-07-06 12:49:07 PDT
Verified fixed.

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


~Grauw

Note You need to log in before you can comment on or make changes to this bug.