search

minbrowser / min

A fast, minimal browser that protects your privacy
https://minbrowser.org/
Apache License 2.0
7.72k stars 684 forks source link

Fix #2208 #2242

Closed Sestowner closed 8 months ago

PalmerAL commented 1 year ago

Can you explain how this works? I don't see why hasUserGesture would indicate whether the PDF is being saved from the context menu.

This section of code is meant to redirect download events so that if you click on a PDF link in a page, it opens the PDF viewer instead. In the main branch, this fails, because the download event is coming from the UI web contents, and so we send the PDF viewer request to the wrong place. In the multi-window branch (#2237), the download event now comes from the page's web contents, and so we actually open the viewer.

I think that a webpage could trigger a PDF download without a user gesture, in which case we do want to intercept that and show the viewer still. We probably need some other way of notifying the main process that this download is coming from the context menu.

Sestowner commented 1 year ago

Can you explain how this works?

hasUserGesture will return false when we call the downloadURL from the context menu, but true when the download button in the pdf viewer is clicked.

This section of code is meant to redirect download events...

So we can remove this section of code now?

PalmerAL commented 1 year ago

This code is identifying requests that should open in the PDF viewer, which is anything that is a PDF and doesn't have the special "pdfjs.action" string at the end of the URL.

hasUserGesture indicates whether the download was begun by the user. So for the context menu it will be false, since that's started by a call to an Electron API, and for the download button, it will be true, because Chromium knows that download event began as a result of a user click. But a webpage can also programmatically start a download:

var a = document.createElement('a')
a.href = ...
a.click()

In the normal case, this is handled by the other listener we have: https://github.com/minbrowser/min/blob/master/main/download.js#L73. But if the link is to something with content-disposition: attachment set, hasUserGesture will be false (I think).

How about we change the context menu to append the #pdfjs.action=download to the URL? It's not a very nice solution, but I think it will work in all cases.