Changeset 225645 in webkit

Timestamp:

Dec 7, 2017 1:59:48 PM (6 years ago)

Author:

achristensen@apple.com

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/tests/dom/document-fragment-expected.txt (added)
• LayoutTests/http/tests/dom/document-fragment.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/loader/FrameLoader.cpp (modified) (2 diffs)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2017-12-07  Alex Christensen  <achristensen@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  <youenn@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-12-07  Alex Christensen  <achristensen@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  <youenn@apple.com>
 

trunk/Source/WebCore/loader/FrameLoader.cpp

@@ -1316 +1316 @@
         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;
     }
@@ -1476 +1475 @@
         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;
     }

trunk/Tools/ChangeLog

@@ +1 @@
+2017-12-07  Alex Christensen  <achristensen@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  <mmaxfield@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm

@@ -527 +527 @@
 }
 
+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 = navigationAction.request.URL.absoluteString.UTF8String;
+    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='#fragment';</script>" baseURL:[NSURL URLWithString:@"http://webkit.org"]];
+    TestWebKitAPI::Util::run(&done);
+}
+
 #endif