Closed Bug 1395207 Opened 3 years ago Closed 5 months ago

downloads.download({saveAs:true}) does not remember recently used directory when prompting for save

Categories

(WebExtensions :: General, enhancement, P5)

57 Branch
enhancement

Tracking

(firefox78 fixed)

RESOLVED FIXED
mozilla78
Tracking Status
firefox78 --- fixed

People

(Reporter: mark, Assigned: mcs, Mentored)

References

Details

(Whiteboard: [downloads])

Attachments

(2 files, 2 obsolete files)

When using downloads.download({saveAs:true}) the save dialog always starts in the default download folder.

When using "Save Image As..." context menu item, the save dialog remembers the last used directory per domain.

It would be nice if the downloads.download() method behaved similarly to "Save Image As..." when the saveAs prompt is requested.

In Foxy Gestures, I have a "Save Media As" gesture that is analogous to the "Save Images As...". The fact that downloads.download() does not remember the last used directory is annoying to my users. See https://github.com/marklieberman/foxygestures/issues/41.

Also, it would be nice if you could just provide a starting directory to downloads.download({saveAs:true,filename:'...'}) rather than having to specify a complete filename.
Status: UNCONFIRMED → NEW
Component: Untriaged → Downloads API
Ever confirmed: true
Product: Firefox → Toolkit
Version: 53 Branch → 55 Branch
[Tracking Requested - why for this release]:
IMO, downloads API would be useful to many web extensions and having to chose the downloads directory every single time is a very noticeable UX regression.

Feel free to untrack this for 57 if the feature request is not important enough.
Version: 55 Branch → 57 Branch
Andy, can you take a look at this and/or triage it in your next meeting (via https://groups.google.com/forum/?fromgroups=&hl=en#!topic/firefox-dev/_UDEcGpiLXQ ) ? Thanks!
Component: Downloads API → WebExtensions: Untriaged
Flags: needinfo?(amckay)
Flags: needinfo?(amckay)
Priority: -- → P5
Whiteboard: [downloads]
Duplicate of this bug: 1431453
Product: Toolkit → WebExtensions
Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958
Component: Untriaged → General

Just stepped in this in FF 66. It's indeed very anoying. It seems strange that the "Save as" box behaves differently when called from the API or when call by clicking on a link. Falling back to the default download directory every time makes the Save-as-box quite unusable for many scenarious. There should be a flag for specifying if the filename should be relative to the default download location (as documented and is now) or to remember the last used directory (as the standard Save-as-box).

The problem is likely one level deeper: the download function only allows filename inside the default download directory, so I guess that's why the SaveAs box does not start with any other location than the default directory.
Doesn't seems to make any sense. If you can choose any directory with the SaveAs dialog, why shouldn't you be allowed to select an arbitrary path in the download function? Permissions for directories are the same in the function and the SaveAs dialog.

Attached patch proposed fix (work in progress) (obsolete) — Splinter Review

Here is a "work in progress" patch (it lacks tests and may need other work). It adds a rememberLastDirectory Boolean option to the downloads.download() function. If it is true and saveAs is also true, the last directory chosen in the file picker is used as the default the next time the file picker is displayed.

But before I or someone else works on this further, would Mozilla accept such a patch? For our Page Saver extension, this is by far the most frequently requested feature; most people rightly consider it a bug that severely affects usability.

Assignee: nobody → mcs
Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies)
Whiteboard: [downloads] → [downloads], webext?

Rob, could you take a look here, maybe offer an opinion? Looks pretty reasonable to me.

Flags: needinfo?(rob)

There is a security risk involved with remembering the last selected location. By design, the downloads.download API only supports saving to the downloads directory or a subdirectory. Using a well-known directory ensures that an extension does not inadvertently have access to a larger part of the filesystem. With the proposed patch, if the user decides to save a file to their home directory, then the extension can write to any file or subdirectory in their home directory, without further user interaction. This poses a significant threat to the integrity of the user's filesystem.

In contrast, the last directory of <input type=file> is remembered, separately for each website. This is reasonably safe, because the web page has no control over the chosen directory, and files have unique names (and users are prompted if there was an existing file).

I would be more favourable towards a patch for this feature request if malicious extensions cannot silently overwrite arbitrary files.

Flags: needinfo?(rob)

@Rob(In reply to Rob Wu [:robwu] from comment #9)

There is a security risk involved with remembering the last selected location. By design, the downloads.download API only supports saving to the downloads directory or a subdirectory. Using a well-known directory ensures that an extension does not inadvertently have access to a larger part of the filesystem. With the proposed patch, if the user decides to save a file to their home directory, then the extension can write to any file or subdirectory in their home directory, without further user interaction.

The proposal is that the saveAs flag has to be true for this feature to work. Thus, the dialog prompt will happen and user input will be required. I don't see a way that extension can do something silently.

The proposal is that the saveAs flag has to be true for this feature to work. Thus, the dialog prompt will happen and user input will be required. I don't see a way that extension can do something silently.

In contrast, the last directory of <input type=file> is remembered, separately for each website. This is reasonably safe, because the web page has no control over the chosen directory, and files have unique names (and users are prompted if there was an existing file).

I agree that the behavior of this patch doesn't appear to be a risk as is (and would be an improvement on the current state of things), but for the sake of UX consistency, I do think it would be nice if Firefox could replicate the directory storage behavior of a traditional download request when an extension requests a download (i.e., share the last download location between manual and extension requests). I'm not familiar enough with the Firefox code base to say whether that option is currently feasible, but from a user-perspective, there's no logical reason why there should be any difference between requesting a download via the context menu and requesting a download via an extension, so any variance between the two is likely to draw confusion and bug reports.

FYI: The tracking of "last directory" appears to be implemented via the DownloadLastDir helper in https://searchfox.org/mozilla-central/rev/581466eef9269afb03d8a0dba2f50215f3a9026c/toolkit/mozapps/downloads/DownloadLastDir.jsm , which relies on the content pref service to persist the setting, whose interface is defined in https://searchfox.org/mozilla-central/rev/581466eef9269afb03d8a0dba2f50215f3a9026c/dom/interfaces/base/nsIContentPrefService2.idl#44-102 .

If you are going to use this interface, you should probably use the extension's origin (opposed to the URL of the download), to not affect the "last directory" of unrelated sites.

(In reply to Girish Sharma [:Optimizer] from comment #10)

@Rob(In reply to Rob Wu [:robwu] from comment #9)

There is a security risk involved with remembering the last selected location. By design, the downloads.download API only supports saving to the downloads directory or a subdirectory. Using a well-known directory ensures that an extension does not inadvertently have access to a larger part of the filesystem. With the proposed patch, if the user decides to save a file to their home directory, then the extension can write to any file or subdirectory in their home directory, without further user interaction.

The proposal is that the saveAs flag has to be true for this feature to work. Thus, the dialog prompt will happen and user input will be required. I don't see a way that extension can do something silently.

Yes, that was the intent of my patch: this new feature will only have an effect if the saveas path is taken (which means there will be user interaction). I am aware of the security risks associated with allowing extensions to save to arbitrary locations without user interaction and we should avoid those risks. I would rather provide the simple solution which is embodied in my patch: (1) user interaction is required and (2) there is no persistent storage of the chosen location. Item (2) will prevent surprises such as users seeing an old directory when they use an extension only rarely (Firefox updates, and therefore restarts, happen frequently enough to avoid this). Remembering the last selected directory will remove a large pain point for many users who perform a series of actions using the same extension, e.g., a series of related page captures, a series of related video downloads, and so on.

Type: defect → enhancement
Whiteboard: [downloads], webext? → [downloads]

I've discussed this with the rest of the team.

We are favorable towards this feature, but we'd like to not have a new option in the downloads.download API. Instead, the default behavior of saveAs would be changed, to remember the most recently used directory, specific to the extension (see comment 12).

Note: directories are not allowed when the "last directory" feature is used. To avoid breaking add-ons that rely on that, keep the current behavior if the filename parameter contains a path separator.

Mentor: rob

Looks like this solution would also address the points raised in Bug 1342563.

I finally found some time to work on this again. This new patch attempts to implement what Rob requested in comment 14. Feedback would be appreciated.

Attachment #9110593 - Attachment is obsolete: true
Flags: needinfo?(rob)

Please submit your patch to Phabricator (and add me as a reviewer) - https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Submitting_a_Patch

Flags: needinfo?(rob)

When the saveAs option is true, instead of always starting in the default
download directory, remember and reuse the last directory that the user
chose. The content preferences service is used to store the path on a
per-extension basis.

When the saveAs option is true, instead of always starting in the
default download directory, remember and reuse the last directory
that the user chose. Unless browser.download.lastDir.savePerSite is
set to false, the last download directory is saved on a per-extension
basis.

Please update the existing Phabricator revision instead of creating a new one. Otherwise the review history will be lost.

If you don't know how to do so, let me know which tool you're using to upload patches and I can provide some advice.

(In reply to Rob Wu [:robwu] from comment #20)

Please update the existing Phabricator revision instead of creating a new one. Otherwise the review history will be lost.

Sorry. I did not intend to create a new Phabricator revision, and I am not sure how to undo my mistake.

If you don't know how to do so, let me know which tool you're using to upload patches and I can provide some advice.

Thanks. I am using moz-phab with a a git repo. I probably need to adapt my usual workflow to work better with moz-phab. Maybe the problem is that I created a new branch for the revised patch? Should I instead make a second commit on my original branch, e.g., a fixup commit? Or amend the original commit? (which is not something I would normally do once a patch is published in any way)

moz-phab does not squash commits. It mirrors the commits to revisions on Phabricator. Phabricator will keep track of the commits and show interdiffs.

Locally you can have a branch, but you should use git commit -v --amend (after adding the desired changes) to update the patch before pushing it to Phabricator.
Since you have already commited the changes, use git rebase to squash the commits together (do edit the commit message to avoid some junk in the commit description).

moz-phab will edit the commit to add the URL to the Phabricator revision in the commit message (Differential Revision: <URL>) when it is not present. So if you keep the first commit message and squash the later commits in the first commit, then you should be able to update the original revision with moz-phab.

To mark the second phabricator revision as obsolete, go to Phabricator's web interface for the revision, scroll to the bottom, choose "Abandon" in the dropdown menu, then submit the changes.

Attachment #9136800 - Attachment is obsolete: true

:mcs, The original patch at https://phabricator.services.mozilla.com/D67060 has not been updated since my last feedback. Did you mean to update it with the changes that you accidentally published as a separate patch?

(In reply to Rob Wu [:robwu] from comment #23)

:mcs, The original patch at https://phabricator.services.mozilla.com/D67060 has not been updated since my last feedback. Did you mean to update it with the changes that you accidentally published as a separate patch?

I updated it now. I was just busy with other work for a couple of days.

Rob, do you have time to look at my revised patch? If not, can you suggest someone else? Thanks!

Flags: needinfo?(rob)

Thanks for the reminder and the patience. I usually respond sooner, but this slept through the cracks. Next time, feel free to ping me if you don't get a response within a few days.

I'll take a look at this on Thursday.

Flags: needinfo?(rob)
Duplicate of this bug: 1635120
Pushed by rmaries@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ea44b224ac1
downloads.download() should remember the last directory used. r=robwu
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
See Also: → 1316394
You need to log in before you can comment on or make changes to this bug.