1750533 - "Delete" context menu could handle read-only files more gracefully

Status: NEW

(Firefox :: Downloads Panel, enhancement, P5)

Description

[Masatoshi Kimura [:emk]]

Because otherwise users explicitly set the read-only attribute. Although File Explorer ignores the read-only attribute, it will move the file to Recycle Bin.

Comment 1

[:Gijs (he/him)]

I'm pretty confused. Under what circumstance is the read only attribute set, if the file isn't in the temp directory, and what happens if it doesn't get overwritten and the user attempts to delete the file from Firefox's UI? In particular, if your aim is that in that case it doesn't get deleted, how do we communicate that to the user?

Comment 2

[Masatoshi Kimura [:emk]]

(In reply to :Gijs (he/him) from comment #1)

I'm pretty confused. Under what circumstance is the read only attribute set, if the file isn't in the temp directory,

The read only attribute is not automatically set. So the user explicitly set the attribute. So Firefox should not silently remove the attribute. See bug 1746386 for more context.

and what happens if it doesn't get overwritten and the user attempts to delete the file from Firefox's UI? In particular, if your aim is that in that case it doesn't get deleted, how do we communicate that to the user?

Maybe disable the "Delete" menu item?

Comment 3

[:Gijs (he/him)]

(In reply to Masatoshi Kimura [:emk] from comment #2)

(In reply to :Gijs (he/him) from comment #1)

I'm pretty confused. Under what circumstance is the read only attribute set, if the file isn't in the temp directory,

The read only attribute is not automatically set. [...] See bug 1746386 for more context.

FWIW, I am not convinced that this is true without doing a lot more digging which I don't have time to do right now. The code linked from bug 1746386 claiming this depends on the new download improvement pref, https://searchfox.org/mozilla-central/rev/72c7cef167829b6f1e24cae216fa261934c455fc/toolkit/components/downloads/DownloadCore.jsm#606-615 , only covers deleting the file when Firefox closes or private browsing mode ends. With what permissions the file is created seems like a separate matter, and will likely depend on what method was used to create the download (ie a network request whose response uses Content-Disposition: attachment triggering a download doesn't use the exact same codepath as "save link as", "save image as", or "save page as", screenshots, and webextension-generated downloads).

So I don't know that the attribute being set is never going to be Firefox's own fault, for one.

So the user explicitly set the attribute. So Firefox should not silently remove the attribute.

Well, the user also explicitly tells Firefox "delete this file"...

I also wonder what happens if the attribute is set on the directory, does Windows inherit it into files saved there?

and what happens if it doesn't get overwritten and the user attempts to delete the file from Firefox's UI? In particular, if your aim is that in that case it doesn't get deleted, how do we communicate that to the user?

Maybe disable the "Delete" menu item?

I don't think that communicates anything, apart from "you can't do this". The user would be left to infer why that would be the case, and would probably blame Firefox, especially if deleting the file is still possible from Windows Explorer. We'd also need to specialcase situations where the download error'd out or was paused, and there is only a .part file.
I think it'd be clearer if we provided a user-readable error if the entry was used, like "This file is marked read-only; are you sure you want to delete it?" or whatever - and even that seems like it might not be particularly user-friendly.

Besides being unclear that we can really rely on the attribute communicating user intent rather than confusing default file permissions generated by Firefox, I also don't understand what the motivation is for users to set files as read only. Under what circumstances do users really go into Windows Explorer, and single out individual downloaded files, right click them, open their file properties, and manually toggle the "read only" flag? Why would anyone do that, especially if Windows Explorer would then still be quite happy to delete said files? Is there some other tool that does this in a more user-friendly manner? What kind of workflow would include this and is it likely to be particularly common?

Comment 4

[Masatoshi Kimura [:emk]]

(In reply to :Gijs (he/him) from comment #3)

FWIW, I am not convinced that this is true without doing a lot more digging which I don't have time to do right now. The code linked from bug 1746386 claiming this depends on the new download improvement pref, https://searchfox.org/mozilla-central/rev/72c7cef167829b6f1e24cae216fa261934c455fc/toolkit/components/downloads/DownloadCore.jsm#606-615 , only covers deleting the file when Firefox closes or private browsing mode ends. With what permissions the file is created seems like a separate matter, and will likely depend on what method was used to create the download (ie a network request whose response uses Content-Disposition: attachment triggering a download doesn't use the exact same codepath as "save link as", "save image as", or "save page as", screenshots, and webextension-generated downloads).

So I don't know that the attribute being set is never going to be Firefox's own fault, for one.

If the Firefox sets the read-only attribute of the non-temp files, it should be considered as a bug IMO.

I also wonder what happens if the attribute is set on the directory, does Windows inherit it into files saved there?

Windows ignores the read-only attribute of the directory.

I also don't understand what the motivation is for users to set files as read only. Under what circumstances do users really go into Windows Explorer, and single out individual downloaded files, right click them, open their file properties, and manually toggle the "read only" flag?

I'm actually doing that everyday. I sort the download directory by date, select all new files, and toggle the checkbox. (File Explorer can change multiple files' attribute at once.)

Why would anyone do that, especially if Windows Explorer would then still be quite happy to delete said files? Is there some other tool that does this in a more user-friendly manner? What kind of workflow would include this and is it likely to be particularly common?

Although File Explorer ignores the attribute, many apps still honor the attribute. For example, Notepad cannot overwrite read-only files. Standard File Save dialogs cannot overwrite read-only files. Usually command-line tools honor the read-only files. If a zip file has the read-only attribute, it will prevent files from being added unexpectedly.

Comment 5

[Shane Hughes [:aminomancer]]

explorer.exe doesn't stop me from deleting a read-only file. Also, if I move it to the recycle bin as read-only, nothing stops me from emptying the bin. My understanding of read-only on Windows is just that it stops you from writing to the file. Doesn't stop anything from deleting it. Firefox's delete menuitem isn't saving or overwriting the file, it's just deleting it. I think if you wanted to make a file protected from deletion you should use the user/group permissions feature instead. Same for macOS, not sure about Linux.

It seems confusing if you right click > "Delete" and the file is not deleted, and the only way you'd ever understand why is if you manually find the file and inspect its properties. It's true you could hide or disable the "Delete" item if the download target is read-only, but 1) that would mean popupshowing handler is blocked by async methods, since OS.File is being removed I believe, and 2) user may want to delete the file even if it is read-only, or they may not know the file is read-only. That would mean not giving them the affordance at all, which could be equally perceived as a bug.

(In reply to :Gijs (he/him) from comment #3)

I also wonder what happens if the attribute is set on the directory, does Windows inherit it into files saved there?

I just tested on Windows 10 and it seems Windows' read-only property for folders only applies to descendant files anyway. The read-only checkbox in file properties says "Only applies to files in folder" and when I tested it, it only changed the file. I basically made a path /1/2/3/ then downloaded a file to /1/2/3/file.txt and set 1 to read-only, then used the menuitem to delete file.txt. Which worked fine.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:mak, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 7

[Marco Bonardo [:mak]]

I agree with Gijs and Shane here.
On Windows read-only just means "don't modify contents of the file", but delete pretty much always ignored that setting, also in other apps.
This is unlikely to change on Windows, it would be disruptive to non-technical users. I must note Windows sort-of workaround this by allowing users to set this "confirm every file removal" checkbox in the trashcan Properties... but it applies to any kind of file.
While there's ways to "fix" this, they involve providing specialized UI for this case, that seems rare enough. This means the benefit/cost ratio is pending towards the latter. We are unlikely to spend time on this, unless Windows suddenly decides read only means something else.

The only solution that seems easier, would be to move file to the trashcan when it's read-only, instead of doing a straight removal. That would not require changes to the ui, but still allow the user to recover in case of a mistake.
Unfortunately, we don't seem to have code to do that, like an IOUtils.moveToTrash() helper or such.
We could accept community patches for that, but we're unlikely to work on it.

So, this is potentially a WONTFIX, but I think the trashcan path may not be too bad, if the community wants to try playing with it.