1258482 - FileList should contain only Files, not Directories

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

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().

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 2

[Jonathan Watt [: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...

Comment 3

[Andrea Marchesini [:baku]]

Attachment: fileList.patch

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/166555b48e77

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c507567ab06

Comment 7

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66834c916e31

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3c507567ab06