Bug 13067 - Manually adding #hash to URL reloads entire page instead of jumping to #hash location in cached page
: Manually adding #hash to URL reloads entire page instead of jumping to #hash ...
Status: RESOLVED FIXED
: WebKit
Page Loading
: 523.x (Safari 3)
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To:
: http://trac.webkit.org/projects/webki...
:
:
:
  Show dependency treegraph
 
Reported: 2007-03-14 03:44 PST by
Modified: 2009-01-04 08:38 PST (History)


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


Note

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


Description From 2007-03-14 03:44:33 PST
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 From 2007-03-14 04:18:41 PST -------
This is so bad.
------- Comment #2 From 2007-03-14 06:25:39 PST -------
(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 From 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 From 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 From 2008-03-13 16:05:20 PST -------
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 From 2008-03-13 16:08:25 PST -------
(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 From 2008-06-30 11:41:29 PST -------
This seems related to bug 19822.
------- Comment #8 From 2008-07-06 07:28:18 PST -------
Created an attachment (id=22108) [details]
Patch v1

Proposed patch.
------- Comment #9 From 2008-07-06 19:26:59 PST -------
(From update of attachment 22108 [details])
         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 From 2008-07-09 08:23:39 PST -------
Assigning to me.  I want to write a manual test case before I land the patch.
------- Comment #11 From 2008-07-12 14:35:19 PST -------
(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 From 2009-01-04 08:38:43 PST -------
Caused regression in Bug 20038.