Bug 195145 – [WK2] It should be possible to follow links with 'download' attributes

• Component: WebKit2
• Status: RESOLVED FIXED

[David Quesada]

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.

[Radar WebKit Bug Importer]

<rdar://problem/48462642>

[David Quesada]

Created attachment 363262 [details]
WIP

[Alex Christensen]

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.

[David Quesada]

(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)

[Alex Christensen]

That actually sounds pretty good.  We could even make the default for this preference false

[David Quesada]

(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?

[David Quesada]

Created attachment 363307 [details]
Patch

[Alex Christensen]

I'm not sure how important it is to maintain compatibility with a relatively recent compatibility-breaking change that nobody has complained about.

[David Quesada]

(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.

[Darin Adler]

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.

[David Quesada]

(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?"

[David Quesada]

Created attachment 363641 [details]
Patch v2

[EWS Watchlist]

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

[EWS Watchlist]

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

[David Quesada]

Created attachment 363658 [details]
Patch v3

Attempt to fix test failures by making WebKitTestRunner download navigation actions from download links.

[Alex Christensen]

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.

[WebKit Commit Bot]

Comment on attachment 363658 [details]
Patch v3

Clearing flags on attachment: 363658

Committed r242521: <https://trac.webkit.org/changeset/242521>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.