- Revision
- 225645
- Author
- achristen...@apple.com
- Date
- 2017-12-07 13:59:48 -0800 (Thu, 07 Dec 2017)
Log Message
Always synchronously continue with fragment navigations
https://bugs.webkit.org/show_bug.cgi?id=180544
<rdar://problem/34815986> and <rdar://problem/35126690>
Reviewed by Geoffrey Garen.
Source/WebCore:
Test: http/tests/dom/document-fragment.html
When a decidePolicyForNavigationAction completionHandler is called asynchronously,
the document's URL has not changed yet when _javascript_ execution continues. This causes
significant web incompatibility because all browsers change the document's URL immediately
during fragment navigations. In order to make WebKit applications more web compatible,
we now immediately continue to have the state consistent. To keep compatibility with any
WebView, UIWebView, or WKWebView applications that use these delegate callbacks to update
state, we still call decidePolicyForNavigationAction. This would break a theoretical app
that would cancel fragment navigations, but it fixes apps that continue fragment navigations
asynchronously.
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::loadURL):
(WebCore::FrameLoader::loadWithDocumentLoader):
Tools:
* TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm:
(-[DecidePolicyForNavigationActionFragmentDelegate webView:decidePolicyForNavigationAction:decisionHandler:]):
(TEST):
Add a test that verifies that decidePolicyForNavigationAction is called for fragment navigations.
LayoutTests:
* http/tests/dom/document-fragment-expected.txt: Added.
* http/tests/dom/document-fragment.html: Added.
Add a test that verifies that the fragment of the document is set immediately during a fragment navigation,
even if decidePolicyForNavigationAction is called asynchronously. Also verify the order of various events
associated with the navigation.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (225644 => 225645)
--- trunk/LayoutTests/ChangeLog 2017-12-07 21:46:11 UTC (rev 225644)
+++ trunk/LayoutTests/ChangeLog 2017-12-07 21:59:48 UTC (rev 225645)
@@ -1,3 +1,17 @@
+2017-12-07 Alex Christensen <achristen...@webkit.org>
+
+ Always synchronously continue with fragment navigations
+ https://bugs.webkit.org/show_bug.cgi?id=180544
+ <rdar://problem/34815986> and <rdar://problem/35126690>
+
+ Reviewed by Geoffrey Garen.
+
+ * http/tests/dom/document-fragment-expected.txt: Added.
+ * http/tests/dom/document-fragment.html: Added.
+ Add a test that verifies that the fragment of the document is set immediately during a fragment navigation,
+ even if decidePolicyForNavigationAction is called asynchronously. Also verify the order of various events
+ associated with the navigation.
+
2017-12-07 Youenn Fablet <you...@apple.com>
Activate IDB and WebSockets in service workers
Added: trunk/LayoutTests/http/tests/dom/document-fragment-expected.txt (0 => 225645)
--- trunk/LayoutTests/http/tests/dom/document-fragment-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/dom/document-fragment-expected.txt 2017-12-07 21:59:48 UTC (rev 225645)
@@ -0,0 +1,2 @@
+ALERT: continuing with correct fragment, promise, popstate, hashchange, timeout
+
Added: trunk/LayoutTests/http/tests/dom/document-fragment.html (0 => 225645)
--- trunk/LayoutTests/http/tests/dom/document-fragment.html (rev 0)
+++ trunk/LayoutTests/http/tests/dom/document-fragment.html 2017-12-07 21:59:48 UTC (rev 225645)
@@ -0,0 +1,27 @@
+<script>
+
+if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+ if (testRunner.setShouldDecideNavigationPolicyAfterDelay)
+ testRunner.setShouldDecideNavigationPolicyAfterDelay(true);
+}
+
+var events = [];
+function eventHappened(eventName) {
+ events.push(eventName);
+ if (events.length == 5) {
+ alert(events[0] + ", " + events[1] + ", " + events[2] + ", " + events[3] + ", " + events[4]);
+ if (window.testRunner)
+ testRunner.notifyDone();
+ }
+}
+
+window._onhashchange_ = ()=>{ eventHappened("hashchange") }
+window._onpopstate_ = ()=>{ eventHappened("popstate") };
+window.location.href = ""
+Promise.resolve().then(()=>{ eventHappened("promise") });
+setTimeout(()=>{ eventHappened("timeout") }, 0)
+eventHappened("continuing with " + (document.URL.endsWith("#fragment") ? "correct fragment" : "incorrect fragment"));
+
+</script>
Modified: trunk/Source/WebCore/ChangeLog (225644 => 225645)
--- trunk/Source/WebCore/ChangeLog 2017-12-07 21:46:11 UTC (rev 225644)
+++ trunk/Source/WebCore/ChangeLog 2017-12-07 21:59:48 UTC (rev 225645)
@@ -1,3 +1,27 @@
+2017-12-07 Alex Christensen <achristen...@webkit.org>
+
+ Always synchronously continue with fragment navigations
+ https://bugs.webkit.org/show_bug.cgi?id=180544
+ <rdar://problem/34815986> and <rdar://problem/35126690>
+
+ Reviewed by Geoffrey Garen.
+
+ Test: http/tests/dom/document-fragment.html
+
+ When a decidePolicyForNavigationAction completionHandler is called asynchronously,
+ the document's URL has not changed yet when _javascript_ execution continues. This causes
+ significant web incompatibility because all browsers change the document's URL immediately
+ during fragment navigations. In order to make WebKit applications more web compatible,
+ we now immediately continue to have the state consistent. To keep compatibility with any
+ WebView, UIWebView, or WKWebView applications that use these delegate callbacks to update
+ state, we still call decidePolicyForNavigationAction. This would break a theoretical app
+ that would cancel fragment navigations, but it fixes apps that continue fragment navigations
+ asynchronously.
+
+ * loader/FrameLoader.cpp:
+ (WebCore::FrameLoader::loadURL):
+ (WebCore::FrameLoader::loadWithDocumentLoader):
+
2017-12-07 Youenn Fablet <you...@apple.com>
Activate IDB and WebSockets in service workers
Modified: trunk/Source/WebCore/loader/FrameLoader.cpp (225644 => 225645)
--- trunk/Source/WebCore/loader/FrameLoader.cpp 2017-12-07 21:46:11 UTC (rev 225644)
+++ trunk/Source/WebCore/loader/FrameLoader.cpp 2017-12-07 21:59:48 UTC (rev 225645)
@@ -1315,9 +1315,8 @@
oldDocumentLoader->setLastCheckedRequest(ResourceRequest());
policyChecker().stopCheck();
policyChecker().setLoadType(newLoadType);
- policyChecker().checkNavigationPolicy(WTFMove(request), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [this] (const ResourceRequest& request, FormState*, bool shouldContinue) {
- continueFragmentScrollAfterNavigationPolicy(request, shouldContinue);
- });
+ continueFragmentScrollAfterNavigationPolicy(request, true);
+ policyChecker().checkNavigationPolicy(WTFMove(request), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [] (const ResourceRequest&, FormState*, bool) { });
return;
}
@@ -1475,9 +1474,8 @@
oldDocumentLoader->setTriggeringAction(action);
oldDocumentLoader->setLastCheckedRequest(ResourceRequest());
policyChecker().stopCheck();
- policyChecker().checkNavigationPolicy(ResourceRequest(loader->request()), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [this] (const ResourceRequest& request, FormState*, bool shouldContinue) {
- continueFragmentScrollAfterNavigationPolicy(request, shouldContinue);
- });
+ continueFragmentScrollAfterNavigationPolicy(loader->request(), true);
+ policyChecker().checkNavigationPolicy(ResourceRequest(loader->request()), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [] (const ResourceRequest&, FormState*, bool) { });
return;
}
Modified: trunk/Tools/ChangeLog (225644 => 225645)
--- trunk/Tools/ChangeLog 2017-12-07 21:46:11 UTC (rev 225644)
+++ trunk/Tools/ChangeLog 2017-12-07 21:59:48 UTC (rev 225645)
@@ -1,3 +1,16 @@
+2017-12-07 Alex Christensen <achristen...@webkit.org>
+
+ Always synchronously continue with fragment navigations
+ https://bugs.webkit.org/show_bug.cgi?id=180544
+ <rdar://problem/34815986> and <rdar://problem/35126690>
+
+ Reviewed by Geoffrey Garen.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm:
+ (-[DecidePolicyForNavigationActionFragmentDelegate webView:decidePolicyForNavigationAction:decisionHandler:]):
+ (TEST):
+ Add a test that verifies that decidePolicyForNavigationAction is called for fragment navigations.
+
2017-12-07 Myles C. Maxfield <mmaxfi...@apple.com>
[Cocoa] Add SPI to disallow user-installed fonts
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm (225644 => 225645)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm 2017-12-07 21:46:11 UTC (rev 225644)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm 2017-12-07 21:59:48 UTC (rev 225645)
@@ -526,6 +526,41 @@
action = ""
}
+static size_t calls;
+static bool done;
+
+@interface DecidePolicyForNavigationActionFragmentDelegate : NSObject <WKNavigationDelegate>
+@end
+
+@implementation DecidePolicyForNavigationActionFragmentDelegate
+
+- (void)webView:(WKWebView *)webView decidePolicyForNavigationAction:(WKNavigationAction *)navigationAction decisionHandler:(void (^)(WKNavigationActionPolicy))decisionHandler
+{
+ decisionHandler(WKNavigationActionPolicyAllow);
+ const char* url = ""
+ switch (calls++) {
+ case 0:
+ EXPECT_STREQ(url, "http://webkit.org/");
+ return;
+ case 1:
+ EXPECT_STREQ(url, "http://webkit.org/#fragment");
+ done = true;
+ return;
+ }
+ ASSERT_NOT_REACHED();
+}
+
+@end
+
+TEST(WebKit, DecidePolicyForNavigationActionFragment)
+{
+ auto webView = adoptNS([[WKWebView alloc] init]);
+ auto delegate = adoptNS([[DecidePolicyForNavigationActionFragmentDelegate alloc] init]);
+ [webView setNavigationDelegate:delegate.get()];
+ [webView loadHTMLString:@"<script>window.location.href='';</script>" baseURL:[NSURL URLWithString:@"http://webkit.org"]];
+ TestWebKitAPI::Util::run(&done);
+}
+
#endif
#endif
