Consider to implement input.webkitdirectory

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: smaug, Assigned: baku)

Tracking

({dev-doc-complete})

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

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

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

Updated

3 years ago
Depends on: 1258490
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.
Reporter

Comment 2

3 years ago
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.
(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.
(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.)
Reporter

Comment 5

3 years ago
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.
(If directory-upload doesn't spec it, we can always toss it into the compat spec.)
(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.
(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.
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.
Reporter

Comment 10

3 years ago
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.)
(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.
Reporter

Updated

3 years ago
Summary: Implement input.webkitdirectory → Consider to implement input.webkitdirectory
Reporter

Updated

3 years ago
Blocks: 1188880
Reporter

Updated

3 years ago
No longer blocks: 1257179
Assignee

Updated

3 years ago
Assignee: nobody → amarchesini
Assignee

Comment 12

3 years ago
Posted patch webkitdirectory.patch (obsolete) — Splinter Review
This is the webkitdirectory patch. It can be reviewed only after many other pending patches, so I don't ask a review for now.
Flags: needinfo?(bugs)
Reporter

Comment 13

3 years ago
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.
Flags: needinfo?(bugs) → needinfo?(miket)
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).
Flags: needinfo?(miket)
Whiteboard: btpp-active
Assignee

Comment 15

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

This patch should be reviewed after bug 1261693
Attachment #8738053 - Flags: review?(bugs)
Reporter

Comment 16

3 years ago
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.
Attachment #8738053 - Flags: review?(bugs) → review-
Filed spec issue: lhttps://github.com/whatwg/compat/issues/54 (this could get moved to directory-upload).
Blocks: 1170774
Assignee

Comment 18

3 years ago
> (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.
Assignee

Comment 19

3 years ago
Attachment #8738053 - Attachment is obsolete: true
Attachment #8753386 - Flags: review?(bugs)
Reporter

Comment 20

3 years ago
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.
Attachment #8753386 - Flags: review?(bugs) → review+
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.
Flags: needinfo?(amarchesini)

Comment 23

3 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/133af19777be
Implement HTMLInputElement.webkitdirectory, r=smaug

Comment 24

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/133af19777be
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee

Updated

3 years ago
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini)
Assignee

Comment 27

3 years ago
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!
Flags: needinfo?(amarchesini) → needinfo?(dietrich)
Depends on: 1282870
Enabled the pref and restarted and re-tested, and got the same exact result. Filed bug 1282870.
Flags: needinfo?(dietrich)
Reporter

Updated

3 years ago
Depends on: 1289254

Updated

3 years ago
Depends on: 1303638
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.