FileList should contain only Files, not Directories

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: smaug, Assigned: baku)

Tracking

36 Branch
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment)

This is totally embarrassing. I reviewed bug 1173320, but realized we don't want that.
<input type=file directory>.files should be null 
https://wicg.github.io/directory-upload/proposal.html#file-input
and that proposal nor http://w3c.github.io/filesystem-api/ modifies FileList.

But, <input type=file webkitdirectory> seems to generate non-null .files, however without any Directory objects. Only Files with webkitrelativepath.

So https://wicg.github.io/directory-upload/proposal.html could be make more compatible with
webkitdirectory by allowing non-null .files, or we could implement webkitdirectory, so that if 
<input> has that, .files would contain all the File objects, and if <input> has just directory attribute, use the new async getFilesAndDirectories().
Jonathan, you might know better the current state of the proposal.
Why was bug 1173320 filed? Perhaps some older version of the proposal had Directories in FileList?
Flags: needinfo?(jwatt)
Ah, this is an unfortunate crossing of wires, but maybe (maybe) it won't be a waste of effort. (I thought I asked one of you about why you were continuing working on this and ended up thinking it was for something else.)

Basically that bug came about because HTMLInputElement.getFilesAndDirectories introduces a different API to .files to try to fix the delay-before-notification issue with <input type=file>, but only for directory picking not file picking, not file picking, and I was/am unhappy about that.

The delay-before-notification issue is that the time between when a use picks some files/directories and the page being notified with an 'input' event can potentially be quite long since building the FileList requires I/O (to gather up lastModifiedDate etc.), and this can lead to poor user experience.

Counter-intuitively, it is actually file picking (rather than directory picking) which is most at risk of long delays since users are more likely to select a large number of files in a picker than a large number of directories (the number of items *in* any picked directories makes no difference to the delay).

To solve the delay-before-notification issue, directory picking allows the 'input' event to fire immediately on the user making their selection by requiring that the .files FileList is not created (so no I/O before notification), and instead consumers call HTMLInputElement.getFilesAndDirectories to get a Promise.

Basically I didn't and don't like the fact that file and directory picking involves quite different API just to solve the delay-before-notification problem for the latter only. I would kind of prefer that we do away with HTMLInputElement.getFilesAndDirectories, put Directory objects in the the .files FileList and find some other way to solve the delay-before-notification problem for BOTH file and directory picking in the same way. I see that as more dev friendly.

Perhaps we could fire a 'willchange' event or something when the user makes their selection before we start doing I/O? The question about that idea is whether devs are more likely to put up some sort of "waiting..." notification to users between an 'input' event and Promise resolving than they are to add code to listen for a 'willchange' event to do the same. I'm not sure, but maybe they are which is partly why I hadn't pursued bug 1173320 (well, that and the need to implement Secure Contexts).

Oh, and there's the issue of symmetry with <drag event>.DataTransfer. In this case the drag event can't be dispatched until the DataTransfer.files is populated, and I don't see us firing a 'will-get-drag-event-with-files' event so probably it's not possible to solve the delay-before-drag-event for *file* picking. Not sure if that means we should try to solve it for directory picking with DataTransfer.getFilesAndDirectories given what I said above about directories being less of an issue than files...
Flags: needinfo?(jwatt)
Assignee: nobody → amarchesini
Blocks: 1188880
No longer blocks: 1257179
Whiteboard: btpp-active
Attachment #8736668 - Flags: review?(bugs)
Comment on attachment 8736668 [details] [diff] [review]
fileList.patch

>-    // |aCount| is the number of Files or Directory for this FileList.
>+    uint32_t tag, offset;
>+    // Offset is the index of the blobImpl from which we can find the blobImpl
find the _first_ blobImpl for the FileList, right?
Perhaps rename offset to something more descriptive?

>+    // for this FileList.
>+    if (!JS_ReadUint32Pair(aReader, &tag, &offset)) {
>+      return nullptr;
>+    }
>+
>+    MOZ_ASSERT(tag == 0);
ok, so we aren't actually storing any tag here, but just a dummy value. Perhaps rename variable 'tag' to something else, 
or actually use the tag for blobimpl there? (and if latter, fix the comment which tells how FileList is serialized.)


>+++ b/dom/events/DataTransfer.cpp
>@@ -858,23 +858,23 @@ DataTransfer::GetFilesAndDirectories(Err
> 
>   if (!mFileList) {
>     GetFiles(aRv);
>     if (NS_WARN_IF(aRv.Failed())) {
>       return nullptr;
>     }
>   }
> 
>-  Sequence<OwningFileOrDirectory> filesAndDirsSeq;
>-  mFileList->ToSequence(filesAndDirsSeq, aRv);
>+  Sequence<RefPtr<File>> filesSeq;
>+  mFileList->ToSequence(filesSeq, aRv);
>   if (NS_WARN_IF(aRv.Failed())) {
>     return nullptr;
>   }
> 
>-  p->MaybeResolve(filesAndDirsSeq);
>+  p->MaybeResolve(filesSeq);
Oh, we don't support directory dnd yet.
I see
        if (isDir) {
          continue;
        }
in DataTransfer::GetFileListInternal
Do we have a bug open to support directory dnd? 
The method is behind dom.input.dirpicker, so need to fix dnd before enabling that pref.
Attachment #8736668 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/3c507567ab06
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.