Bug 13067 - Manually adding #hash to URL reloads entire page instead of jumping to #hash location in cached page
Summary: Manually adding #hash to URL reloads entire page instead of jumping to #hash ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL: http://trac.webkit.org/projects/webki...
Keywords:
Depends on:
Blocks:
 
Reported: 2007-03-14 03:44 PDT by David Kilzer (:ddkilzer)
Modified: 2009-01-04 08:38 PST (History)
4 users (show)

See Also:


Attachments
Patch v1 (8.40 KB, patch)
2008-07-06 07:28 PDT, David Kilzer (:ddkilzer)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2007-03-14 03:44:33 PDT
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).
Comment 1 Dave Hyatt 2007-03-14 04:18:41 PDT
This is so bad.
Comment 2 David Kilzer (:ddkilzer) 2007-03-14 06:25:39 PDT
(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.

Comment 3 David Kilzer (:ddkilzer) 2008-01-02 11:31:12 PST
(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?

Comment 4 David Kilzer (:ddkilzer) 2008-01-02 11:38:29 PST
(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.

Comment 5 Khoo Yit Phang 2008-03-13 16:05:20 PDT
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.

Comment 6 David Kilzer (:ddkilzer) 2008-03-13 16:08:25 PDT
(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.

Comment 7 Cameron Zwarich (cpst) 2008-06-30 11:41:29 PDT
This seems related to bug 19822.
Comment 8 David Kilzer (:ddkilzer) 2008-07-06 07:28:18 PDT
Created attachment 22108 [details]
Patch v1

Proposed patch.
Comment 9 Darin Adler 2008-07-06 19:26:59 PDT
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
Comment 10 David Kilzer (:ddkilzer) 2008-07-09 08:23:39 PDT
Assigning to me.  I want to write a manual test case before I land the patch.
Comment 11 David Kilzer (:ddkilzer) 2008-07-12 14:35:19 PDT
(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
Comment 12 David Kilzer (:ddkilzer) 2009-01-04 08:38:43 PST
Caused regression in Bug 20038.