The HTML spec doesn't mandate that a user agent must download links with 'download' attributes. It says the decision to download or navigate to a hyperlink can be based on the user's preference. For links that have a 'download' attribute set, allowing web view to navigate (rather than download) isn't currently possible with the modern WebKit API because WebPageProxy::receivedNavigationPolicyDecision() makes the decision to convert any download-attribute-having navigation that the navigation delegate allowed into a download. It should be possible for clients to opt-out of this behavior and leave it entirely up to the navigation delegate whether the navigation action starts a navigation or a download.
<rdar://problem/48462642>
Created attachment 363262 [details] WIP
Comment on attachment 363262 [details] WIP This is certainly the quick and easy way to get this working, but I'm not sure what you will do if you want to decide to follow just this link but download others. You'll have to set this preference, call the WKNavigationDelegate completion handler, then set the preference back. That seems like a bad API design.
(In reply to Alex Christensen from comment #3) > Comment on attachment 363262 [details] > WIP > > This is certainly the quick and easy way to get this working, but I'm not > sure what you will do if you want to decide to follow just this link but > download others. If you want to download one link, then the navigation delegate would just specify _WKNavigationActionPolicyDownload. That will be translated to WebCore::PolicyAction::Download, and it will be downloaded just like normal. In other words, this line in WebPageProxy isn't a required part of getting a navigation action to turn into a download. It is simply the implementation of WebKit2's default policy that all "Use"'d navigations to links with 'download' attributes should be turned into downloads instead. My intent is to add a switch to disable that behavior and instead leave it entirely up to the navigation delegate. The SPI from 195121 will tell the navigation delegate about the presence/absence of the attribute, allowing it to make the right decision. > You'll have to set this preference, call the > WKNavigationDelegate completion handler, then set the preference back. That > seems like a bad API design. One could certainly do that... Though my intention is that a client that wants the ability to ask the user to View or Download one of these links could do so by: - Setting WKPreferences._downloadAttributeInitiatesDownloads = NO; - In -webView:decidePolicyForNavigationAction:decisionHandler: - If WKNavigationAction._shouldPerformDownload, ask the user what to do: - "I want to download it" => call decisionHandler(_WKNavigationActionPolicyDownload) - "I want to view it" => call decisionHandler(WKNavigationActionPolicyAllow)
That actually sounds pretty good. We could even make the default for this preference false
(In reply to Alex Christensen from comment #5) > That actually sounds pretty good. We could even make the default for this > preference false Ideally, I do think the default should be false, but I thought I would start this behavior out as opt-in, rather than getting all clients currently depending on the force-download behavior to adopt WKNavigationAction._shouldPerformDownload (or port-specific equivalent). Do you think this is ok as-is, with the default being true to maintain compatibility?
Created attachment 363307 [details] Patch
I'm not sure how important it is to maintain compatibility with a relatively recent compatibility-breaking change that nobody has complained about.
(In reply to Alex Christensen from comment #8) > I'm not sure how important it is to maintain compatibility with a relatively > recent compatibility-breaking change that nobody has complained about. There isn't a "recent" compatibility-breaking change I'm talking about. You might be thinking of rdar://48459714, but that's different. The issue here is that if we effectively disable DownloadAttributeInitiatesDownloads for everyone, then that will break 'download' links for clients that don't know that they now have to check the 'shouldPerformDownload' flag and manually use the 'Download' policy for such navigation actions. Other ports will need to add a "shouldPerformDownload" API on their navigation actions and get their clients' navigation delegates to check that. I don't know if this change in behavior is important enough to justify making it the universal default and getting clients to adapt to it.
Comment on attachment 363307 [details] Patch Adding a web preference does not seem like the best way to do this. We should consider other ways to keep old code compatible. Maybe a linked-on-or-after check? Maybe a new delegate method and leave the old one with the old meaning? Long term we don’t want to have to set a preference to get this behavior. I think this new behavior would be a good new future default.
(In reply to Darin Adler from comment #10) > Comment on attachment 363307 [details] > Patch > > Adding a web preference does not seem like the best way to do this. We > should consider other ways to keep old code compatible. Maybe a > linked-on-or-after check? Maybe a new delegate method and leave the old one > with the old meaning? > > Long term we don’t want to have to set a preference to get this behavior. I > think this new behavior would be a good new future default. Based on your and Alex's feedback, I think a linked-on-or-after check is the way to go here. We could consider some other API to add, but I can't think of any* that fit in better than an out-of-the-way signal (e.g. a preference in this patch) that you want the new behavior, or nothing at all (linked-on-or-after check). I'll add a linked-on-or-after check. Possible options might be: - A new WKNavigationActionPolicy, e.g. _WKNavigationActionPolicyAllowIgnoringDownloadAttribute. Adding a new policy in this case seems unnecessary when we could just trust the navigation delegate to make the right decision given the knowledge that the website wants to download the link. - A new WKNavigationDelegate method which is a copy of -decidePolicyForNavigationAction: but won't turn an 'Allow' into a 'Download'. This just feels wrong, since we're not asking the navigation delegate to do anything conceptually new, and encoding this change in behavior in the name (and preserving the old behavior for the existing method) seem really tedious. What would we call it: -webView:decidePolicyForNavigationActionAndIfYouTellUsToNavigateWeActuallyWill:decisionHandler? -webView:decidePolicyForNavigationActionV2:decisionHandler:? - A new WKNavigationDelegate method which asks the delegate to confirm before converting a 'Use' into a 'Download'. This API would be weirdly redundant - "Hey navigation delegate, you knew the website wanted to download this file, but you told me to navigate to it. Are you sure about that, or should I download it anyway?"
Created attachment 363641 [details] Patch v2
Comment on attachment 363641 [details] Patch v2 Attachment 363641 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11379510 New failing tests: fast/dom/HTMLAnchorElement/anchor-nodownload-set.html fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target.html imported/w3c/web-platform-tests/html/semantics/text-level-semantics/the-a-element/a-download-click.html fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes.html fast/dom/HTMLAnchorElement/anchor-download-synthetic-click.html fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-backslash.html fast/dom/HTMLAnchorElement/anchor-file-blob-download.html http/tests/download/anchor-download-redirect.html http/tests/download/anchor-download-no-extension.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-area-element/area-download-click.html http/tests/security/anchor-download-allow-sameorigin.html fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-base-target-popup-not-allowed.html fast/dom/HTMLAnchorElement/anchor-download-user-triggered-synthetic-click.html fast/dom/HTMLAnchorElement/anchor-file-blob-download-blank-target-popup-not-allowed.html http/wpt/html/semantics/text-level-semantics/the-a-element/a-download-click-404.html http/tests/download/anchor-load-after-download.html fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html imported/w3c/web-platform-tests/html/semantics/text-level-semantics/the-a-element/a-download-click-404.html http/tests/download/area-download.html http/tests/download/anchor-download-attribute-content-disposition.html fast/dom/HTMLAnchorElement/anchor-download.html fast/dom/HTMLAnchorElement/anchor-file-blob-download-no-extension.html http/tests/security/anchor-download-allow-data.html fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-doublequote.html http/tests/download/anchor-download-no-value.html fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-unicode.html http/tests/security/anchor-download-allow-blob.html
Created attachment 363645 [details] Archive of layout-test-results from ews106 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Created attachment 363658 [details] Patch v3 Attempt to fix test failures by making WebKitTestRunner download navigation actions from download links.
Comment on attachment 363658 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=363658&action=review > Source/WebKit/UIProcess/Cocoa/VersionChecks.h:36 > +#define DYLD_IOS_VERSION_FIRST_WHERE_DOWNLOAD_ATTRIBUTE_DOES_NOT_OVERRIDE_NAVIGATION_DELEGATE 0 These need to be added to WebKitAdditions before landing this.
Comment on attachment 363658 [details] Patch v3 Clearing flags on attachment: 363658 Committed r242521: <https://trac.webkit.org/changeset/242521>
All reviewed patches have been landed. Closing bug.