Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: correctly support the webkitdirectory input attr #18343

Merged
merged 1 commit into from May 21, 2019

Conversation

@MarshallOfSound
Copy link
Member

MarshallOfSound commented May 17, 2019

Fixes #839

The implementation here was loosely inspired by the implementation in
//chrome found in their FileSelectHelper. I.e. That's where the usage
of net::DirectoryList comes from.

Refs: https://cs.chromium.org/chromium/src/chrome/browser/file_select_helper.cc

Notes: Fixed support for the webkitdirectory attribute on input[type=file] elements

Fixes #839

The implementation here was loosely inspired by the implentation in
//chrome found in their FileSelectHelper.  I.e. That's where the usage
of net::DirectoryLister comes frome.

Refs: https://cs.chromium.org/chromium/src/chrome/browser/file_select_helper.cc
@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented May 20, 2019

Do we also want this to target 5-0-x? This could be perceived as semver patch or minor, depending on strictness of evaluation 🤔

@zcbenz
zcbenz approved these changes May 21, 2019
@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented May 21, 2019

If an app is using the webkitdirectory attribute then it is currently relying on the old buggy behavior and will likely break with this change, so I think we should at least bump the minor version for it, and it may even belong to the breaking changes.

@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

MarshallOfSound commented May 21, 2019

then it is currently relying on the old buggy behavior and will likely break with this change

I'm firmly in the camp of "if you're relying on broken, non-spec behavior then when it's fixed that's on you to deal with". But will defer to @codebytere / @electron/wg-releases on this one 👍

Copy link
Member

ckerr left a comment

Added to the 2019-05-22 Releases WG agenda.

Marking as 'Request changes' only to set a flag saying "let's let the WG decide before taking action".

Copy link
Member

codebytere left a comment

Approving the implementation, but agreeing that this should be routed through the Releases WG before the backport PR is merged.

@ckerr ckerr added the target/5-0-x label May 21, 2019
@ckerr
ckerr approved these changes May 21, 2019
Copy link
Member

ckerr left a comment

Moving the Request changes flag to the trop backports at @codebytere's request so that this can be unblocked in the nightly releases

@ckerr ckerr removed the target/5-0-x label May 21, 2019
@ckerr ckerr merged commit a8ff689 into master May 21, 2019
14 of 15 checks passed
14 of 15 checks passed
Backportable? - 5-0-x Cancelled
Details
Artifact Comparison No Changes
Details
Backportable? - 6-0-x Clean Backport
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190517.13 succeeded
Details
electron-arm64-testing Build #20190517.13 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented May 21, 2019

Release Notes Persisted

Fixed support for the webkitdirectory attribute on input[type=file] elements

@ckerr ckerr deleted the webkitdirectory branch May 21, 2019
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented May 21, 2019

I have automatically backported this PR to "6-0-x", please check out #18389

@trop trop bot added in-flight/6-0-x and removed target/6-1-x labels May 21, 2019
Copy link
Member

deepak1556 left a comment

Sorry for the late review. Also destroy FileSelectHelper when WebContentsDestroyed is called. Couldn't mark this line in the code with github review.

}
void DirectoryListerHelper::OnListDone(int error) {
std::vector<FileChooserFileInfoPtr> file_info;
for (auto path : paths_)

This comment has been minimized.

Copy link
@deepak1556

deepak1556 May 21, 2019

Member

Why copy ? could have taken references.

private:
friend class base::RefCounted<FileSelectHelper>;

~FileSelectHelper() override {}

void EnumerateDirectory(base::FilePath base_dir) {
auto* lister = new net::DirectoryLister(

This comment has been minimized.

Copy link
@deepak1556

deepak1556 May 21, 2019

Member

Whats the lifetime of the lister ? I don't see it being destroyed.

base_dir = paths[0];

// Actually enumerate soemwhere off-thread
base::SequencedTaskRunnerHandle::Get()->PostTask(

This comment has been minimized.

Copy link
@deepak1556

deepak1556 May 21, 2019

Member

This doesn't mean off-thread, it will be a task that runs on the same sequence as where its called but will be guaranteed to run in a defined order. Prefer base::PostTaskWithTraits.

@jkleinsc

This comment has been minimized.

Copy link
Contributor

jkleinsc commented May 22, 2019

Per discussion with @electron/wg-releases it was decided to not backport this to 5-0-x

@mklnz

This comment has been minimized.

Copy link

mklnz commented Nov 2, 2019

So previously I was able to get the path of the directory selected. However with the "fixed" version I'm only able to get a list of files within that directory, how to get the directory path if there are no files inside said directory since the target.files array is empty?

files: FileList
  length: 0
@erickzhao erickzhao referenced this pull request Nov 2, 2019
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.