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

Chromium supports webkitdirectory on mobile #25036

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

danielhjacobs
Copy link
Contributor

@danielhjacobs danielhjacobs commented Nov 11, 2024

Summary

Android Chrome now supports webkitdirectory. It crashed the browser in Chrome 131, but a fix for that came out in Chrome 132 and a fix for webkitRelativePath to use the actual path instead of the content-URI also barely eked into Chrome 132 (seems to be included in Chrome Canary 132.0.6834.0)

Test results and supporting details

https://issues.chromium.org/issues/40248532
https://issues.chromium.org/issues/376834374
https://issues.chromium.org/issues/377716464

Related issues

Fixes #24938

@github-actions github-actions bot added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Nov 11, 2024
@danielhjacobs danielhjacobs marked this pull request as ready for review November 12, 2024 14:35
@danielhjacobs
Copy link
Contributor Author

I see the errors:

api.HTMLInputElement.webkitdirectory - Error → The data for (chrome_android) says no support, but contains additional properties that suggest support.
api.HTMLInputElement.webkitdirectory - Error → chrome_android cannot have a version_added: false in an array of statements.

How can I convey the meaning I intend while passing these checks?

@danielhjacobs
Copy link
Contributor Author

danielhjacobs commented Nov 12, 2024

Does this make sense?

            "chrome_android": {
              "version_added": "132",
              "notes": [
                "In Chrome 131, the property could be set, but choosing a directory crashed the browser.",
                "Before Chrome 131, the property could be set, but had no effect."
              ]
            },

@caugner
Copy link
Contributor

caugner commented Nov 15, 2024

Does this make sense?

It makes sense, but based on the following existing case, it might be more consistent to move those notes to a [131, 132) statement with partial implementation.

{
"version_added": "13",
"partial_implementation": true,
"notes": [
"Until Edge 14 (build 14357), attempting to download data URIs caused Edge to crash (<a href='https://developer.microsoft.com/microsoft-edge/platform/issues/7160092/'>bug 7160092</a>).",
"Edge 17 or older didn't follow the attributes' value to determine filename (<a href='https://developer.microsoft.com/microsoft-edge/platform/issues/7260192/'>bug 7260192</a>)."
]

The reasoning probably is that these notes are not relevant for Chrome 132, where the feature is fully supported.

Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielhjacobs
Copy link
Contributor Author

If feels weird to mark the implementation partial in Chrome 131. In a way, this was more functional before Chrome 131 than in Chrome 131. In Chrome 131, this crashed the browser. Before Chrome 131, it was merely ignored.

@danielhjacobs
Copy link
Contributor Author

If possible, I'd mark the implementation unsupported until Chrome 131 and differently unsupported from Chrome 131 until Chrome 132, but I don't think yari supports that.

@caugner
Copy link
Contributor

caugner commented Nov 15, 2024

If feels weird to mark the implementation partial in Chrome 131.

There is no perfect solution, but it's definitely also weird to have the note on the current support statement, as if the caveat in Chrome 131 was relevant for Chrome 132+. @ddbeck has proposed to allow notes with version ranges, which could help here.

I really think it's fine to add the partial implementation entry for Chrome 131. In MDN's BCD table, you wouldn't notice this version, unless you actively expand the history.

@caugner caugner added the meeting agenda 🏢 Issues or pull requests in need of discussion in a project meeting. label Nov 18, 2024
@caugner
Copy link
Contributor

caugner commented Nov 18, 2024

Added to tomorrow's BCD call agenda.

@ddbeck
Copy link
Collaborator

ddbeck commented Nov 19, 2024

I think the issue here is that the notes are vague and we're missing data for the webkitdirectory HTML attribute, which is where some of this information should live.

The primary problem that the webkitdirectory HTML attribute is untracked. We're missing a feature, html.elements.input.webkitdirectory, that captures support for the actual attribute. It's really hard to tell what support for attribute reflection should be without knowing the support status of the underlying HTML attribute.

Before Chrome 131, the property could be set, but had no effect.

The failure mode here is not explained. I think there are three plausible readings:

  1. "webkitdirectory" in HTMLInputElment.prototype is true, but someInputElement.webkitdirectory = "" doesn't set the attribute (i.e., after someInputElement.webkitdirectory = "", someInputElement.webkitdirectory is still undefined).
  2. "webkitdirectory" in HTMLInputElment.prototype is true, setting the attribute succeeds, but the attribute has no behavior (i.e., it's equivalent to <input type="file" fake-bool-attr>).
  3. "webkitdirectory" in HTMLInputElment.prototype is false. That is, reflection of this attribute is not supported.

In the first two cases, you'd have a partial_implementation, but you'd have to write rather different notes. In the last case, it'd be a noteless "version_added": false entry.

In Chrome 131, the property could be set, but choosing a directory crashed the browser.

This sounds like the attribute reflection works just fine. I'd much rather have a note (and partial implementation) on the hypothetical html.elements.input.webkitdirectory feature that says, "In Chrome 131, choosing a directory crashes the browser." api.HTMLInputElement.webkitdirectory could mention this fact too, but it would have no partial_implementation.

One last note: I assumed that 131 still crashes, not that 131 shipped and was hotfixed to fix this crash—otherwise some very different notes and data apply. Writing the notes in the present tense (i.e., "In Chrome for Android 131, choosing a directory crashes the browser.") helps avoid this kind of ambiguity.

@caugner
Copy link
Contributor

caugner commented Nov 19, 2024

The primary problem that the webkitdirectory HTML attribute is untracked. We're missing a feature, html.elements.input.webkitdirectory, that captures support for the actual attribute.

@danielhjacobs Would you be up for adding that feature under html.elements.input here?

@danielhjacobs
Copy link
Contributor Author

"webkitdirectory" in HTMLInputElment.prototype is true, setting the attribute succeeds, but the attribute has no behavior (i.e., it's equivalent to <input type="file" fake-bool-attr>).

This is the one that applies here.

@danielhjacobs
Copy link
Contributor Author

Additionally, 131 does still crash

@danielhjacobs
Copy link
Contributor Author

In #24543 (comment), @caugner suggested "version_added": false, so I guess we're going back on that.

@github-actions github-actions bot added the size:s [PR only] 7-24 LoC changed label Nov 20, 2024
@danielhjacobs
Copy link
Contributor Author

danielhjacobs commented Nov 20, 2024

The primary problem that the webkitdirectory HTML attribute is untracked. We're missing a feature, html.elements.input.webkitdirectory, that captures support for the actual attribute. It's really hard to tell what support for attribute reflection should be without knowing the support status of the underlying HTML attribute.

Isn't it more accurate to track that with https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file#webkitdirectory?

@danielhjacobs
Copy link
Contributor Author

Something like this for input type file - webkitdirectory?

"insecure_login_handling": {
"__compat": {
"description": "Special handling of password inputs in insecure login pages",
"support": {
"chrome": {
"version_added": false
},
"chrome_android": "mirror",
"edge": "mirror",
"firefox": {
"version_added": "52"
},
"firefox_android": "mirror",
"ie": {
"version_added": false
},
"oculus": "mirror",
"opera": "mirror",
"opera_android": "mirror",
"safari": {
"version_added": false
},
"safari_ios": "mirror",
"samsunginternet_android": "mirror",
"webview_android": "mirror",
"webview_ios": "mirror"
},
"status": {
"experimental": false,
"standard_track": false,
"deprecated": false
}
}
}

@caugner
Copy link
Contributor

caugner commented Nov 21, 2024

Unless I misunderstood something, the bottom line seems to be that we need three support statements here (on api.HTMLInputElement.webkitdirectory):

  1. Supported in 132+
  2. Partially supported in 131 (mention crash)
  3. Partially supported in 18-130 (setting the attribute succeeds, but the attribute has no behavior).

Isn't it more accurate to track that with https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file#webkitdirectory?

I think it makes more sense to keep it next to the other input attributes (like capture, which also only applies to the file type), i.e. html.elements.input.webkitdirectory.

@danielhjacobs
Copy link
Contributor Author

Unless I misunderstood something, the bottom line seems to be that we need three support statements here (on api.HTMLInputElement.webkitdirectory)

But didn't @ddbeck say:

I'd much rather have a note (and partial implementation) on the hypothetical html.elements.input.webkitdirectory feature that says, "In Chrome 131, choosing a directory crashes the browser." api.HTMLInputElement.webkitdirectory could mention this fact too, but it would have no partial_implementation.

@github-actions github-actions bot added data:html 📄 Compat data for HTML elements. https://developer.mozilla.org/docs/Web/HTML size:m [PR only] 25-100 LoC changed and removed size:s [PR only] 7-24 LoC changed labels Nov 21, 2024
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one nit.

It would be good if @ddbeck could take a quick look as well.

image

html/elements/input.json Outdated Show resolved Hide resolved
@caugner caugner requested a review from ddbeck November 21, 2024 21:49
Co-authored-by: Claas Augner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API data:html 📄 Compat data for HTML elements. https://developer.mozilla.org/docs/Web/HTML meeting agenda 🏢 Issues or pull requests in need of discussion in a project meeting. size:m [PR only] 25-100 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

api.HTMLInputElement.webkitdirectory - Supported on latest Chrome (technically)
3 participants