Make the files attribute of HTMLInputElement writable

ASSIGNED
Assigned to

Status

()

Core
DOM: Core & HTML
ASSIGNED
5 years ago
2 years ago

People

(Reporter: thakis, Assigned: bz)

Tracking

(Depends on: 1 bug, {dev-doc-needed})

Trunk
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 626246 [details]
Patch

The files attribute of the input element is currently marked readonly
[1], to protect from `myInput.files = "/etc/passwd"; myForm.submit()`.
Since its type is now FileList and not string, that's no longer
necessary.

Making the attribute writable would allow setting the files property
of an input element to dataTransfer.files from a drop handler. For
example, I would like to use this to create a larger drop-target for a
file input. Here's one request for this functionality:
http://stackoverflow.com/questions/8006715/drag-drop-files-into-standard-html-file-input

1: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html#the-input-element



---

whatwg thread: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-May/036140.html
webkit change: https://bugs.webkit.org/show_bug.cgi?id=87154


---

I'm happy to add a test too, but https://developer.mozilla.org/en/Mozilla_automated_testing listed so many options that I couldn't decide on one. Please let me know what the best test type is for this change, if any.
(Reporter)

Comment 1

5 years ago
Comment on attachment 626246 [details]
Patch

`hg log` suggests that sicking might be a good reviewer for this.
Attachment #626246 - Flags: review?(jonas)
Component: General → DOM
Product: Firefox → Core
QA Contact: general → general

Comment 2

5 years ago
Comment on attachment 626246 [details]
Patch

You should update the uuid of nsIDOMHTMLInputElement and write some tests.
(Reporter)

Comment 3

5 years ago
Created attachment 626263 [details] [diff] [review]
patch

Thanks for the comment! I suppose uuids always need to be updated when an interface changes. I just created an arbitrary new uuid, is that ok?

Can you point me to the directory of the existing input element tests? As mentioned above, I couldn't find them.
Attachment #626246 - Attachment is obsolete: true
Attachment #626246 - Flags: review?(jonas)
The relevant test location would probably be http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/forms/ . There are other tests there you can use for inspiration. Those are "plain" Mochitests: https://developer.mozilla.org/en/Mozilla_automated_testing#Mochitest_%28M%29

See https://developer.mozilla.org/en/Mochitest#Running_select_tests for how to run them.
Assignee: nobody → thakis
Component: DOM → DOM: Core & HTML
OS: Mac OS X → All
Hardware: x86 → All
(Reporter)

Comment 5

5 years ago
Created attachment 628547 [details] [diff] [review]
patch

Now with test. I haven't run the test yet (still waiting for my build), but it's probably good enough that you can tell me if this is basically the right way to go.
Attachment #626263 - Attachment is obsolete: true
Comment on attachment 628547 [details] [diff] [review]
patch

Review of attachment 628547 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks good but the tests need a bit of love.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1310,3 @@
>  }
>  
>  nsresult

NS_IMETHODIMP

::: content/html/content/test/forms/test_input_files_writable.html
@@ +18,5 @@
> +var input2 = document.forms[0].input2;
> +
> +var myFile = ["/foo"];
> +netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> +input1.mozSetFileNameArray(myFile, myFile.length);

I thought MozSetFileNameArray() was ignoring files not part of the file system. Is /foo in your machine? Or is that some kind of magic file we are using?

The method I use is what is in content/html/content/test/forms/test_required_attribute.html (checkInputRequiredValidityForFile method).

@@ +21,5 @@
> +netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> +input1.mozSetFileNameArray(myFile, myFile.length);
> +netscape.security.PrivilegeManager.disablePrivilege("UniversalXPConnect");
> +
> +input2.files = input1.files;

Could you test that this is not throwing?

@@ +23,5 @@
> +netscape.security.PrivilegeManager.disablePrivilege("UniversalXPConnect");
> +
> +input2.files = input1.files;
> +
> +ok(input2.files.length === 1, "files attribute should be writable");

is(input2.files.length, 1, "...");

Could you also check that input2.files.length is equal to 0 before the set.
Also, could you check that .value was returning "" and is now returning the file name?
(Reporter)

Comment 7

5 years ago
Created attachment 628891 [details] [diff] [review]
patch
Attachment #628547 - Attachment is obsolete: true
(Reporter)

Comment 8

5 years ago
> ::: content/html/content/src/nsHTMLInputElement.cpp
> @@ +1310,3 @@
> >  }
> >  
> >  nsresult
> 
> NS_IMETHODIMP

Done.

> 
> ::: content/html/content/test/forms/test_input_files_writable.html
> @@ +18,5 @@
> > +var input2 = document.forms[0].input2;
> > +
> > +var myFile = ["/foo"];
> > +netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> > +input1.mozSetFileNameArray(myFile, myFile.length);
> 
> I thought MozSetFileNameArray() was ignoring files not part of the file
> system. Is /foo in your machine? Or is that some kind of magic file we are
> using?

I found this in some other test. I can't find where I found it though. I don't have /foo on my machine, yet still this seems to work.

> @@ +21,5 @@
> > +netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> > +input1.mozSetFileNameArray(myFile, myFile.length);
> > +netscape.security.PrivilegeManager.disablePrivilege("UniversalXPConnect");
> > +
> > +input2.files = input1.files;
> 
> Could you test that this is not throwing?

I verified that mochitest implicitly complains about every thrown exception, so that's already done.

> @@ +23,5 @@
> > +netscape.security.PrivilegeManager.disablePrivilege("UniversalXPConnect");
> > +
> > +input2.files = input1.files;
> > +
> > +ok(input2.files.length === 1, "files attribute should be writable");
> 
> is(input2.files.length, 1, "...");
> 
> Could you also check that input2.files.length is equal to 0 before the set.
> Also, could you check that .value was returning "" and is now returning the
> file name?

Done.
(Reporter)

Comment 9

5 years ago
Created attachment 628895 [details] [diff] [review]
patch
Attachment #628891 - Attachment is obsolete: true
Attachment #628895 - Flags: review?(mounir)
(Reporter)

Comment 10

5 years ago
(I verified that the test fails without the code change but passes with it.)
Comment on attachment 628895 [details] [diff] [review]
patch

Review of attachment 628895 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the requested changes.

Though, this needs a super-review from Jonas.

::: content/html/content/test/forms/test_input_files_writable.html
@@ +34,5 @@
> +netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> +input1.mozSetFileNameArray(myFile, myFile.length);
> +
> +input2.files = input1.files;
> +is(input2.files.length, 1, "files attribute should be writable");

I would really prefer something like that:
caught = false;
try {
  input2.files = input1.files;
} catch(e) {
  caught = true;
}
is(caught, false, "files attribute should be writable");
is(input2.files.length, 1, "setting the files attribute should have updated it");
Attachment #628895 - Flags: superreview?(jonas)
Attachment #628895 - Flags: review?(mounir)
Attachment #628895 - Flags: review+
(Reporter)

Comment 12

5 years ago
> ::: content/html/content/test/forms/test_input_files_writable.html
> @@ +34,5 @@
> > +netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> > +input1.mozSetFileNameArray(myFile, myFile.length);
> > +
> > +input2.files = input1.files;
> > +is(input2.files.length, 1, "files attribute should be writable");
> 
> I would really prefer something like that:
> caught = false;
> try {
>   input2.files = input1.files;
> } catch(e) {
>   caught = true;
> }
> is(caught, false, "files attribute should be writable");
> is(input2.files.length, 1, "setting the files attribute should have updated
> it");

What's the advantage of doing that? If the assignment throws, mochitest will fail the test anway, and it's 6 lines less as is.
(In reply to thakis from comment #12)
> > ::: content/html/content/test/forms/test_input_files_writable.html
> > @@ +34,5 @@
> > > +netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> > > +input1.mozSetFileNameArray(myFile, myFile.length);
> > > +
> > > +input2.files = input1.files;
> > > +is(input2.files.length, 1, "files attribute should be writable");
> > 
> > I would really prefer something like that:
> > caught = false;
> > try {
> >   input2.files = input1.files;
> > } catch(e) {
> >   caught = true;
> > }
> > is(caught, false, "files attribute should be writable");
> > is(input2.files.length, 1, "setting the files attribute should have updated
> > it");
> 
> What's the advantage of doing that? If the assignment throws, mochitest will
> fail the test anway, and it's 6 lines less as is.

1. This is something that should fail without the patch and doesn't fail with the patch.
2. Explicit check is always better than implicit check.
3. If something goes wrong, in one blink a developer would be able to understand what is happening.
(Reporter)

Comment 14

5 years ago
Created attachment 629233 [details] [diff] [review]
patch

With the requested test change.
Attachment #628895 - Attachment is obsolete: true
Attachment #628895 - Flags: superreview?(jonas)
Attachment #629233 - Flags: superreview?(jonas)
Attachment #629233 - Flags: review?(mounir)
We need to think about the security implications of this and how it interacts with session restore before landing.
Comment on attachment 629233 [details] [diff] [review]
patch

Review of attachment 629233 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. The patch looks fine. Thank you! :)

I will let Jonas taking the decision regarding whether we want that change to happen.
We might also need a security review if the change is pushed to m-c.
Attachment #629233 - Flags: review?(mounir) → review+
I guess we can mark this bug as confirmed for the moment, keeping the option of marking it WONTFIX if we don't want that feature.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
I think we need to integrate with session-storage *where we can*. I.e. I don't think we should take arbitrary blobs and store them in session storage. However if someone takes a file-list, removes a couple of files, and then re-inserts it, then that should work with session-storage.

I don't see how the patch as-is would work given that we have no API for creating FileLists. We should make it possible to set any array containing Files as the value for .files.

We need to make sure to update the rendered text when .files is set. I'm not sure if the current patch does that or not.
(Reporter)

Comment 19

5 years ago
My use case is to install an html5 drop handler, which gets a file list. So there's no need to create FileLists.

Supporting an array of Files can be done in a follow-up; I'm not interested in working on that part.

The current patch updates the rendered text.

Session-storage integration should work just as well as it does for files added through the file picker dialog.
I'd rather not land a half-baked patch, that's generally not how we do things in Gecko. If we want to do this, we should do it right.
(Reporter)

Comment 21

5 years ago
I don't see how this is half-baked. It's functional, tested, does what it's supposed to do, and is better than what's currently in. It's forward-compatible with supporting arrays of Files if someone wants to implement that.
For one it means that pages can't do feature detection by assigning into a .files property. Instead you have to wait for a drop event before figuring out the platform capabilities.
(Reporter)

Comment 23

5 years ago
True, but is that worse than not being able to do this at all?

Comment 24

5 years ago
What thakis proposes is the way it works in Chrome, who doesn't support creating FileList either.
And the use case is huge : there is no other way to use drag n drop in a form without AJAX (which means server-side complexity, temp file...)
Blocks: 834652
Sorry, wrong bug...
No longer blocks: 834652
Comment on attachment 629233 [details] [diff] [review]
patch

Review of attachment 629233 [details] [diff] [review]:
-----------------------------------------------------------------

I'll let mounir make a decision here. I still think that we should implement the ability to do

inputElement.files = [blob1, blob2];

That should be pretty easy to implement once bug 825196 is fixed so that we can use webidl. Using WebIDL would also mean that passing a FileList would JustWork(tm) if I understand things correctly.

Maybe something Andrea is interested in taking on since he's been doing a lot of work on HTMLInputElement lately.
Attachment #629233 - Flags: superreview?(jonas) → feedback?(mounir)
Comment on attachment 629233 [details] [diff] [review]
patch

Marking sr- to make clear the patch is fine but we want to hold the landing for the moment.
Attachment #629233 - Flags: feedback?(mounir) → superreview-
So Jonas, if I understand correctly, a way to fix that is to add a Constructor to FileList that takes an array of File and make input.fileList r/w. Is that correct?

I agree that we could probably make this happen after bug 825196 anyway.
Depends on: 825196
Flags: needinfo?(jonas)
No, that wouldn't be enough to let the author do:

myInputElement.files = [blob1, blob2];
Flags: needinfo?(jonas)
I don't actually know what types we should use here, and what operations we should permit. I think we do want to allow the following:

element.files = otherElement.files;
element.files = [blob1, blob2];
element.files = [blob1, file1];

We technically could also permit:

element.files.push(blob1);
element.files.splice(0, 1);

But what's tricky is that if we permit the last two, those should update the UI. This means that we couldn't use a plain old js Array.

I don't know what mechanisms WebIDL provides here that would help us. Sounds like a question for bz, Anne, Cameron and public-script-coord.
Anne, could you give your feedback on comment 30?
Flags: needinfo?(annevk)
Sounds like a WebIDL Array (Blob[])?

Comment 33

4 years ago
See https://www.w3.org/Bugs/Public/show_bug.cgi?id=20407
Flags: needinfo?(annevk)

Comment 34

4 years ago
So what I think the conclusion will be is that the setter takes a sequence<Blob> (with special filename extracting semantics for File) and getter always returns a FileList. If we can implement that today without IDL that'd be a good way forward.
(In reply to Anne (:annevk) from comment #34)
> So what I think the conclusion will be is that the setter takes a
> sequence<Blob> (with special filename extracting semantics for File) and
> getter always returns a FileList. If we can implement that today without IDL
> that'd be a good way forward.

According to the WebIDL specification, we MUST NOT use sequence<> for an attribute. Does that mean we shouldn't use sequence<> for input.files?

Comment 36

4 years ago
Well, what we're really doing here is treat the setting and getting as completely different from an IDL perspective. As if you had `FileList setFiles(sequence<Blob>)` and `FileList getFiles()`. In that case it seems fine to use sequence.
I think we should fix (or extend) WebIDL to allow declaring different types for the getter and setter of an attribute.  This has come up numerous times now, and there's no reason I can see for not allowing it.
Depends on: 862800
I guess we will be able to fix this bug when bug 862800 will be fixed.
(In reply to Boris Zbarsky (:bz) from comment #37)
> I think we should fix (or extend) WebIDL to allow declaring different types
> for the getter and setter of an attribute.  This has come up numerous times
> now, and there's no reason I can see for not allowing it.

I think that is the right approach here.  Olli didn't seem to like it in https://www.w3.org/Bugs/Public/show_bug.cgi?id=20407, but I think it makes enough sense.
I posted http://lists.w3.org/Archives/Public/public-script-coord/2013AprJun/0153.html just to make sure people don't have objections.
Looks like WebKit implemented this: https://bugs.webkit.org/show_bug.cgi?id=87154

Is there an intent here to match WebKit exactly, or to do something similar, or something else?
When we floated this idea (see comment 40), people thought this was a terrible API match for JS, so at the moment there are no plans to do anything here, I believe.
Flags: needinfo?(ian)
More precisely, taking random non-FileList objects was considered bad API.
So apparently WebKit only allows actual FileList objects, and actually changes the filelist pointer in the file input to the new object.  See https://www.w3.org/Bugs/Public/show_bug.cgi?id=22682#c6
Could we please simply implement a setter which takes any array-like object and then take all File or Blob objects in the array-like and construct a new FileList containing only those.

I.e. I'd really like all of:

input.files = otherInput.files;
input.files = dropEvent.files;
input.files = [file1, file2];

to work.
> Could we please simply implement a setter which takes any array-like object 

See comment 43?
I don't see that the webkit approach could be better than my proposal though. Do you have links to the objections?
See the link in comment 40 and the following thread.
Based on that thread I'm going to stand by my proposal.

The only real counterproposal was to introduce more array types, i.e. |new FileList([...])| which has been equally frowned upon elsewhere.

I just sent this proposal to the list:

myFileInput.files returns a simple raw JS Array
This array can be modified using normal JS Array functions, like any other Array can.
myFileInput.files is settable to any other raw JS Array, i.e.
myFileInput.files = ["a", 2, myFile3] works.
x = [...]; myFileInput.files = x; myFileInput.files === x; always returns true
Submitting ignores any non-Files in the Array

When exactly the UI is updated is somewhat tricky. We can likely use
Object.observe which would lead to updating at end-of-task I think.
http://lists.w3.org/Archives/Public/public-script-coord/2013OctDec/0083.html

Updated

4 years ago
Keywords: dev-doc-needed

Comment 51

4 years ago
Have you decided on how to make the files attribute writable?

I'm asking because, according to the comment below, it seems that the File API spec will become RC soon, so the window of opportunity for adding a FileList constructor is going to close soon.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=23853#c18

I'm not advocating for (or against) a FileList constructor. I'm bringing this up because I would like a method for changing the files attribute.

Thank you!
My current proposal is here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=23682
Either way this should not affect the FileAPI spec.

Comment 54

3 years ago
So it's been 9-10 months since last comment and nothing has been done, although a working path has been submitted here. The same functionality, as thakis suggested, would work for me and make Firefox behave same way as Chrome or Safari.

Updated

2 years ago
Duplicate of this bug: 1148831
My recommendation, based on the discussion in [1] is to implement the following here:

Make our current DOMFileList implementation implement the JS-iterable protocol (this might already be the case?)

When the .files setter is run, treat the passed in value as an JS-iterable value. If it's not an iterable value, throw a TypeError exception.

Iterate the list and try to convert each value to a DOM Blob. If any value is not a DOM Blob, throw an TypeError exception.

Create a new DOMFileList, and populate it with the Blobs.

Make the getter return the newly created DOMFileList.

The official "decision" hasn't been made in [1] yet. But it definitely feels like this is the direction it's heading. The main unknown is if we'll keep returning a DOMFileList, or if we can return a frozen array, from the getter. But that's something we can worry about separately I think.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=23682
> this might already be the case?

Yes, since it has an indexed getter we stick an iterator on it already.

> When the .files setter is run, treat the passed in value as an JS-iterable value.

OK.  So basically, we want to either use the "any" type or add support for having attributes whose get and set types are different, right?

If we move to returning a frozen array, things are a bit simpler impl-wise, because then we can just make this a sequence type for both get and set.  So if we think we'll end up there it might be better to just use "any" for now and suck up the pain of doing manual iteration in C++...
(In reply to Not doing reviews right now from comment #57)
> OK.  So basically, we want to either use the "any" type or add support for
> having attributes whose get and set types are different, right?

Yes.

> If we move to returning a frozen array, things are a bit simpler impl-wise,
> because then we can just make this a sequence type for both get and set.

I would think so yes. That happening is probably mainly blocked on getting [1] fixed in WebIDL. I don't know what the timeframe on that is.

Alternatively we could just try to return a frozen array right away, and see what feedback we get. That might help move [1] along. We can always switch back to returning a DOMFileList if the feedback is too negative.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=23682
> That happening is probably mainly blocked on getting [1] fixed in WebIDL

Getting what part of it fixed, exactly?

> Alternatively we could just try to return a frozen array right away

If we think we can do that without an obvious backwards compat problem, that seems like the way to go to me, in terms of simplicity of implementation on our end.
(In reply to Not doing reviews right now from comment #59)
> > That happening is probably mainly blocked on getting [1] fixed in WebIDL
> 
> Getting what part of it fixed, exactly?

Checking in a patch to the WebIDL spec which changes it to define that "attribute sequence<X> attrName" creates a getter which returns a frozen Array and a setter which accepts an iterable. And getting at least one other browser vendor to say that this looks good.

> > Alternatively we could just try to return a frozen array right away
> 
> If we think we can do that without an obvious backwards compat problem, that
> seems like the way to go to me, in terms of simplicity of implementation on
> our end.

Clearly there are theoretical problems with this. Someone might modify the DOMFileList prototype and add functions, and then expect myinputElement.files to have those functions.

But I suspect this is very rare. Other than possibly adding functions from the Array prototype, which would be fine.

Even more theoretical would be someone depending on myinputElement.files *not* having things from the Array prototype on it. But this seems even more theoretical.

I think chances are good enough that it's worth trying.
> Checking in a patch to the WebIDL spec which changes it to define that "attribute
> sequence<X> attrName" creates a getter which returns a frozen Array and a setter which
> accepts an iterable.

To be clear, this is a convenience; this behavior can already be defined in prose, yes?

> Clearly there are theoretical problems with this.

The main question is non-theoretical: is someone using .item() on FileLists?
(In reply to Not doing reviews right now from comment #61)
> > Checking in a patch to the WebIDL spec which changes it to define that "attribute
> > sequence<X> attrName" creates a getter which returns a frozen Array and a setter which
> > accepts an iterable.
> 
> To be clear, this is a convenience; this behavior can already be defined in
> prose, yes?

Yes. The purpose of doing the WebIDL check is to see if browser vendors think that the pattern is a good pattern.

> > Clearly there are theoretical problems with this.
> 
> The main question is non-theoretical: is someone using .item() on FileLists?

Doh! Right. I forgot about that. I guess that's something we could actually get telemetry on.

A simple search did turn up a few cases on stackoverflow though, which is concerning.

So I guess we'd need to use a subclass of Array which has a .item function. But that seems to make this more complicated. So maybe the right thing to do is to just use 'attribute any' for now.
Created attachment 8590993 [details] [diff] [review]
part 1.  Move CreateNewFileInstance over into File code so it can be used from other callsites
Attachment #8590993 - Flags: review?(amarchesini)
Assignee: thakis → bzbarsky
Created attachment 8590994 [details] [diff] [review]
part 2.  Allow assigning an iterable to HTMLInputElement.files and have it replace the DOMFileList it's holding with a new one

Jonas, three things I want feedback on:

1) The behavior when null is passed.
2) The behavior when assigning on a non-file input.
3) The behavior when upgrading blobs, and in particular how that part differs from what MozSetFileNameArray does.  See also bug 1146116.
Attachment #8590994 - Flags: review?(amarchesini)
Attachment #8590994 - Flags: feedback?(jonas)
Flags: needinfo?(ian)
Comment on attachment 8590993 [details] [diff] [review]
part 1.  Move CreateNewFileInstance over into File code so it can be used from other callsites

Review of attachment 8590993 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/File.cpp
@@ +344,5 @@
> +  nsRefPtr<MultipartFileImpl> impl =
> +    new MultipartFileImpl(blobImpls, filename, contentType);
> +
> +  nsRefPtr<File> file = new File(GetParentObject(), impl);
> +  return file.forget();  

extra space.
Attachment #8590993 - Flags: review?(amarchesini) → review+
Comment on attachment 8590994 [details] [diff] [review]
part 2.  Allow assigning an iterable to HTMLInputElement.files and have it replace the DOMFileList it's holding with a new one

Review of attachment 8590994 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. Have you checked if we have some web-platform-tests about HTMLInputElement.files getters/setters?

::: dom/html/HTMLInputElement.cpp
@@ +2780,5 @@
> +    }
> +
> +    nsRefPtr<File> blob;
> +    nsresult rv =
> +      UnwrapObject<prototypes::id::Blob, mozilla::dom::File>(&temp.toObject(),

Any reason why you don't use UNWRAP_OBJECT(Blob, &tmp.toObject(), blob)?
Attachment #8590994 - Flags: review?(amarchesini) → review+
> Any reason why you don't use UNWRAP_OBJECT(Blob, &tmp.toObject(), blob)?

Er, no.  Mostly I copy/pasted that part from codegen for a sequence-typed attribute.  ;)  Fixed.
Comment on attachment 8590994 [details] [diff] [review]
part 2.  Allow assigning an iterable to HTMLInputElement.files and have it replace the DOMFileList it's holding with a new one

Review of attachment 8590994 [details] [diff] [review]:
-----------------------------------------------------------------

Is this just stalled on me? It definitely looks good to me.
Attachment #8590994 - Flags: feedback?(jonas) → feedback+
> 1) The behavior when null is passed.

I think the cleanest thing to do would be to clear the list. IIRC we return null when the <input> doesn't contain any files.

> 2) The behavior when assigning on a non-file input.

Probably throw.

> 3) The behavior when upgrading blobs, and in particular how that part
> differs from what MozSetFileNameArray does.  See also bug 1146116.

I don't know how bug 1146116 will turn out. But I hope really really hard to not have to upgrade Blobs to Files here. I guess it's a module owner decision, but I hope I made my opinion clear.
Did you mean bug 1162658 by any chance?
> Is this just stalled on me?

It's mostly stalled on sorting out the blob-upgrading bits.

> I think the cleanest thing to do would be to clear the list.

Great.  That's what I'm doing.

> Probably throw.

Can do.

> Did you mean bug 1162658 by any chance?

No, I meant bug 1146116.  That's what introduced the current MozSetFileNameArray behavior.  It's mostly relevant in that it's an existing "upgrade to File" path we have here, except it always clones the File, since it's assuming it's called from chrome.  And the real question is what setting .files from chrome should do....
Flags: needinfo?(jonas)
Honestly I don't care much about what happens in the case when chrome calls the .files setter. Though of course if it's called on a chrome <input> we should just run the setter as normal.

But other than that, an ideal solution might be that if the .files setter is called when the caller is chrome code, and the 'this' object is a content <input> element then:

* For any Blob/File object which is *not* same-origin with the <input> element, create a new Blob/File with the same contents and metadata (name/lastModified) which belongs to the same origin as the <input>.
* For any Blob/File object which *is* same-origin with the <input> element, just use the passed in object.

But it all depends on how much complexity we want to deal with. It might be fine to "do nothing" which I think means that if the page could access the .files property, but accessing .files[n].size might throw an exception. And then make it the responsibility of the chrome code to make sure to create an object in the right origin first.

What do we do for Element.appendChild? Does it throw if chrome tries to insert a chrome node into a content document?
Flags: needinfo?(jonas)
> but accessing .files[n].size might throw an exception

Right, and that causes problems.  See bug 1146116.

> What do we do for Element.appendChild?

We adopt the argument into the content document.  Then it behaves like any other content node.
(In reply to Not doing reviews right now from comment #73)
> > but accessing .files[n].size might throw an exception
> 
> Right, and that causes problems.  See bug 1146116.

One could argue that that's a bug in the chrome/addon code calling the .files setter. I.e. that that code should call MozSetFileArray, which takes care of this problem, or create new File/Blob objects in the correct scope.

Or we could make chrome code calling the .files setter on a content <input> always throw.

Or we could do something more complex as mentioned in comment 72.

I'm really fine either way here.

> > What do we do for Element.appendChild?
> 
> We adopt the argument into the content document.  Then it behaves like any
> other content node.

Makes sense. Sadly not something we can do here I'd think.
I think we should get rid of MozSetFileArray once this setter exists, fwiw.
(In reply to Not doing reviews right now from comment #75)
> I think we should get rid of MozSetFileArray once this setter exists, fwiw.

Fine by me, for what it's worth; I never entirely liked using a mozPrefixed method for that, but I didn't have any better ideas.  I assume we have ways of dealing with any add-ons that already started using it.
You need to log in before you can comment on or make changes to this bug.