WICG / file-system-access

Expose the file system on the user’s device, so Web apps can interoperate with the user’s native applications.
https://wicg.github.io/file-system-access/
Other
653 stars 65 forks source link

Step 7.7 of `showSaveFilePicker()` spec #360

Open tomayac opened 2 years ago

tomayac commented 2 years ago

(Filing on behalf of @kuckir from @photopea.)

Obtaining a handle for an existing file erases said files contents, even if no actual write operation is being performed. You can test the behavior in this demo that doesn't do anything with the handle after obtaining it.

This is actually working as specified, specifically, step 7:

  1. Set entry’s binary data to an empty byte sequence.

Since creating the contents to be saved to a file in an app like Photopea can take a while, they need to asynchronously obtain a file handle first, and only then write back the contents to the file once it's ready, as the reverse process would consume the user gesture and not show the dialog anymore. Should, however, anything go wrong during the data preparation phase, the user has lost the data in the file.

AshleyScirra commented 1 year ago

I just ran in to a similar thing around logging. Suppose the user wants to select a text file to add log entries to. The natural thing to do would be to use a save picker, and choose either a new file, or an existing file to append to. However if you choose an existing file, this step of the spec says to clear the file, so it always clears the existing log file.

This means you have to have two approaches:

  1. If you want to save to a new log file, use the save file picker
  2. If you want to append to an existing log file, use the open file picker

This means having two buttons in the user interface instead of one and making the user decide ahead of time whether they want to choose a new or existing log file (which they might not be able to decide until they've seen the available files in the picker). Also the user could easily make a mistake if they accidentally use the save file picker instead and choose an existing file, and then their existing log is wiped.

I think that step should be removed - the save file picker should return a file handle with the existing content of the file, and whether or not it is fully overwritten depends on the keepExistingData parameter of createWritable.

AshleyScirra commented 1 year ago

Actually I think this is a really nasty gotcha with the API. In general if you use a save file picker, any error preparing or writing the data is a data loss scenario for the user. For example if the app crashes preparing the save data the user's chosen file is wiped. That's pretty bad! It's much better to fail with the file content intact.

AshleyScirra commented 1 year ago

I've noticed that we get occasional support requests along the lines of "my computer crashed while saving and now my project won't open", and the customer sends their project file which just entirely consists of zero bytes and so is completely unrecoverable. One of these just came in today. I've realized it's possible it's this spec issue that is behind all these. But if we change it to save first and show the picker later, we lose the user gesture and so we have to make the user click again, which creates an annoying usability problem.

IMO this is a severe data loss issue in the spec and should be fixed ASAP! Is there any news on when this might be changed?

I think we are going to have to go with an annoying UX merely to avoid the possibility of a data loss scenario!

AshleyScirra commented 1 year ago

Actually, I guess the question is: does step 7.7's wording mean "overwrite the file with zero bytes", or essentially "create a new and empty file with zero size"? I suppose if it's the latter then this may in fact have been caused by some other issue.

photopea commented 1 year ago

I totally agree. The browser should either keep the original file, or save a proper new file. It can not simply delete the old file (replace it with zeros), if the website decides not to finish the write process.

Also, I think the mechanism of gestures should have not been created at all. There is still a dialog asking the user to confirm the action, why cant you let a webap call it without a user gesture?

It is like wanting the web apps and native apps to compete in a race, but you tie the webapps legs together, and give them a blindfold.

AshleyScirra commented 1 year ago

I don't think the user gesture requirement is unreasonable, nor is it directly related to the root cause issue here, although it does complicate workarounds. The key problem is the spec should not have a step that says "wipe the file the user chose", especially when web apps already decide whether or not to clear the existing file contents when writing with the keepExistingData option.

photopea commented 1 year ago

@tomayac could you simply implement it in Chrome correctly, i.e. not letting a website turn the whole file into zeros?

We have been waiting for over a year for the specification to get fixed, but they are not fixing it, so just implement it correctly yourself.

tomayac commented 1 year ago

@a-sully, Construct 3 (@AshleyScirra) and Photopea (@photopea) are experiencing data loss issues, presumably due to this very Issue. I've also opened a Chromium bug, so we have both on file and can act on them.

a-sully commented 1 year ago

I just ran in to a similar thing around logging. Suppose the user wants to select a text file to add log entries to. The natural thing to do would be to use a save picker, and choose either a new file, or an existing file to append to. However if you choose an existing file, this step of the spec says to clear the file, so it always clears the existing log file.

This means you have to have two approaches:

  1. If you want to save to a new log file, use the save file picker
  2. If you want to append to an existing log file, use the open file picker

This means having two buttons in the user interface instead of one and making the user decide ahead of time whether they want to choose a new or existing log file (which they might not be able to decide until they've seen the available files in the picker). Also the user could easily make a mistake if they accidentally use the save file picker instead and choose an existing file, and then their existing log is wiped.

Please correct me if I'm wrong, but is this not the case for native apps today? For example, Mac has NSOpenPanel for selecting files that exist (i.e. showOpenFilePicker() or <input type=file>) and NSSavePanel for saving new files (i.e. showSaveFilePicker() or a download). AFAIK most platforms don't have the concept of a "select new or existing file" picker, and selecting an existing file from a "save" picker generally shows an "Are you sure you want to replace this file?" dialog.

A key difference is that, for native apps, file pickers are just an indication of which file the user wants the app to access. All that's returned from the picker is a path, and the app can access that file at will.

Meanwhile, this API uses pickers to gate access to files and directories. But showOpenFilePicker() only gives read access to the selected file. If you want write access to the file, you can requestPermission() on the returned handle. This is WAI. showSaveFilePicker() is not intended to be used as a shortcut to get "readwrite" access to a file with fewer prompts.

I think that step should be removed - the save file picker should return a file handle with the existing content of the file, and whether or not it is fully overwritten depends on the keepExistingData parameter of createWritable.

While I understand that this is the common pattern, it's not guaranteed that createWritable() is called after showSaveFilePicker(). We could consider expanding the API such that showSaveFilePicker() could optionally behave more as a "download" (e.g. by passing in the data to be downloaded. See #29), but that's not what's being proposed here

Actually I think this is a really nasty gotcha with the API. In general if you use a save file picker, any error preparing or writing the data is a data loss scenario for the user. For example if the app crashes preparing the save data the user's chosen file is wiped. That's pretty bad! It's much better to fail with the file content intact.

The same scenario exists today with a standard download, no? Either way, this seems to fall under the umbrella of improving the "download" use case more broadly

@tomayac could you simply implement it in Chrome correctly, i.e. not letting a website turn the whole file into zeros?

Frankly, this is a far more significant change than it seems on the surface. Chromium leverages each operating system's native file picker APIs (such as NSOpenPanel) to show file and directory pickers, which helps set user expectations for the actions they're taking (e.g. that a "save" file picker will overwrite an existing file). Implementing "select new or existing file" functionality would mean setting new user expectations and (arguably) requiring browsers to build their own custom file picker, which is not trivial

tomayac commented 1 year ago

Implementing "select new or existing file" functionality would mean setting new user expectations and (arguably) requiring browsers to build their own custom file picker, which is not trivial

+1, let's definitely not go there. Implementing custom dialogs contributes to the uncanny valley feeling of "the app not being fully at home on the platform".

AshleyScirra commented 1 year ago

A key difference is that, for native apps, file pickers are just an indication of which file the user wants the app to access. All that's returned from the picker is a path, and the app can access that file at will.

For me, this is the key point: a picker is just to choose a file. The act of showing a picker should not itself modify the file.

I don't think anyone is proposing another type of file picker here, only to remove step 7.7 of the existing spec, i.e. don't wipe the file that was chosen.

Data loss bugs are one of the most severe class of bugs and can be catastrophic for users. I think this alone is the main argument to change this. Regardless of what other platforms do or how the permissions model works, the design of a web API should not make it so easy to cause potentially catastrophic data loss for the user.