523771 - Support <input type=file multiple>

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If the 'multiple' attribute is set on an <input type=file> then we should use a filepicker that allows multiple files to be selected.

All the files should then be used when submitting, and be available through the HTMLInputElement.files propery.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: litmus test

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v1.1

This fixes one bug and one review comment in the automated tests for the sessionrestore code.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 407916 [details] [diff] [review]
Patch v1.1

Paul: would be great if you could review the sessionrestore parts

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 407916 [details] [diff] [review]
Patch v1.1

r=zpao for sessionstore bits

I know we talked about this on IRC, but for posterity's sake, this will cause type="file" form data to be lost between an upgrade/downgrade. I'm OK with that since it's not a typical case and files fields are much easier to fill back in than text fields.

Comment 6

[Ben Newman (:bnewman) (:benjamn)]

Attachment: Feedback on "Patch v1.1"

Summary:

- Weird indentation & unnecessary reevaluation of mozGetFileNameArray within the if (node.type == "file") {...} block.

- Preallocating nsTArray<nsString> of known size in nsHTMLInputElement::MozSetFileNameArray.

- "A better way to downcast an already_AddRefed" in nsHTMLInputElement::GetFileArray.

- Avoiding multiple SimpleTest.waitForExplicitFinish() calls in test_bug523771.html.

- Adding back the mCachedState fallback in nsFileControlFrame::GetFormProperty. Was that taken out for a reason? If so, feel free to ignore this suggestion.

Comment 7

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 407916 [details] [diff] [review]
Patch v1.1

r=me with the above issues addressed

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> - Weird indentation & unnecessary reevaluation of mozGetFileNameArray within
> the if (node.type == "file") {...} block.

Looks good. (I optimized for simple code given that this is just a test, but your solution is just as fine)

> - Preallocating nsTArray<nsString> of known size in
> nsHTMLInputElement::MozSetFileNameArray.

Looks good.

> - "A better way to downcast an already_AddRefed" in
> nsHTMLInputElement::GetFileArray.

Ugh, that scares me. I'm not even sure that this is entirely safe, or if it effectively is a reinterpret_cast. getter_AddRefs can return a void**, which means that you're assigning from a nsILocalFile* to a void** that's pointing to an nsIFile. It'll work since reinterpret_cast between nsIFile and nsILocalFile is safe, but it still scares me. I'd rather leave it as it is.

> - Avoiding multiple SimpleTest.waitForExplicitFinish() calls in
> test_bug523771.html.

Looks good.

> - Adding back the mCachedState fallback in nsFileControlFrame::GetFormProperty.
> Was that taken out for a reason? If so, feel free to ignore this suggestion.

Actually, this was a safty-change. We've had a lot of issues in the past with this code when the filename was kept in various variables around the code. So at some point I simplified things so that filenames are always kept in the element and kept there separately from anything else.

Missed this one place where a value can "leak" from mCachedState through the filepicker dialog to the filename member. So the change here was fixing that.

So I'd rather keep things as they are in my patch.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 407916 [details] [diff] [review]
Patch v1.1

- In content/html/content/public/nsIFileControlElement.h:

-  virtual void GetFileName(nsAString& aFileName) = 0;
+  virtual void GetReadableFileName(nsAString& aFileName) = 0;

Maybe name this GetDisplayFileName()? Either way is fine with me tho, your call?

sr=jst!

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in to trunk

http://hg.mozilla.org/mozilla-central/rev/33a5c5092caf

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 407916 [details] [diff] [review]
Patch v1.1

The patch ended up being a bit bigger than I originally thought, but it's still really simple logic.

It's really simple to test, and the contained automated tests cover most of it.

The only thing we can't automatically test is the actual filepicker. This is something we're currently lacking tests for. We should create a litmus test for this. I've attached a test that we can base a litmus test on.

The value for authors is pretty high. For example this provides the last remaining piece for gmail to be able to throw out the use of flash for their attachment UI.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 407916 [details] [diff] [review]
Patch v1.1

>+  void mozGetFileNameArray(out unsigned long aLength,
>+                           [array,size_is(aLength), retval] out wstring aFileNames);

aLength should be optional, per http://weblogs.mozillazine.org/bz/archives/020274.html

Comment 13

[Tony Chung [:tchung]]

(In reply to comment #11)
> The only thing we can't automatically test is the actual filepicker. This is
> something we're currently lacking tests for. We should create a litmus test for
> this. I've attached a test that we can base a litmus test on.

Crashing and restoring sessions does not persist the files that i single and multi picked.   I used crashme.xpi to test that.   Shall i open a bug for this?

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b2pre) Gecko/20091026 Namoroka/3.6b2pre

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Tony: Were you by any chance using a https page when testing? Such as loading the attachment directly in bugzilla? Note that we don't do form-fields-restore on https pages.

Sorry, didn't notice this until after having attached the litmus test to bugzilla.

Comment 15

[Tony Chung [:tchung]]

(In reply to comment #14)
> Tony: Were you by any chance using a https page when testing? Such as loading
> the attachment directly in bugzilla? Note that we don't do form-fields-restore
> on https pages.
> 
> Sorry, didn't notice this until after having attached the litmus test to
> bugzilla.

Ah yes, i was using https.   i ran the file locally and restoring session is now persisting the files.  Next step is to get a testplan and a few manual tests into litmus.

Comment 16

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091027 Minefield/3.7a1pre 
and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091027 Minefield/3.7a1pre

Added a test to litmus.mozilla.org
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=9629.  If we get real world sites that use this api, please post it here so i can enhance our testcase selection.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Branch patch

Branch-patch also containing fix for bug 524421

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Fixed for 1.9.2:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5769de8e06e0

Comment 19

[Marco Zehe (:MarcoZ)]

Note to davidb: This does not appear to have any immediate a11y impact. The one thing that currently is not indicated is the fact that you can select multiple files in the "Open" dialog in Windows. But that's not under our control, because this is a Windows dialog (or other platform respectively).

Comment 20

[Dão Gottwald [:dao]]

As far as I can tell, this is not actually on 1.9.2 any more, as of http://hg.mozilla.org/releases/mozilla-1.9.2/rev/172b299874ef

Comment 21

[Tony Chung [:tchung]]

The testcase passes on build Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b2pre) Gecko/20091104 Namoroka/3.6b2pre.   I can select multiple files.

Comment 22

[Dão Gottwald [:dao]]

Ok, so a slightly modified version relanded later: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d5b2b5b6ad43

Comment 23

[Tony Chung [:tchung]]

jonas, given the changes for the later patch, is there any tests in addition i should be looking for?

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No, in fact the opposite. The current tests should be failing on the branch since I had to remove some functionality that for some reason wasn't working on the branch. It's "just" session restore that isn't working for multiple files. I'll file a separate bug on that so we can decide what to do about tests over in that bug.

Comment 25

[Fabian (Crash) Grutschus]

dev doc added
https://developer.mozilla.org/en/DOM/Input
https://developer.mozilla.org/en/DOM/FileList
https://developer.mozilla.org/En/DOM/File

and in "Firefox 3.6 for developers" and also was documented here:
https://developer.mozilla.org/En/NsIDOMFile

Comment 26

[Eric Shepherd [:sheppy]]

While we now have some documentation for these objects, we should add an article about how to handle file uploading that would include this information as well as information about doing progress monitoring and the like, all in one place. I'm marking this as doc complete, however, since I'm creating a stub for that article to remind me to write it out before the ship of 3.6.

Comment 27

[Boris Zbarsky [:bzbarsky]]

This patch changed submission behavior for empty file inputs: see bug 529859.

Comment 28

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Relanded the sessionstore part on branch now that bug 513428 landed.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1756a41d1fe3

Tony, this bug needs in-testsuite+ right?

Comment 29

[Tony Chung [:tchung]]

yes please!

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yup, the chrome-tests should be testing this as much as can be tested automatically. The rest (the filepicker dialogs) should be covered by the litmus tests.

Comment 31

[pd]

I've looked everywhere and can't find a way to process these multiple file fields on the server-side with Perl. All documentation refers to PHP for the server-side processing. Apologies in advance for posting this question here as I expect it's not strictly in context but I'm at my wits end trying to use this multiple ability.