Bug 13067

Summary: Manually adding #hash to URL reloads entire page instead of jumping to #hash location in cached page
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: Page LoadingAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: hyatt, mrowe, rachael, rik
Priority: P2    
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
URL: http://trac.webkit.org/projects/webkit/changeset/20182
Attachments:
Description Flags
Patch v1 darin: review+

David Kilzer (:ddkilzer)
Reported 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).
Attachments
Patch v1 (8.40 KB, patch)
2008-07-06 07:28 PDT, David Kilzer (:ddkilzer) 		darin: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Dave Hyatt
Comment 1 2007-03-14 04:18:41 PDT
This is so bad.
David Kilzer (:ddkilzer)
Comment 2 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.
David Kilzer (:ddkilzer)
Comment 3 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?
David Kilzer (:ddkilzer)
Comment 4 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.
Khoo Yit Phang
Comment 5 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.
David Kilzer (:ddkilzer)
Comment 6 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.
Cameron Zwarich (cpst)
Comment 7 2008-06-30 11:41:29 PDT
This seems related to bug 19822.
David Kilzer (:ddkilzer)
Comment 8 2008-07-06 07:28:18 PDT
Created attachment 22108 [details] Patch v1 Proposed patch.
Darin Adler
Comment 9 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
David Kilzer (:ddkilzer)
Comment 10 2008-07-09 08:23:39 PDT
Assigning to me. I want to write a manual test case before I land the patch.
David Kilzer (:ddkilzer)
Comment 11 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
David Kilzer (:ddkilzer)
Comment 12 2009-01-04 08:38:43 PST
Caused regression in Bug 20038.
Note You need to log in before you can comment on or make changes to this bug.