1258489 - Consider to implement input.webkitdirectory

Status: RESOLVED FIXED

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

Description

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

MS is implementing it too, before the directory upload proposal.
I filed also https://github.com/WICG/directory-upload/issues/29

Comment 1

[Jonathan Watt [:jwatt]]

I'd really like to hold off on this. The webkitdirectory behavior requires traversing the entire tree under a directory to build up a flat list of files along with their lastModifiedDate etc. and as a result can block for a while due to I/O. It's crappy API. I'd rather ship our async API first, otherwise we have no leverage to push devs onto an API that is better for users. MS is going to implement the async API, and it sounds like Google will ship it too if we do first.

That said, note that we did have an implementation of the flat list thing that I removed in bug 1173390. If we do implement this then the code there may or may not be useful to refer to.

Comment 2

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

That I/O should happen on non-main thread, so at least it wouldn't cause jank.

And given that the spec has still some issues, we might want to implement webkitdirectory and webkitrelativepath if the upload spec doesn't stabilize very soon.

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Olli Pettay [:smaug] from comment #2)
> That I/O should happen on non-main thread, so at least it wouldn't cause
> jank.

Indeed. The code I removed in bug 1173390 did the I/O off main thread. The issue is there can be a delay between when the user confirms and closes the picker, and when the page gets an 'input' event.

Comment 4

[Mike Taylor [:miketaylr]]

(In reply to Jonathan Watt [:jwatt] from comment #1)
> MS is going to implement the async API, and it sounds like Google
> will ship it too if we do first.

https://lists.w3.org/Archives/Public/public-wicg/2016Mar/0002.html suggests Microsoft isn't going to do that this year, instead focusing on the webkit stuff for compat. 

https://www.chromestatus.com/metrics/feature/timeline/popularity/47 indicates usage is really low (maybe Google Drive is the one site that's actually using this in production.)

Comment 5

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

So I think we should implement webkitdirectory and webkitrelativepath too, but not the dnd stuff and ask MS to not ship that not-so-good API.
This would mean spec'ing input.webkitdirectory and file.webkitrelativepath.

Comment 6

[Mike Taylor [:miketaylr]]

(If directory-upload doesn't spec it, we can always toss it into the compat spec.)

Comment 7

[Jonathan Watt [:jwatt]]

(In reply to Mike Taylor [:miketaylr] from comment #4)
> https://www.chromestatus.com/metrics/feature/timeline/popularity/47
> indicates usage is really low (maybe Google Drive is the one site that's
> actually using this in production.)

The only other non-toy site that I'm aware of that provides directory upload is mega.nz, and they only allow that with drag and drop not webkitdirectory to get a directory picker.

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Olli Pettay [:smaug] from comment #5)
> So I think we should implement webkitdirectory and webkitrelativepath too,
> but not the dnd stuff

Why? Shouldn't we have some more evidence of sites that we're not working as well in? I'm pretty sure Drive would add support for Firefox promptly once we ship our implementation of the Directory Upload proposal.

> and ask MS to not ship that not-so-good API.

I don't imagine we can persuade them not to do that given that they weren't persuaded to not ship webkitdirectory.

Also remember that Safari does not support directory picking or DnD at all.

Comment 9

[Jonathan Watt [:jwatt]]

So as I said on IRC, if we can't push devs onto a better API in this situation then I don't know when we can.

Comment 10

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

The decision should definitely go via dev.platform, and I'm trying to get more feedback from other browser vendors too. But if there is a risk that we'll be the only ones to implement the new API, no one will use it.
Given that webkitdirectory + webkitrelativepath is rather simple thing, implementing that and the new API and warn about use of webkitdirectory might make sense. That might give better UX for the users faster, but longer term the ux would even improve when sites move to use the new API.
(I would probably think very differently if webkitdirectory was a hard API to implement.)

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to Olli Pettay [:smaug] from comment #10)
> The decision should definitely go via dev.platform,

Yup.

> and I'm trying to get
> more feedback from other browser vendors too.

Sounds good.

> Given that webkitdirectory + webkitrelativepath is rather simple thing,
> implementing that and the new API and warn about use of webkitdirectory
> might make sense.

It's not about how hard it is. We already had this implemented and removed it. It's about the fact that once devs have webkitdirectory which avoids the use of Promises at the expense of poorer user experience, they will not switch to the newer API because the poorer user experience isn't common enough for devs to pay attention to it. They'll just go with the easier API. I feel if we ship webkitdirectory we sabotage our Directory Upload work and any chance to move devs over to an API that is less harmful to users. We might as well remove it at the same time.

> But if there is a risk that
> we'll be the only ones to implement the new API, no one will use it.

No-one is really using webkitdirectory either right now. Can you point to the problem sites that we're not working on that's driving us to implement webkitdirectory? We should see if your fear actually comes about before giving up and implementing it.

> That might give better UX for the users faster, but longer
> term the ux would even improve when sites move to use the new API.

As noted above, I think they won't move if we give them the simpler API.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: webkitdirectory.patch

This is the webkitdirectory patch. It can be reviewed only after many other pending patches, so I don't ask a review for now.

Comment 13

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

Mike, did you have chance to discuss with MS about their plans about webkit* APIs related to file handling?

If we add webkitdirectory, Bug 1258490 should be also fixed, and we'll need some tests for submitting directories with some subdirectories containing files.

Comment 14

[Mike Taylor [:miketaylr]]

I tried to! Greg Whitworth recommended I email Ali directly (who wasn't at the conf) -- I've just sent an email (and cc'd you smaug, hope that's OK).

Comment 15

[Andrea Marchesini [:baku]]

Comment on attachment 8738053 [details] [diff] [review]
webkitdirectory.patch

This patch should be reviewed after bug 1261693

Comment 16

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

Comment on attachment 8738053 [details] [diff] [review]
webkitdirectory.patch

>diff --git a/dom/filesystem/tests/test_webkitdirectory.html b/dom/filesystem/tests/test_webkitdirectory.html
>new file mode 100644
>--- /dev/null
>+++ b/dom/filesystem/tests/test_webkitdirectory.html
>@@ -0,0 +1,96 @@
>+<!DOCTYPE HTML>
>+<html>
>+<head>
>+  <title>Test for webkitdirectory and webkitRelativePath</title>
>+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
>+</head>
>+
>+<body>
>+<input id="inputFileWebkitDirectory" type="file" webkitdirectory></input>
>+<input id="inputFileWebkitDirectoryAndDirectory" type="file" webkitdirectory directory></input>
>+<input id="inputFileDirectory" type="file" directory></input>
So 'directory' shouldn't affect to anything. Want to comment on that here.

(Upload proposal uses allowdirs, which is totally different thing. We will probably need a test checking how allowdirs + webkitdirectory work together.)



I don't see anything in this patch implementing webkitdirectory webidl attribute.
And use of webkitdirectory DOM attribute should be also behind a pref, the same pref as webkitrelativepath, I think.

Comment 17

[Mike Taylor [:miketaylr]]

Filed spec issue: lhttps://github.com/whatwg/compat/issues/54 (this could get moved to directory-upload).

Comment 18

[Andrea Marchesini [:baku]]

> (Upload proposal uses allowdirs, which is totally different thing. We will
> probably need a test checking how allowdirs + webkitdirectory work together.)

In our code we still have 'attribute boolean directory'. We should change it in a separate bug.
I'm testing what you suggested, but with the previous name of this attribute.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: webkitdirectory.patch

Comment 20

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

Comment on attachment 8753386 [details] [diff] [review]
webkitdirectory.patch

>+// This callback is used for postponing the calling of SetFilesOrDirectories
>+// onces the exploration of the directory is completed.
once? Though there is something odd with the sentence.


What happens if, while getting the files for some directory containing lots of files, user tries to open the filepicker again
and selects something new? Please test.


HTMLInputElement::GetOrCreateGetFilesHelper(bool aRecursiveFlag,
>+                                            ErrorResult& aRv)
>+{
>+  nsCOMPtr<nsIGlobalObject> global = OwnerDoc()->GetScopeObject();
>+  MOZ_ASSERT(global);
>+  if (!global) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return nullptr;
>+  }
>+
>+  if (aRecursiveFlag) {
>+    if (!mGetFilesRecursiveHelper) {
>+      mGetFilesRecursiveHelper =
>+       GetFilesHelper::Create(global,
...
>+  if (!mGetFilesNonRecursiveHelper) {
>+    mGetFilesNonRecursiveHelper =
>+     GetFilesHelper::Create(global,

now that I think of this some more... not about this bug, but could we please store
those helpers as node properties. type=file is rarely used, so we shouldn't really increase the size of all the input elements.
That change could be done in a separate bug.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17e20404362d

Comment 22

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

Had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7aba31d799f2 to cleanly back out bug 1261693, which caused some test failures.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/133af19777be
Implement HTMLInputElement.webkitdirectory, r=smaug

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/133af19777be

Comment 25

[Jean-Yves Perrier [:teoli]]

Updated:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#attr-webkitdirectory
https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement
and
https://developer.mozilla.org/en-US/Firefox/Releases/49#HTML


Created:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement/webkitDirectory

Comment 26

[Dietrich Ayala (:dietrich)]

What's the expected behavior with this change?

Using Nightly 50.0a1 (2016-06-28) and Addy Osmani's sample at http://jsfiddle.net/addyo/Ha979/ I got the following results:

* File picker only picks files, not directories, which seems like the opposite of the expected behavior. Chrome doesn't allow file picking, only directories (which perhaps is a bug... https://bugs.chromium.org/p/chromium/issues/detail?id=59818).

* Dropping a directory onto the <input> lists the directory name, not the files contained within. Chrome lists the files contained within the dropped directory, but not the directory itself.

Comment 27

[Andrea Marchesini [:baku]]

This feature is current behind pref. Can you test it again enabling dom.webkitBlink.dirPicker.enabled ?
And in case, can you file a new bug? Thanks!

Comment 28

[Dietrich Ayala (:dietrich)]

Enabled the pref and restarted and re-tested, and got the same exact result. Filed bug 1282870.