Bug 13067 – Manually adding #hash to URL reloads entire page instead of jumping to #hash location in cached page

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[David Kilzer (:ddkilzer)]

Summary:

Manually adding "#hash" to the URL in the address bar then hitting Enter will reload the entire page instead of jumping to the "#hash" location in the existing document.  If one clicks on a "#hash" link in the page, though, the page does NOT reload and the user is taken to the "#hash" location in the existing document.

Steps to reproduce:

1. Open Safari/WebKit.
2. Open URL with #hash locations like:  http://trac.webkit.org/projects/webkit/changeset/20182
3. Add "#file3" to the end of the URL in the address bar, then hit Enter.

Expected results:

Safari/WebKit should jump to "#file3" location without reloading the page.

Actual results:

Safari/WebKit reloads the entire page, then jumps to "#file3" location.

Regression:

Surprisingly, this is not a regression from shipping Safari 2.0.4 (419.3) on Mac OS X 10.4.8 (8L127).  It has the exact same behavior.

Notes:

Tested with a local debug build of WebKit r20184 with the above software.

Note that Firefox 2.0.0.2 and Opera 9.10 both exhibit the expected behavior.  OmniWeb 5.5.3 exhibits the actual behavior (as a WebKit browser).

[Dave Hyatt]

This is so bad.

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> This is so bad.

FWIW, Mac IE 5.2.3 behaves the same as Safari.

It also appears that opening the URL (http://trac.webkit.org/projects/webkit/changeset/20182) in one browser window, then opening the same URL with a hash (http://trac.webkit.org/projects/webkit/changeset/20182#file13) in a second window (or tab) also doesn't used the cached copy of the page.

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> This is so bad.

Does this behavior coincide with Safari's "always reload the page when hitting Enter in the URL Address Field" behavior, though?

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> Does this behavior coincide with Safari's "always reload the page when hitting
> Enter in the URL Address Field" behavior, though?

I guess we could do the following:

1. If the new URL only differs from the previous URL by adding a #hash, don't reload the page, just jump to #hash.
2. Else if the new URL only differs from the previous URL by a different #hash, don't reload the page, just jump to #newhash.
3. Else reload the page.

[Khoo Yit Phang]

I'm wondering if this is related to the bug I just filed? (http://bugs.webkit.org/show_bug.cgi?id=17832)

In this case, setting window.location.hash to an empty string when it was originally empty to begin with will cause the page to reload.

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> I'm wondering if this is related to the bug I just filed?
> (http://bugs.webkit.org/show_bug.cgi?id=17832)
> 
> In this case, setting window.location.hash to an empty string when it was
> originally empty to begin with will cause the page to reload.

It's definitely related.

[Cameron Zwarich (cpst)]

This seems related to bug 19822.

[David Kilzer (:ddkilzer)]

Created attachment 22108 [details]
Patch v1

Proposed patch.

[Darin Adler]

Comment on attachment 22108 [details]
Patch v1

         checkNavigationPolicy(request, oldDocumentLoader.get(), formState,
-            callContinueFragmentScrollAfterNavigationPolicy, this);
+                              callContinueFragmentScrollAfterNavigationPolicy, this);

I don't personally consider this indentation change an improvement. It's a change from a form that can be done with a simple editor to a form that requires careful indenting or a syntax-sensitive editor (like emacs).

+    // These rules are based on what KHTML was doing in KHTMLPart::openURL.

I think this comment should either go away or change its wording to "were originally based on".

+        bool shouldScrollToAnchor(bool isFormSubmission, FrameLoadType newLoadType, const KURL& newURL);

I'm not sure the "new" prefix alone is enough to justify giving the argument names here.

r=me

[David Kilzer (:ddkilzer)]

Assigning to me.  I want to write a manual test case before I land the patch.

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> (From update of attachment 22108 [details] [edit])
>          checkNavigationPolicy(request, oldDocumentLoader.get(), formState,
> -            callContinueFragmentScrollAfterNavigationPolicy, this);
> +                              callContinueFragmentScrollAfterNavigationPolicy,
> this);
> 
> I don't personally consider this indentation change an improvement. It's a
> change from a form that can be done with a simple editor to a form that
> requires careful indenting or a syntax-sensitive editor (like emacs).

Fixed to only indent 4 spaces from the previous line.

> +    // These rules are based on what KHTML was doing in KHTMLPart::openURL.
> 
> I think this comment should either go away or change its wording to "were
> originally based on".

Fixed by changing the wording.

> +        bool shouldScrollToAnchor(bool isFormSubmission, FrameLoadType
> newLoadType, const KURL& newURL);
> 
> I'm not sure the "new" prefix alone is enough to justify giving the argument
> names here.

Removed "new" prefixes.  (I originally kept them because using a parameter name of "url" didn't allow the "url()" method to be called, so I had to qualify it with "this->url()".

Committed revision 35151.
http://trac.webkit.org/changeset/35151

[David Kilzer (:ddkilzer)]

Caused regression in Bug 20038.