Bug 178267 – Clicks on Link with download attribute causes all (other) links to trigger download when clicked

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Jonas Ohlsson Aden]

Minimal reproducible test case here:
https://codepen.io/pocketjoso/pen/QqVOWy

I can reproduce on:
Safari 11.0 (12604.1.38.1.7)
macOS Sierra (10.12.6)

It does not happen in other browsers; I am not aware whether it happened in earlier Safari versions.

[Radar WebKit Bug Importer]

<rdar://problem/34985016>

[Chris Dumez]

In WebPageProxy.cpp:
#if ENABLE(DOWNLOAD_ATTRIBUTE)
    if (m_syncNavigationActionHasDownloadAttribute && action == PolicyAction::Use)
        action = PolicyAction::Download;
#endif

m_syncNavigationActionHasDownloadAttribute likely does not get reset.

[Chris Dumez]

Created attachment 323749 [details]
WIP Patch

[Chris Dumez]

Created attachment 323770 [details]
WIP Patch

[Build Bot]

Attachment 323770 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1362:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 323770 [details]
WIP Patch

Attachment 323770 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4852456

New failing tests:
http/tests/download/anchor-load-after-download.html

[Build Bot]

Created attachment 323781 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 323770 [details]
WIP Patch

Attachment 323770 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4852623

New failing tests:
imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name.html

[Build Bot]

Created attachment 323782 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 323770 [details]
WIP Patch

Attachment 323770 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4852749

New failing tests:
http/tests/download/anchor-load-after-download.html

[Build Bot]

Created attachment 323786 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 323770 [details]
WIP Patch

Attachment 323770 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4852995

New failing tests:
http/tests/download/anchor-load-after-download.html

[Build Bot]

Created attachment 323791 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 323844 [details]
Patch

[Sam Weinig]

Comment on attachment 323844 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323844&action=review

> Source/WebKit/UIProcess/API/APINavigation.h:73
> +    bool m_shouldForceDownload { false };

I'm not sure the should is getting you much here.  Maybe, just forceDownload?

[Chris Dumez]

(In reply to Sam Weinig from comment #15)
> Comment on attachment 323844 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323844&action=review
> 
> > Source/WebKit/UIProcess/API/APINavigation.h:73
> > +    bool m_shouldForceDownload { false };
> 
> I'm not sure the should is getting you much here.  Maybe, just forceDownload?

I thought this was part of our coding style for Booleans. I personally like it better with the should prefix too.

[Darin Adler]

Comment on attachment 323844 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323844&action=review

>>> Source/WebKit/UIProcess/API/APINavigation.h:73
>>> +    bool m_shouldForceDownload { false };
>> 
>> I'm not sure the should is getting you much here.  Maybe, just forceDownload?
> 
> I thought this was part of our coding style for Booleans. I personally like it better with the should prefix too.

The rule is inspired by Cocoa; it’s to help make sure that getters don’t sound like verb phrases. A function called “force download” doesn’t necessarily sound like a getter to everyone.

I like the way the "should" decreases ambiguity in the function name. And I also like booleans that work as a sentence "navigation should force download" as opposed to "navigation force download".

> LayoutTests/imported/w3c/ChangeLog:13
> +        * web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt:

Do these tests that rely things that have to time out slow down test runs overall, or is there some reason they don’t?

[Chris Dumez]

Comment on attachment 323844 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323844&action=review

>> LayoutTests/imported/w3c/ChangeLog:13
>> +        * web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt:
> 
> Do these tests that rely things that have to time out slow down test runs overall, or is there some reason they don’t?

No, I don't think these tests rely on such things. We are timing out on some of them but those are likely bugs in our implementation that I need to investigate.

[WebKit Commit Bot]

Comment on attachment 323844 [details]
Patch

Clearing flags on attachment: 323844

Committed r223413: <https://trac.webkit.org/changeset/223413>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jonas Ohlsson Aden]

Thanks for the work on this! When (in what version) in Safari can I expect to see this fixed?

[Chris Dumez]

(In reply to Jonas Ohlsson Aden from comment #21)
> Thanks for the work on this! When (in what version) in Safari can I expect
> to see this fixed?

No problem, thank you for the bug report.

Apple does not communicate when a particular fix will ship to customers.
I suggest validating the fix using the next release of Safari Technology Preview.