⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 236987 in webkit


Ignore:
Timestamp:
Oct 9, 2018, 4:21:18 PM (8 years ago)
Author:
Chris Dumez
Message:

Anchor target should be ignored on activation when the download attribute is set
​https://bugs.webkit.org/show_bug.cgi?id=190408

Reviewed by Geoffrey Garen.

Source/WebCore:

Anchor target should be ignored on activation when the download attribute is set:

When the download attribute is set, we should use the "download the hyperlink" algorithm [1]
instead of the "follow the hyperlink" [2] algorithm.

Note that the "download the hyperlink" triggers a download and ignores the target attribute
entirely.

This is important as an anchor element with target=_blank and the download attribute set may
fail because of Safari's popup blocker if we do not disregard the anchor target.

[1] ​https://html.spec.whatwg.org/#downloading-hyperlinks
[2] ​https://html.spec.whatwg.org/#following-hyperlinks-2

Tests: fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-base-target-popup-not-allowed.html

fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target-popup-not-allowed.html

  • loader/FrameLoader.cpp:

(WebCore::FrameLoader::loadURL):

Tools:

Update WebKitTestRunner to block popups by default and add support for testRunner.setCanOpenWindows()
for tests that need popups. This aligns WebKitTestRunner with DumpRenderTree and allows testing
popup blocking.

  • WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:
  • WebKitTestRunner/InjectedBundle/TestRunner.cpp:

(WTR::TestRunner::setCanOpenWindows):

  • WebKitTestRunner/InjectedBundle/TestRunner.h:
  • WebKitTestRunner/TestController.cpp:

(WTR::TestController::createOtherPage):

  • WebKitTestRunner/TestController.h:
  • WebKitTestRunner/TestInvocation.cpp:

(WTR::TestInvocation::didReceiveSynchronousMessageFromInjectedBundle):

  • WebKitTestRunner/TestInvocation.h:

LayoutTests:

  • fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-base-target-popup-not-allowed-expected.txt: Added.
  • fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-base-target-popup-not-allowed.html: Added.
  • fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target-popup-not-allowed-expected.txt: Added.
  • fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target-popup-not-allowed.html: Added.

Add layout test coverage.

  • http/tests/download/anchor-load-after-download.html:
  • http/tests/workers/service/resources/registration-task-queue-scheduling-1.js:

Update existing WK2-only layout tests to call testRunner.setCanOpenWindows() since they rely on
opening a new window. We failed to notice the issue because these tests are not run on WK1.

  • platform/ios-wk2/TestExpectations:
  • platform/mac-wk1/TestExpectations:
  • platform/win/TestExpectations:

Skip new tests on WebKit1 since the download attribute is only supported on WebKit2.

Location:
trunk
Files:
4 added
16 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r236980 r236987  
     12018-10-09  Chris Dumez  <cdumez@apple.com>
     2
     3        Anchor target should be ignored on activation when the download attribute is set
     4        https://bugs.webkit.org/show_bug.cgi?id=190408
     5
     6        Reviewed by Geoffrey Garen.
     7
     8        * fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-base-target-popup-not-allowed-expected.txt: Added.
     9        * fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-base-target-popup-not-allowed.html: Added.
     10        * fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target-popup-not-allowed-expected.txt: Added.
     11        * fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target-popup-not-allowed.html: Added.
     12        Add layout test coverage.
     13
     14        * http/tests/download/anchor-load-after-download.html:
     15        * http/tests/workers/service/resources/registration-task-queue-scheduling-1.js:
     16        Update existing WK2-only layout tests to call testRunner.setCanOpenWindows() since they rely on
     17        opening a new window. We failed to notice the issue because these tests are not run on WK1.
     18
     19        * platform/ios-wk2/TestExpectations:
     20        * platform/mac-wk1/TestExpectations:
     21        * platform/win/TestExpectations:
     22        Skip new tests on WebKit1 since the download attribute is only supported on WebKit2.
     23
    1242018-10-09  Chris Dumez  <cdumez@apple.com>
    225

  • trunk/LayoutTests/http/tests/download/anchor-load-after-download.html

    r223413 r236987  
    55if (window.testRunner) {
    66  testRunner.dumpAsText();
     7  testRunner.setCanOpenWindows();
    78  testRunner.waitUntilDone();
    89}

  • trunk/LayoutTests/http/tests/workers/service/resources/registration-task-queue-scheduling-1.js

    r226722 r236987  
    11// This test makes sure that two different windows from two different domains who spam SW registrations get responses intertwined with each other
    22// as each registration should operate on its own task queue.
     3
     4if (window.testRunner)
     5    testRunner.setCanOpenWindows();
    36
    47if (location.hostname != "127.0.0.1")

  • trunk/LayoutTests/platform/ios-wk2/TestExpectations

    r236980 r236987  
    10681068webkit.org/b/156067 fast/dom/HTMLAnchorElement/anchor-file-blob-convert-to-download-async-delegate.html [ Skip ]
    10691069webkit.org/b/156067 fast/dom/HTMLAnchorElement/anchor-file-blob-download.html [ Skip ]
     1070webkit.org/b/156067 fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-base-target-popup-not-allowed.html [ Skip ]
    10701071webkit.org/b/156067 fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target.html [ Skip ]
     1072webkit.org/b/156067 fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target-popup-not-allowed.html [ Skip ]
    10711073webkit.org/b/156067 fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-backslash.html [ Skip ]
    10721074webkit.org/b/156067 fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-doublequote.html [ Skip ]

  • trunk/LayoutTests/platform/mac-wk1/TestExpectations

    r236970 r236987  
    308308webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-download-user-triggered-synthetic-click.html [ Skip ]
    309309webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download.html [ Skip ]
     310webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-base-target-popup-not-allowed.html [ Skip ]
    310311webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target.html [ Skip ]
     312webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target-popup-not-allowed.html [ Skip ]
    311313webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-backslash.html [ Skip ]
    312314webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-doublequote.html [ Skip ]

  • trunk/LayoutTests/platform/win/TestExpectations

    r236970 r236987  
    456456fast/dom/HTMLAnchorElement/anchor-download-user-triggered-synthetic-click.html [ Skip ]
    457457fast/dom/HTMLAnchorElement/anchor-file-blob-download.html [ Skip ]
     458fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-base-target-popup-not-allowed.html [ Skip ]
    458459fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target.html [ Skip ]
     460fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target-popup-not-allowed.html [ Skip ]
    459461fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-backslash.html [ Skip ]
    460462fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-doublequote.html [ Skip ]

  • trunk/Source/WebCore/ChangeLog

    r236985 r236987  
     12018-10-09  Chris Dumez  <cdumez@apple.com>
     2
     3        Anchor target should be ignored on activation when the download attribute is set
     4        https://bugs.webkit.org/show_bug.cgi?id=190408
     5
     6        Reviewed by Geoffrey Garen.
     7
     8        Anchor target should be ignored on activation when the download attribute is set:
     9        - https://html.spec.whatwg.org/#the-a-element:downloading-hyperlinks-2
     10
     11        When the download attribute is set, we should use the "download the hyperlink" algorithm [1]
     12        instead of the "follow the hyperlink" [2] algorithm.
     13
     14        Note that the "download the hyperlink" triggers a download and ignores the target attribute
     15        entirely.
     16
     17        This is important as an anchor element with target=_blank and the download attribute set may
     18        fail because of Safari's popup blocker if we do not disregard the anchor target.
     19
     20        [1] https://html.spec.whatwg.org/#downloading-hyperlinks
     21        [2] https://html.spec.whatwg.org/#following-hyperlinks-2
     22
     23        Tests: fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-base-target-popup-not-allowed.html
     24               fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target-popup-not-allowed.html
     25
     26        * loader/FrameLoader.cpp:
     27        (WebCore::FrameLoader::loadURL):
     28
    1292018-10-09  Michael Catanzaro  <mcatanzaro@igalia.com>
    230

  • trunk/Source/WebCore/loader/FrameLoader.cpp

    r236970 r236987  
    13231323    Ref<Frame> protect(m_frame);
    13241324
    1325     String frameName = frameLoadRequest.frameName();
     1325    // Anchor target is ignored when the download attribute is set since it will download the hyperlink rather than follow it.
     1326    String effectiveFrameName = frameLoadRequest.downloadAttribute().isNull() ? frameLoadRequest.frameName() : String();
    13261327    AllowNavigationToInvalidURL allowNavigationToInvalidURL = frameLoadRequest.allowNavigationToInvalidURL();
    13271328    NewFrameOpenerPolicy openerPolicy = frameLoadRequest.newFrameOpenerPolicy();
    … …  
    13461347
    13471348    // The search for a target frame is done earlier in the case of form submission.
    1348     Frame* targetFrame = isFormSubmission ? nullptr : findFrameForNavigation(frameName);
     1349    Frame* targetFrame = isFormSubmission ? nullptr : findFrameForNavigation(effectiveFrameName);
    13491350    if (targetFrame && targetFrame != &m_frame) {
    13501351        frameLoadRequest.setFrameName("_self");
    … …  
    13611362    action.setHasOpenedFrames(!m_openedFrames.isEmpty());
    13621363
    1363     if (!targetFrame && !frameName.isEmpty()) {
     1364    if (!targetFrame && !effectiveFrameName.isEmpty()) {
    13641365        action = action.copyWithShouldOpenExternalURLsPolicy(shouldOpenExternalURLsPolicyToApply(m_frame, frameLoadRequest));
    1365         policyChecker().checkNewWindowPolicy(WTFMove(action), WTFMove(request), WTFMove(formState), frameName, [this, allowNavigationToInvalidURL, openerPolicy, completionHandler = completionHandlerCaller.release()] (const ResourceRequest& request, WeakPtr<FormState>&& formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) mutable {
     1366        policyChecker().checkNewWindowPolicy(WTFMove(action), WTFMove(request), WTFMove(formState), effectiveFrameName, [this, allowNavigationToInvalidURL, openerPolicy, completionHandler = completionHandlerCaller.release()] (const ResourceRequest& request, WeakPtr<FormState>&& formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) mutable {
    13661367            continueLoadAfterNewWindowPolicy(request, formState.get(), frameName, action, shouldContinue, allowNavigationToInvalidURL, openerPolicy);
    13671368            completionHandler();

  • trunk/Tools/ChangeLog

    r236984 r236987  
     12018-10-09  Chris Dumez  <cdumez@apple.com>
     2
     3        Anchor target should be ignored on activation when the download attribute is set
     4        https://bugs.webkit.org/show_bug.cgi?id=190408
     5
     6        Reviewed by Geoffrey Garen.
     7
     8        Update WebKitTestRunner to block popups by default and add support for testRunner.setCanOpenWindows()
     9        for tests that need popups. This aligns WebKitTestRunner with DumpRenderTree and allows testing
     10        popup blocking.
     11
     12        * WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:
     13        * WebKitTestRunner/InjectedBundle/TestRunner.cpp:
     14        (WTR::TestRunner::setCanOpenWindows):
     15        * WebKitTestRunner/InjectedBundle/TestRunner.h:
     16        * WebKitTestRunner/TestController.cpp:
     17        (WTR::TestController::createOtherPage):
     18        * WebKitTestRunner/TestController.h:
     19        * WebKitTestRunner/TestInvocation.cpp:
     20        (WTR::TestInvocation::didReceiveSynchronousMessageFromInjectedBundle):
     21        * WebKitTestRunner/TestInvocation.h:
     22
    1232018-10-09  Jer Noble  <jer.noble@apple.com>
    224

  • trunk/Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl

    r236690 r236987  
    6767    void keepWebHistory();
    6868    void setAcceptsEditing(boolean value);
    69     void setCanOpenWindows(boolean value);
     69    void setCanOpenWindows();
    7070    void setCloseRemainingWindowsWhenComplete(boolean value);
    7171    void setXSSAuditorEnabled(boolean value);

  • trunk/Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp

    r236954 r236987  
    411411}
    412412
    413 void TestRunner::setCanOpenWindows(bool)
    414 {
    415     // The test plugins/get-url-with-blank-target.html requires that the embedding client forbid
    416     // opening windows (by omitting a call to this function) so as to test that NPN_GetURL()
    417     // with a blank target will return an error.
    418     //
    419     // It is not clear if we should implement this functionality or remove it and plugins/get-url-with-blank-target.html
    420     // per the remark in <https://trac.webkit.org/changeset/64504/trunk/LayoutTests/platform/mac-wk2/Skipped>.
    421     // For now, just ignore this setting.
     413void TestRunner::setCanOpenWindows()
     414{
     415    WKRetainPtr<WKStringRef> messsageName(AdoptWK, WKStringCreateWithUTF8CString("SetCanOpenWindows"));
     416    WKRetainPtr<WKBooleanRef> messageBody(AdoptWK, WKBooleanCreate(true));
     417    WKBundlePostSynchronousMessage(InjectedBundle::singleton().bundle(), messsageName.get(), messageBody.get(), nullptr);
    422418}
    423419

  • trunk/Tools/WebKitTestRunner/InjectedBundle/TestRunner.h

    r236954 r236987  
    100100    void keepWebHistory();
    101101    void setAcceptsEditing(bool value) { m_shouldAllowEditing = value; }
    102     void setCanOpenWindows(bool);
     102    void setCanOpenWindows();
    103103    void setCloseRemainingWindowsWhenComplete(bool value) { m_shouldCloseExtraWindows = value; }
    104104    void setXSSAuditorEnabled(bool);

  • trunk/Tools/WebKitTestRunner/TestController.cpp

    r236720 r236987  
    243243}
    244244
    245 WKPageRef TestController::createOtherPage(WKPageRef oldPage, WKPageConfigurationRef configuration, WKNavigationActionRef navigationAction, WKWindowFeaturesRef windowFeatures, const void *clientInfo)
     245WKPageRef TestController::createOtherPage(WKPageRef, WKPageConfigurationRef configuration, WKNavigationActionRef navigationAction, WKWindowFeaturesRef windowFeatures, const void *clientInfo)
    246246{
    247247    PlatformWebView* parentView = static_cast<PlatformWebView*>(const_cast<void*>(clientInfo));
     248    return TestController::singleton().createOtherPage(parentView, configuration, navigationAction, windowFeatures);
     249}
     250
     251WKPageRef TestController::createOtherPage(PlatformWebView* parentView, WKPageConfigurationRef configuration, WKNavigationActionRef navigationAction, WKWindowFeaturesRef windowFeatures)
     252{
     253    // The test needs to call testRunner.setCanOpenWindows() to open new windows.
     254    if (!m_currentInvocation->canOpenWindows())
     255        return nullptr;
    248256
    249257    PlatformWebView* view = platformCreateOtherPage(parentView, configuration, parentView->options());

  • trunk/Tools/WebKitTestRunner/TestController.h

    r236690 r236987  
    401401
    402402    static WKPageRef createOtherPage(WKPageRef, WKPageConfigurationRef, WKNavigationActionRef, WKWindowFeaturesRef, const void*);
     403    WKPageRef createOtherPage(PlatformWebView* parentView, WKPageConfigurationRef, WKNavigationActionRef, WKWindowFeaturesRef);
    403404
    404405    static void runModal(WKPageRef, const void* clientInfo);

  • trunk/Tools/WebKitTestRunner/TestInvocation.cpp

    r236690 r236987  
    803803        return WKRetainPtr<WKTypeRef>(AdoptWK, WKBooleanCreate(m_dumpFrameLoadCallbacks));
    804804
     805    if (WKStringIsEqualToUTF8CString(messageName, "SetCanOpenWindows")) {
     806        ASSERT(WKGetTypeID(messageBody) == WKBooleanGetTypeID());
     807        m_canOpenWindows = static_cast<unsigned char>(WKBooleanGetValue(static_cast<WKBooleanRef>(messageBody)));
     808        return nullptr;
     809    }
     810
    805811    if (WKStringIsEqualToUTF8CString(messageName, "SetWindowIsKey")) {
    806812        ASSERT(WKGetTypeID(messageBody) == WKBooleanGetTypeID());

  • trunk/Tools/WebKitTestRunner/TestInvocation.h

    r235690 r236987  
    8585   
    8686    void dumpResourceLoadStatistics();
     87
     88    bool canOpenWindows() const { return m_canOpenWindows; }
    8789   
    8890private:
    … …  
    133135    bool m_pixelResultIsPending { false };
    134136    bool m_shouldDumpResourceLoadStatistics { false };
     137    bool m_canOpenWindows { false };
    135138    WhatToDump m_whatToDump { WhatToDump::RenderTree };
    136139
Note: See TracChangeset for help on using the changeset viewer.