Last Comment Bug 523771 - Support <input type=file multiple>
: Support <input type=file multiple>
Status: VERIFIED FIXED
: dev-doc-complete, verified1.9.2
Product: Core
Classification: Components
Component: HTML: Form Submission (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla1.9.2
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
Depends on: 524533 525736 529859
Blocks: 63687 547710
  Show dependency treegraph
 
Reported: 2009-10-21 20:26 PDT by Jonas Sicking (:sicking) PTO Until July 5th
Modified: 2013-09-25 17:15 PDT (History)
21 users (show)
jonas: in‑testsuite+
chung808: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta2-fixed


Attachments
Patch to fix (45.28 KB, patch)
2009-10-22 12:17 PDT, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Splinter Review
litmus test (1.48 KB, text/html)
2009-10-22 12:35 PDT, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details
Patch v1.1 (45.31 KB, patch)
2009-10-22 18:08 PDT, Jonas Sicking (:sicking) PTO Until July 5th
mozilla+ben: review+
paul: review+
jst: superreview+
mbeltzner: approval1.9.2+
Details | Diff | Splinter Review
Feedback on "Patch v1.1" (5.46 KB, patch)
2009-10-23 19:05 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
Branch patch (46.28 KB, patch)
2009-10-27 16:18 PDT, Jonas Sicking (:sicking) PTO Until July 5th
jonas: review+
jonas: superreview+
jst: approval1.9.2+
Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) PTO Until July 5th 2009-10-21 20:26:26 PDT
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) PTO Until July 5th 2009-10-22 12:17:17 PDT
Created attachment 407816 [details] [diff] [review]
Patch to fix
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2009-10-22 12:35:26 PDT
Created attachment 407823 [details]
litmus test
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2009-10-22 18:08:19 PDT
Created attachment 407916 [details] [diff] [review]
Patch v1.1

This fixes one bug and one review comment in the automated tests for the sessionrestore code.
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2009-10-22 18:09:36 PDT
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 reading much bugmail, email directly) 2009-10-22 18:13:47 PDT
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) 2009-10-23 19:05:08 PDT
Created attachment 408163 [details] [diff] [review]
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) 2009-10-23 19:06:48 PDT
Comment on attachment 407916 [details] [diff] [review]
Patch v1.1

r=me with the above issues addressed
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2009-10-23 19:30:42 PDT
> - 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, jst@mozilla.com) 2009-10-24 19:56:46 PDT
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) PTO Until July 5th 2009-10-24 23:00:53 PDT
Checked in to trunk

http://hg.mozilla.org/mozilla-central/rev/33a5c5092caf
Comment 11 Jonas Sicking (:sicking) PTO Until July 5th 2009-10-25 00:20:16 PDT
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] 2009-10-25 03:14:59 PDT
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] 2009-10-26 08:35:46 PDT
(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) PTO Until July 5th 2009-10-26 09:38:11 PDT
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] 2009-10-26 11:08:51 PDT
(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] 2009-10-27 12:58:39 PDT
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) PTO Until July 5th 2009-10-27 16:18:34 PDT
Created attachment 408727 [details] [diff] [review]
Branch patch

Branch-patch also containing fix for bug 524421
Comment 18 Jonas Sicking (:sicking) PTO Until July 5th 2009-10-27 16:29:18 PDT
Fixed for 1.9.2:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5769de8e06e0
Comment 19 Marco Zehe (:MarcoZ) on PTO until August 15 2009-11-01 22:40:01 PST
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] 2009-11-04 02:50:42 PST
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] 2009-11-04 07:55:25 PST
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] 2009-11-04 09:43:02 PST
Ok, so a slightly modified version relanded later: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d5b2b5b6ad43
Comment 23 Tony Chung [:tchung] 2009-11-04 10:36:33 PST
jonas, given the changes for the later patch, is there any tests in addition i should be looking for?
Comment 24 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-04 16:33:45 PST
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 2009-11-05 08:45:04 PST
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] 2009-11-06 10:17:44 PST
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 [:bz] 2009-11-19 09:37:08 PST
This patch changed submission behavior for empty file inputs: see bug 529859.
Comment 28 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2009-11-20 12:42:25 PST
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] 2009-11-20 13:25:23 PST
yes please!
Comment 30 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-20 21:25:23 PST
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 2010-08-10 23:08:52 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.