Closed Bug 523771 Opened 15 years ago Closed 15 years ago

Support <input type=file multiple>

Categories

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

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.2
Tracking Status
status1.9.2 --- beta2-fixed

People

(Reporter: sicking, Assigned: sicking)

References

Details

(Keywords: dev-doc-complete, verified1.9.2)

Attachments

(4 files, 1 obsolete file)

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.
Attached patch Patch to fix (obsolete) — Splinter Review
Assignee: nobody → jonas
Attachment #407816 - Flags: superreview?(jst)
Attachment #407816 - Flags: review?(bnewman)
Attachment #407823 - Attachment is patch: false
Attachment #407823 - Attachment mime type: text/plain → text/html
Attached patch Patch v1.1Splinter Review
This fixes one bug and one review comment in the automated tests for the sessionrestore code.
Attachment #407816 - Attachment is obsolete: true
Attachment #407916 - Flags: superreview?(jst)
Attachment #407916 - Flags: review?(bnewman)
Attachment #407816 - Flags: superreview?(jst)
Attachment #407816 - Flags: review?(bnewman)
Comment on attachment 407916 [details] [diff] [review]
Patch v1.1

Paul: would be great if you could review the sessionrestore parts
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.
Attachment #407916 - Flags: review?(paul) → review+
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 on attachment 407916 [details] [diff] [review]
Patch v1.1

r=me with the above issues addressed
Attachment #407916 - Flags: review?(bnewman) → review+
> - 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 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!
Attachment #407916 - Flags: superreview?(jst) → superreview+
Checked in to trunk

http://hg.mozilla.org/mozilla-central/rev/33a5c5092caf
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.
Attachment #407916 - Flags: approval1.9.2?
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
Keywords: dev-doc-needed
Target Milestone: --- → mozilla1.9.3a1
(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
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.
(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.
Whiteboard: [doc-waiting-1.9.3]
Depends on: 524533
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus+
Attached patch Branch patchSplinter Review
Branch-patch also containing fix for bug 524421
Attachment #408727 - Flags: superreview+
Attachment #408727 - Flags: review+
Attachment #408727 - Flags: approval1.9.2?
Attachment #407916 - Flags: approval1.9.2?
Attachment #408727 - Flags: approval1.9.2? → approval1.9.2+
Whiteboard: [doc-waiting-1.9.3]
Target Milestone: mozilla1.9.3a1 → mozilla1.9.2
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).
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
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.
Keywords: verified1.9.2
Ok, so a slightly modified version relanded later: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d5b2b5b6ad43
jonas, given the changes for the later patch, is there any tests in addition i should be looking for?
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.
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.
Depends on: 529859
This patch changed submission behavior for empty file inputs: see bug 529859.
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?
yes please!
Flags: in-testsuite?
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.
Flags: in-testsuite? → in-testsuite+
Blocks: 547710
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.
Blocks: 63687
No longer depends on: 63687
Component: HTML: Form Submission → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.