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 Loading | Assignee: | 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
David Kilzer (:ddkilzer)
2007-03-14 03:44:33 PDT
This is so bad. (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. (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? (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. 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. (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. Created attachment 22108 [details]
Patch v1
Proposed patch.
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
Assigning to me. I want to write a manual test case before I land the patch. (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 Caused regression in Bug 20038. |