Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make FileList mutable. #3269

Open
Kaiido opened this issue Dec 4, 2017 · 52 comments
Open

Make FileList mutable. #3269

Kaiido opened this issue Dec 4, 2017 · 52 comments
Labels
addition/proposal New features or enhancements

Comments

@Kaiido
Copy link
Contributor

Kaiido commented Dec 4, 2017

Since recent changes about HTMLInputElement.prototype.files, it is possible to set this property to an other FileList object.

Currently the only ways to get an FileList object are from an HTMLInputElement and a DataTransfer object.
Previously, these objects could not be created in a mutable state from scripts, but since the implementation of the DataTransfer constructor, it is now possible to create a mutable DataTransfer object.

This means that a script can currently create arbitrary FileList, containing script generated Files objects, and, by extension, that a script can set arbitrary Files in HTMLInputElement.prototype.files.
See this fiddle in Blink which are currently the only ones to implement the DataTransfer constructor.

If this behavior is intended, it might be a good idea to implement a real API allowing this:

  • create a FileList.FileList constructor,
  • make the FileList mutable.

However, I am not sure about any security considerations there should be in such cases (e.g possibility to set a generated File with the same description as a File provided by the user).

Original discovery made by guest271314.

@annevk
Copy link
Member

annevk commented Dec 4, 2017

Since this primarily affects drag & drop and <input type=file> I think it's reasonable to track this idea here for a bit since another alternative would be to change <input type=file>.files so you can also set it to a sequence of Blob objects. However, it seems simpler to just embrace FileList as a thing.

cc @bsittler @mkruisselbrink @inexorabletash

@annevk annevk added the addition/proposal New features or enhancements label Dec 4, 2017
@annevk
Copy link
Member

annevk commented Dec 4, 2017

(I don't see any security implications by the way. You can already recreate most of the setup with FormData and XMLHttpRequest (you don't get to navigate an <iframe> to the response body though).)

@bsittler
Copy link

bsittler commented Dec 4, 2017

For the list of Blob objects how could the filenames be specified?

@inexorabletash
Copy link
Member

FormData has append(name, blob, filename) but IMHO I wouldn't try to repeat that pattern. That door hasn't been cracked open yet here, at least. Kudos to @Kaiido for raising this!

It seems that we do now have (1) a way to construct an empty FileList and (2) append a File to it, so this could be a thing:

function newFileList() {
  const dt = new DataTransfer();
  if (arguments[0] !== undefined) {
    for (const file of arguments[0]) dt.items.add(file);
  }
  const fl = dt.files;
  Object.defineProperty(fl, 'append', {value: file => { dt.items.add(file); }});
  return fl;
}

Note the append() bolt-on sketched here doesn't currently work in Blink — we don't currently honor [SameObject] or the spec prose "The files attribute must return a live FileList..", and return a new FileList each time.

Conceptually I'd be comfortable adding [Constructor(optional sequence<File>)] and append(File). We've had a FileList constructor as a feature request for a long time and if we're not dropping it from the platform then let's make it testable.

@domenic
Copy link
Member

domenic commented Dec 4, 2017

We could also add [LegacyArrayClass] and an indexed setter, to give it all the array methods. Then it would be push(), not append().

@annevk
Copy link
Member

annevk commented Dec 5, 2017

I was thinking if you add Blob you get a default filename, but not allowing it altogether is probably better indeed.

I thought we were trying to get rid of [LegacyArrayClass] still?

Also, is this a list or a set? Can you get the same File object in there twice? I guess maybe with drag & drop now it has this API form we didn't fully consider when adding, but not with <input type=file> currently.

If we settle list/set I think the most conservative v0 would be solely a constructor and leave mutation for a later version. That's how we approached FormData and other than the mistake mentioned above it worked out reasonably.

@annevk
Copy link
Member

annevk commented Dec 5, 2017

Oh, and let's copy @guest271314 since they apparently discovered this and are therefore probably interested.

@pwnall
Copy link
Contributor

pwnall commented Dec 5, 2017

FWIW, there was a way to get File instances from the File constructor into FileList before the DataTransfer constructor (at least in Chrome), but it required a drag-and-drop gesture -- see https://crbug.com/367334

I am strongly in favor of adding a straightforward way to build a FileList out of a sequence of Files, or adding a way to set an <input type="file">'s value to a sequence of Files.

I weakly prefer that FileList remains / becomes immutable, to avoid complexity in the conceptual model for <input type="file">. Specifically, if a FileList is the value of one or multiple inputs, when the FileList is mutated, the inputs have to repaint themselves.

@guest271314
Copy link
Contributor

guest271314 commented Dec 6, 2017

@annevk Is the dispatching of change event relevant? If yes, should the value of the trusted property of the change be set to true when the FileList is created by user action, and false when the event is dispatched programmatically by setting the .files property to an arbitrary FileList? Or are the concerns over change event moot due to the ability to set arbitrary File objects at a FormData object?

Should directory uploads also be considered? That is to also have the ability to create a Directory https://wicg.github.io/entries-api/#directory instance to populate <input type="file" allowdirs webkitdirectory/>? Or is the possibility of creating a Directory instance to populate and <input type="file"> .files a different topic?

What would be the practical difference between FileList being a list or a set? Would the distinction only be evident if we get to creating directory instances where multiple directories could contain copies of the same file?

@Kaiido
Copy link
Contributor Author

Kaiido commented Dec 6, 2017

@guest271314 the change event on HTMLInputElement is triggered by end-user's action. Just like if you change any value of any type of input, this event should not fire when the change in value has been triggered by script.

As to your concern about Set vs List, even with the webkitDirectory attribute, you can not have twice the same File from usual input interaction. Your two copies example are still two differents File objects, at least with their own webkitRelativePath property.

@guest271314
Copy link
Contributor

@Kaiido Note, change event is currently dispatched without user action when .files property is set at Chromium browser.

@Kaiido
Copy link
Contributor Author

Kaiido commented Dec 6, 2017

@guest271314 and they don't when you clear this FileList through input.value = null.

https://html.spec.whatwg.org/multipage/indices.html#event-change

Fired at controls when the user commits a value change

You might want to open an issue to bugs.chromium and bugs.webkit since it seems this was introduced prior to Blink split.

@guest271314
Copy link
Contributor

guest271314 commented Dec 6, 2017

@annevk
Copy link
Member

annevk commented Dec 6, 2017

@guest271314 I meant to get back to you on that, sorry for not having done so yet, but it seems you resolved it yourself together with @Kaiido by filing those bugs. Hopefully they can be fixed but otherwise we'll need a new issue to sort that out. Thank you for taking care of that!

As for directories, they're separate. We will need to integrate that document into the HTML Standard in due course. Any new features can be considered after that or potentially in parallel by filing issues against that document.

As for set vs list. Consider const file = new File([], "hi"); const fileList = new FileList([file, file]);. Hope that helps.

@guest271314
Copy link
Contributor

@annevk The only security issue that could fathom is a malicious script which switched an uploaded file initiated by user action to attribute the file to a user where the remainder of the form was attributed to the user - based on the initial user action of uploading the file that the user selected from a file system. Though for practical purposes that would not necessarily matter because the trusted boolean property is not a validation issue with an uploaded file and the above scenario is possible using FormData and .submit(), see https://stackoverflow.com/a/29873845; though that is what was considering when mentioning change event.

As to whether or not change event should be fired when .files is set programmatically the inquiry was into how the event observes the .files property? That is, is the event not dispatched at Firefox because the original FileList is no longer being observed because the original FileList no longer exists; technically the original FileList is not changed, but rather replaced. For example, if a Proxy is observing an object that proxy would not dispatch set trap on the original object as that object is replaced by the new FileList object. Or, the set trap is dispatched (at Chromium) because the .files property itself is being observed, and not the FileList value of the .files property?

@annevk
Copy link
Member

annevk commented Dec 6, 2017

A developer cannot observe it. They need to do their own bookkeeping basically. Chrome dispatches the event from the files setter presumably, but that's contrary to how <input> element APIs generally behave, so trying to get it removed is good.

@annevk
Copy link
Member

annevk commented Dec 7, 2017

I weakly prefer that FileList remains / becomes immutable

This seems fine to me by the way, but we should make sure to note this reason in File API as otherwise we might forget it later on.

@annevk
Copy link
Member

annevk commented Dec 7, 2017

Remaining open questions:

  • Can FileList contain multiple references to the same File? I suggest no.
  • Does FileList take an array or unlimited number of arguments (is that called vararg? I always forget)? An array would be consistent with the File constructor.

@inexorabletash
Copy link
Member

My general bias would be that we avoid a future where a suite of tests for FileList needs to use a back door (e.g. DataTransferItemList) to test it. It seems a bit weird if you can get a mutable FileList or a FileList with multiple refs to the same file through some means, but we can't write isolated tests for that. But that runs counter to trying to make an ergonomic platform.

  • Multiple references to the same file: DataTransferItemList.add() doesn't prevent the same file from being added twice, so a FileList containing the same item twice is already a thing. Unless we change that, restricting FileList constructor doesn't seem helpful. Easiest to restrict now and open up later, though.
  • Array or varargs: I'd weakly lean towards array for consistency (although DOM has grown some varargs) and because we can toss options at the end (but no idea what those would be), but really no strong prefs here. (I also still haven't gotten used to the spread operator)

As an aside: we should be explicit (and test) that identity of the files is retained, i.e. new FileList([f])[0] === f. And for the record, currently dt = new DataTransfer; dt.items.add(f); dt.items.add(f); dt.files[0] === f; dt.files[0] === dt.files[1];

@annevk
Copy link
Member

annevk commented Dec 7, 2017

I'm not sure I understand your first paragraph. I don't have a problem with supporting duplicate files here. Let's just go with that if that's what we already do.

@inexorabletash
Copy link
Member

I'm not sure I understand your first paragraph.

Sorry, mostly just arguing with myself and not getting anywhere.

@guest271314
Copy link
Contributor

@annevk

  • Can FileList contain multiple references to the same File? I suggest no.

Multiple references to the same file should be possible.

  • Does FileList take an array or unlimited number of arguments (is that called vararg? I always forget)? An array would be consistent with the File constructor.

An array should should meet requirement. There should not be a limit on the number of File objects within array passed to FileList constructor.

One additional implementation feature could be the ability to pass a single File object to the constructor that is not an array, which would be set as File object at index 0 of resulting FileList new FileList(new File([], "file.txt"))

@domenic
Copy link
Member

domenic commented Dec 8, 2017

We should follow the other collections on the platform, such as Set, and accept an iterable of Files. I don't think anything else is reasonable; in particular, there is no precedent on the web platform for some of the more exotic suggestions in recent comments.

@domenic
Copy link
Member

domenic commented Dec 8, 2017

Let me be more specific; this is a collection class, and our precedents must be other collection classes. (Blob is not a collection class.)

@bsittler
Copy link

bsittler commented Dec 8, 2017

With behavior like that I'd expect it to be concat on a mutable FileList, not the constructor. Array.prototype.concat also behaves this way.

@bsittler
Copy link

bsittler commented Dec 8, 2017

Actually I was wrong, @Kaiido - that returns a new array rather than modifying existing ones - so it could work even with an immutable container.

@Kaiido
Copy link
Contributor Author

Kaiido commented Dec 8, 2017

@bsitler Good for me.
I remove my previous exotic idea, and then do the humble request to have a new

FileList.prototype.concat method which would work like the one of Array.

@bsittler
Copy link

bsittler commented Dec 8, 2017

Might it be worth polyfilling ideas for the new API first on top of the extant (at least in Blink) API to get a feeling for the most ergonomic and platform-native approach?

For instance, here's a polyfilled shallow-only FileList pseudoconstructor:

(() => {
  try {
    return void(new FileList(new File([''], '')));
  } catch(e) {
  }
  const _real_FileList = FileList;
  FileList = function(...items) {
    const dataTransfer = new DataTransfer;
    for (let item of items) dataTransfer.items.add(item);
    return dataTransfer.files;
  };
  FileList.prototype = _real_FileList.prototype;
})();

edit: and with that polyfill, you can do e.g.

new FileList(
  new File(['a'],'a.txt'),
  ...new FileList(new File(['b'], 'b.txt'), new File(['c'], 'c.txt')),
  new File(['d'], 'd.txt'));

to flatten one filelist into another. Is it too cumbersome?

@annevk
Copy link
Member

annevk commented Dec 8, 2017

@domenic so [Constructor(sequence<File>), Exposed=Window] interface FileList { };?

@guest271314
Copy link
Contributor

@bsittler If we include items = [].concat(...items) we can pass an array to the constructor

@domenic
Copy link
Member

domenic commented Dec 8, 2017

@annevk yep.

@annevk
Copy link
Member

annevk commented Dec 8, 2017

@pwnall if FileList is immutable, how does DataTransfer work? It's files attribute is annotated as [SameObject]. Is that a lie? Do we actually replace it with a new FileList object each time? (Or do we want to start doing it to allow FileList to be immutable?)

@inexorabletash
Copy link
Member

FYI, I checked in the latest browsers. files is treated as [SameObject] in Edge/Firefox/Safari. Only Chrome vends a new object each time the attribute is accessed. (https://jsfiddle.net/4oqmkxfj/)

@bsittler
Copy link

bsittler commented Dec 9, 2017

@inexorabletash outside of Chrome is it [SameObject] across a change in the selected file set (that is, is it effectively mutable?)

@guest271314
Copy link
Contributor

@annevk Have no experience with C++, am not certain if this is the check for [SameObject] https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8DataTransferItemList.cpp?rcl=082678956a8b505c0f3f9fd563ee2b7b37409dee&l=61 at blink?

@guest271314
Copy link
Contributor

@pwnall
Copy link
Contributor

pwnall commented Dec 10, 2017

@annevk It'd be interesting to know how other browsers deal with the issue of repainting the input when a FileList vended by DataTransfer is updated. I'll write some code and get results on Monday, unless someone beats me to it.

Given that Chrome doesn't follow [SameObject] on DataTransfer.files, I think we can hope that wouldn't be breaking the Web by removing the attribute.

If this doesn't work out, the other lever I see is having the HTMLInput.files setter do a copy of the FileList that is given to it. This seems slightly more wasteful, as DataTransfer can get away with creating a single FileLists when the files property is read after all the files are added (which I think is normal usage?). The approach also seems a bit worse for the future than having an immutable FileList, as any new API that takes in FileList will have to worry about its mutability. I think it's still better than having the repainting issue, though.

/cc @inexorabletash @bsittler @guest271314

@Kaiido
Copy link
Contributor Author

Kaiido commented Dec 11, 2017

@pwnall Firefox seems to follow [SameObject], and to set DataTransfer.files as mutable.[1]

However, their young implementation of settable input.files property doesn't seem to handle repaints very carefully yet.
I did open an related issue about a month ago, but it has been set as low priority.

@guest271314
Copy link
Contributor

guest271314 commented Dec 11, 2017

FWIW, the code below returns same result at Chromium 62, Firefox 57 (have not tried at Safari, Edge) save for change event being dispatched at Chromium, and does not appear to affect FileList of <input type="file"> element at change event following selection of file by user action

// FileList.js
class FileList {
  constructor(...items) { 
    // flatten rest parameter
    items = [].concat(...items);
    // check if every element of array is an instance of `File`
    if (items.length && !items.every(file => file instanceof File)) {
      throw new TypeError("expected argument to FileList is File or array of File objects");
    }
    // use `ClipboardEvent("").clipboardData` for Firefox, which returns `null` at Chromium
    // we just need the `DataTransfer` instance referenced by `.clipboardData`
    const dt = new ClipboardEvent("").clipboardData || new DataTransfer();
    // add `File` objects to `DataTransfer` `.items`
    for (let file of items) {
      dt.items.add(file)
    }
    return dt.files;
  }
}
<!DOCTYPE html>
<html>

<head>
</head>

<body>
  <input id="file" type="file" name="files[]" multiple>
  <script src="FileList.js">
  </script>
  <script>
    file.onchange = e => console.log(e.target.files);
    // or new FileList(new File(["c"], "c.txt"))
    let files = new FileList([new File(["a"], "a.txt"), new File(["b"], "b.txt")]);

    file.files = files;
  </script>
</body>

</html>

plnkr

@guest271314
Copy link
Contributor

guest271314 commented Dec 11, 2017

@Kaiido Do you suggest that we compose the appropriate FileList from scratch, taking into account the various existing API's that use the current implementation of FileList?

Does [SameObject] affect what we are attempting to achieve, that is setting arbitrary File objects at <input type="file"> element? What is the exact requirement?

@pwnall
Copy link
Contributor

pwnall commented Dec 11, 2017

@guest271314 Thank you for the research! I think that Firefox's bug confirms my hunch that the input repaint issue is non-obvious and that we should try to find a solution that doesn't involve having a way to change the input's value indirectly (by mutating a FileList).

@guest271314
Copy link
Contributor

@pwnall The code at previous comment was one of the approaches tried as a proof of concept for programmatically setting arbitrary File objects at .files property of <input type="file"> element.

The previous attempt did set arbitrary File objects at FileList, though did not affect the .length of the FileList, also the File objects were not correctly reflected when the parent <form> was passed to FormData

const input = document.createElement("input");

const form = document.createElement("form");

const data = [
  new File(["a"], "a.txt")
, new File(["b"], "b.txt")
];

input.type = "file";

input.name = "files[]";

input.multiple = true;
// set `File` objects at `FileList`
input.files[Symbol.iterator] = function*() {
   for (const file of data) {
     yield file
   };
};

form.appendChild(input);

const fd = new FormData(form);

for (const file of input.files) {
  console.log(file); // `File` objects set at `data`
}

for (const [key, prop] of fd) {
  // `"files"`, single `File` object having `lastModified` property
  // set to a time greater than last `File` object within `data`
  // at Chromium 61, only `"files"` at Firefox 57
  console.log(key, prop); 
}

console.log(input.files.length); // 0

If we could set .length of FileList to result of the set iterable returned from the function call and set those file objects to be recognized by FormData as valid key, value pairs, we could avoid using DataTransfer.items .add() method to create a new FileList object.

Did not consider concepts "mutability" or "repaint" when attempting code to meet requirement.

Given that the initial requirement is possible, from your perspective, what is the exact requirement now relevant to defining how the procedure of setting arbitrary File objects at an <input type="file"> element should ideally be implemented?

@pwnall
Copy link
Contributor

pwnall commented Dec 11, 2017

@guest271314 Sorry for being unclear earlier! I didn't mean to imply there was anything wrong with your code! I am grateful that you looked into today's implementations!

Given that (IIRC) we can't remove FileList from the Web platform, I think that the best outcome would be:

// setup
const file1 = new File(...);
const file2 = new File(...);

const list = new FileList([file1, file2]);
fileInput.files = list;
fileInput.files === list;  // true

const dt = new DataTransfer();
dt.items.add(file1);
const dt_list1 = dt.files;
dt.items.add(file2);
const dt_list2 = dt.files;
dt_list1 === dt.list2;  // false

Summary: FileList is immutable, HTMLInputElement.files acts like a vanilla JS property.

A worse situation (IMO) that requires fewer browser changes:

// setup
const file1 = new File(...);
const file2 = new File(...);

const list = new FileList();
list.push(file1);
list.push(file2);
fileInput.files = list;
fileInput.files.length === 2;  // true
fileInput.files.item(0) === file1;  // true
fileInput.files.item(1) === file2;  // true
fileInput.files === list;  // false

const dt = new DataTransfer();
dt.items.add(file1);
const dt_list1 = dt.files;
dt.items.add(file2);
const dt_list2 = dt.files;
dt_list1 === dt.list2;  // true

Summary: FileList is mutable, and the HTMLInputElement.files setter makes a copy of its argument.

Does this answer your question?

@pwnall
Copy link
Contributor

pwnall commented Dec 11, 2017

@annevk The question above also made me figure out one more wrinkle in the mutable FileList alternatives -- what happens to a FileList vended by a DataTransfer instance after the DataTransfer isn't readwrite anymore? I think this happens during the drop event.

@guest271314
Copy link
Contributor

@pwnall While we are here why cannot we rewrite the entire code including all dependencies to result in consistency between the API's as what what exactly FileList is and how that object is implemented? Not certain gather what expected result is if not to adjust the definitions, descriptions and code accordingly? Is the requirement the first block of code at your previous comment?

@annevk
Copy link
Member

annevk commented Dec 12, 2017

Thanks all, sounds like it should be immutable and we'll need to update the drag & drop API to replace the to be returned instance upon mutation instead (and remove [SameObject]). That seems like the simplest path forward and also would not get in the way of any particular use case.

@annevk
Copy link
Member

annevk commented Oct 13, 2020

I looked into this briefly and at seems that as of yet, https://searchfox.org/mozilla-central/source/dom/webidl/DataTransfer.webidl and https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/clipboard/data_transfer.idl have files of DataTransfer as readonly. That doesn't mean someone couldn't pursue this change, but it would likely be a bit of work.

@Kaiido
Copy link
Contributor Author

Kaiido commented Oct 13, 2020

Not sure to understand this last comment, @annevk could you clarify a bit how this is a blocker?

From my understanding, browsers just follow the specs here, the DataTransfer.files member is a getter that returns whatever files are in the associated drag data store at this time.
The exposed technique is using the fact that new DataTransfer() constructor creates a drag data store in read/write mode, and thus on which we can add new data through the DataTransfer.items.add() method.
So the .files getter should return the (same) FileList, with the updated list of Files in the drag data store. I.E, this FileList is mutable.

I fail to see how the fact that this DataTransfer.files getter is readonly complicates anything about making a FileList constructor. We don't modify this property directly.

@annevk
Copy link
Member

annevk commented Oct 13, 2020

I thought the latest state of affairs in this was issue was #3269 (comment). HTML is not the place to introduce a FileList constructor. All I meant was that this is more than editorial work. It needs implementer support, tests, and the editorial work.

@Kaiido
Copy link
Contributor Author

Kaiido commented Oct 14, 2020

Ok, from #3269 (comment) I didn't get as far as "HTML is not the place to introduce a FileList constructor", but it's a fair point.
[edit]: I now see there is w3c/FileAPI#24 which is apparently blocked by this very issue?

Regarding what concerns HTML, I believe you can make it stop mutate the FileList by removing the [SameObject] for files in DataTransfer dfn, and maybe by removing the "live" in "live FileList" from the .files getter steps.
I think DataTransfer.files can stay readonly for this.

As demonstrated by #3269 (comment) this would

  • match Blink's current behavior,
  • require a change in Gecko and Webkit (the latter still doesn't expose the DataTransfer constuctor though).

The fact Chromium hasn't a bug against this interop issue makes me believe it wouldn't be a breaking change, but it's true WPT and implementers support are required.

@andreubotella
Copy link
Member

andreubotella commented Oct 8, 2022

Could this be achieved instead by replacing FileList with array/sequence-like types? DataTransfer.files would be an ObservableArray<File> where the "set/delete an indexed value" algorithms either do nothing or throw; and HTMLInputElement.files would be a FrozenArray<File>?, since it's immutable but the attribute can be reset by authors or by the UA when the selection changes. Furthermore, since converting a JS array into a FrozenArray results in a copy, this solves the issue of the set of selected files on an <input type=file> changing unknowingly because it's linked to a drag data store.

Do we expect this to be impossible due to compat concerns?

@domenic
Copy link
Member

domenic commented Oct 8, 2022

The likely compat issue is the item() method. We'd need to create some kind of ObservableArray-with-item() to reduce the compat worries.

(Even then, there could be compat problems of various sorts. But they'd be less likely...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements
Development

No branches or pull requests

9 participants