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

VERIFIED FIXED in mozilla1.8beta3

Status

()

Core
Document Navigation
P1
normal
VERIFIED FIXED
12 years ago
9 years ago

People

(Reporter: Laurens Holst, Assigned: bz)

Tracking

({testcase})

Trunk
mozilla1.8beta3
testcase
Points:
---
Bug Flags:
blocking1.8b3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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
(Reporter)

Comment 1

12 years ago
Blocking fastback bug.
Blocks: 274784
(Reporter)

Comment 2

12 years ago
Created attachment 185712 [details]
Testcase

Attaching testcase

Updated

12 years ago
Keywords: testcase
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.
No longer blocks: 274784
(Reporter)

Comment 4

12 years ago
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
(Reporter)

Comment 5

12 years ago
I suspect bug 277224 of being the cause of this regression.

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


~Grauw
(Reporter)

Comment 6

12 years ago
I backed out bug 277224 and it works, so that is most certainly the cause.
Blocks: 277224
(Reporter)

Comment 7

12 years ago
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
(Reporter)

Updated

12 years ago
Attachment #186811 - Flags: review?(darin)
(Reporter)

Updated

12 years ago
Attachment #186811 - Flags: review?(darin) → review?(jst)
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...
(Reporter)

Comment 9

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

Thanks for taking a look at it.


~Grauw
Created attachment 187943 [details] [diff] [review]
The right fix
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #187943 - Flags: superreview?(jst)
Attachment #187943 - Flags: review?(cbiesinger)
Attachment #186811 - Flags: review?(jst) → review-
Requesting blocking for this regression; the fix is very simple...
Flags: blocking1.8b3?
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Summary: Back and forward through fragment links reloads the page → [FIX]Back and forward through fragment links reloads the page
Target Milestone: --- → mozilla1.8beta3

Comment 12

12 years ago
same problem as Bug 298377?
(Reporter)

Updated

12 years ago
Attachment #186811 - Attachment is obsolete: true
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) {
Attachment #187943 - Flags: review?(cbiesinger) → review+

Updated

12 years ago
Flags: blocking1.8b3? → blocking1.8b3+
Comment on attachment 187943 [details] [diff] [review]
The right fix

sr=jst
Attachment #187943 - Flags: superreview?(jst) → superreview+
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.
Attachment #187943 - Flags: approval1.8b3?
Summary: [FIX]Back and forward through fragment links reloads the page → [FIXr]Back and forward through fragment links reloads the page
Attachment #187943 - Flags: approval1.8b3? → approval1.8b3+
Fixed for 1.8b3
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 17

12 years ago
Verified fixed.

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


~Grauw
Status: RESOLVED → VERIFIED

Updated

9 years ago
Component: History: Session → Document Navigation
QA Contact: history.session → docshell
You need to log in before you can comment on or make changes to this bug.