| Summary: | Clicks on Link with download attribute causes all (other) links to trigger download when clicked | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Jonas Ohlsson Aden <jonas.sebastian.ohlsson> |
| Component: | UI Events | Assignee: | Chris Dumez <cdumez> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | bfulgham, buildbot, cdumez, commit-queue, darin, jond, rniwa, sam, webkit-bug-importer |
| Priority: | P2 | Keywords: | InRadar |
| Version: | Safari 11 | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Attachments: | |||
|
Description
Jonas Ohlsson Aden
2017-10-13 10:30:03 PDT
In WebPageProxy.cpp:
#if ENABLE(DOWNLOAD_ATTRIBUTE)
if (m_syncNavigationActionHasDownloadAttribute && action == PolicyAction::Use)
action = PolicyAction::Download;
#endif
m_syncNavigationActionHasDownloadAttribute likely does not get reset.
Created attachment 323749 [details]
WIP Patch
Created attachment 323770 [details]
WIP Patch
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.
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 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
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 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
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 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
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 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
Created attachment 323844 [details]
Patch
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? (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. 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? 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. Comment on attachment 323844 [details] Patch Clearing flags on attachment: 323844 Committed r223413: <https://trac.webkit.org/changeset/223413> All reviewed patches have been landed. Closing bug. Thanks for the work on this! When (in what version) in Safari can I expect to see this fixed? (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. |