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

Remove the / -> : replacement from the File constructor #41

Closed
Ms2ger opened this issue Jun 16, 2016 · 9 comments · Fixed by #171
Closed

Remove the / -> : replacement from the File constructor #41

Ms2ger opened this issue Jun 16, 2016 · 9 comments · Fixed by #171

Comments

@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 16, 2016

This was added in 5cd3dda, allegedly in response to https://www.w3.org/Bugs/Public/show_bug.cgi?id=23147. Neither Chrome nor Gecko seems to implement it.

@inexorabletash
Copy link
Member

sgtm

@mkruisselbrink
Copy link
Collaborator

Looking at https://wpt.fyi/results/FileAPI/file/File-constructor.html?label=experimental&label=master&aligned it appears that "Neither Chrome nor Gecko seems to implement it." is perhaps no longer true? It seems that now Gecko is the odd one out and actually does implement what the current spec says...

@TimothyGu
Copy link
Member

Gecko implemented this in https://bugzilla.mozilla.org/show_bug.cgi?id=1321534 (2016). The actual discussion thread is https://lists.w3.org/Archives/Public/public-webapps/2013JulSep/0379.html, where there was quite a bit of back and forth.

FWIW, Servo implements it too with a reference to this issue.

@annevk
Copy link
Member

annevk commented Jun 2, 2020

@bakulf mentions the Entries API as motivation, which seems like it would still be relevant?

@bakulf
Copy link

bakulf commented Jun 2, 2020

In the Entries API, a name cannot contain '/' (https://wicg.github.io/entries-api/#names-paths). That spec uses '/' as directory separator here: https://wicg.github.io/entries-api/#evaluate-a-path
We should try to keep the File API compatible with the Entries API.

@mkruisselbrink
Copy link
Collaborator

I see, it's unfortunate Firefox implemented this change without noticing that this open issue existed, but I guess ultimately that is on us spec editors for not clearly marking these things in the spec.

I'm not sure what the Entries API has to do with this. The Entries API does not consume File objects, it produces them. And when it produces them it doesn't do so by invoking the File constructor. The logic here only effect File objects created by javascript it seems completely irrelevant what the Entries API does (well, not irrelevant, it is good to be consistent. But that has nothing to do with being compatible).

(also I might be missing something/not finding the right message, but I don't see anything in that discussion thread specifically related to replacing '/' with ':'?)

If we do actually want to disallow '/' in the name attribute of a script created File, I'm not sure why replacing it with ':' makes sense. Wouldn't it make more sense to just reject entirely?

Also, non-traditional file systems, like for example Google Drive, do allow '/' in file names. And it's not immediately obvious to me why we would need to stop them from creating File objects with those names. Different platforms/file systems have different limitations, but we're not limiting File objects to the strictest possible subset of filenames (8.3 filenames only anyone?)

@annevk
Copy link
Member

annevk commented Jun 3, 2020

Replacing rather than throwing does seem bad and Entries API does indeed only produce File objects, it doesn't appear to consume them. Entries API does appear to define a model for files in https://wicg.github.io/entries-api/#names-paths onward and that does conflict with allowing various code points in the name, including /. And its processing model does seem to get confused if a name with / would appear as it deals with paths as single strings.

Looking at this I think a reasonable outcome would be:

  1. Remove the replace operation from File API and Firefox. And add/change a test.
  2. File an issue against Entries API to ensure it's model for files is aligned with File API. We shouldn't have multiple conflicting models of what files are in the web platform.

@andreubotella
Copy link
Member

andreubotella commented Jun 11, 2020

There's also the issue that HTML specifies that filenames for <input type="file"> must not contain path components separated by '\\', but it says nothing about '/'. I don't think this is an issue that any of the algorithms in HTML would choke on (unless you count input.value being "C:\\fakepath\\" + filename), but in the interest of consistency, if the File constructor throws on one, it should also throw on the other.

romainmenke added a commit to romainmenke/polyfill-library that referenced this issue Jul 14, 2020
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 17, 2020
Change the test to follow the proposed change in w3c/FileAPI#41,
now that all browsers agree to not replace /.

Depends on D86981

Differential Revision: https://phabricator.services.mozilla.com/D87112

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1650607
gecko-commit: 365ae38a5bca68e4d8c8cf25b8f5230e306b0f09
gecko-integration-branch: autoland
gecko-reviewers: baku
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 17, 2020
Change the test to follow the proposed change in w3c/FileAPI#41,
now that all browsers agree to not replace /.

Depends on D86981

Differential Revision: https://phabricator.services.mozilla.com/D87112
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 17, 2020
Change the test to follow the proposed change in w3c/FileAPI#41,
now that all browsers agree to not replace /.

Depends on D86981

Differential Revision: https://phabricator.services.mozilla.com/D87112

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1650607
gecko-commit: 365ae38a5bca68e4d8c8cf25b8f5230e306b0f09
gecko-integration-branch: autoland
gecko-reviewers: baku
sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this issue Sep 23, 2020
sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this issue Sep 23, 2020
Change the test to follow the proposed change in w3c/FileAPI#41,
now that all browsers agree to not replace /.

Depends on D86981

Differential Revision: https://phabricator.services.mozilla.com/D87112
@domenic
Copy link
Contributor

domenic commented Feb 28, 2021

The WPTs now expect no replacement, but the spec still says replacement must happen. Can we fix the spec?

This was referenced Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants