-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Correct Safari data for HTML Element APIs #7402
Correct Safari data for HTML Element APIs #7402
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch seems to have helped surface some instances of some ghost features in our data that should just be completely removed. I now feel like we should be looking into how we could somehow automate identifying other ghost features like this ones this patch has exposed. It seems like y’all might have the means to do that with mdn-bcd-collector stuff?
@@ -268,10 +268,10 @@ | |||
"version_added": true | |||
}, | |||
"safari": { | |||
"version_added": true | |||
"version_added": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I realize this patch is scoped to just updating the Safari data — but as far as I can see, all the data we have for this feature is wrong. There’s no HTMLAnchorElement.prototype.media
in Blink or Gecko either — which is unsurprising since https://html.spec.whatwg.org/multipage/text-level-semantics.html#htmlanchorelement doesn’t actually define a media
member for HTMLAnchorElement
, not even as an obsolete member in https://html.spec.whatwg.org/multipage/obsolete.html#HTMLAnchorElement-partial. And there is actually no https://developer.mozilla.org/docs/Web/API/HTMLAnchorElement/media article.
So it seems to me we should just remove this entire feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that this should be removed. I think it's worth applying this change first, however, since (eventually) the feature would then show up in a listing of features not supported anywhere. Per our data it's still in Edge and Firefox, and we should check that before removal.
@vinyldarkscratch can you split this out into a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm: by splitting out, are you talking about this change, or removing the feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean doing the removal separately. Setting this to false here is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, thanks for the clarification! Definitely, I'll get a PR for that.
@@ -220,10 +220,10 @@ | |||
"version_added": true | |||
}, | |||
"safari": { | |||
"version_added": true | |||
"version_added": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There’s no HTMLAreaElement.prototype.hreflang
in Blink or Gecko either — and https://html.spec.whatwg.org/multipage/image-maps.html#htmlareaelement doesn’t actually define a hreflang
member for HTMLAreaElement
, not even as an obsolete member in https://html.spec.whatwg.org/multipage/obsolete.html#HTMLAreaElement-partiall. And there is actually no https://developer.mozilla.org/en-US/docs/Web/API/HTMLAreaElement/hreflang article.
So it seems to me we should just remove this entire feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The data still says Edge and Firefox support this. Not sure if that's right, though.
This is a capitalization typo: mdn/browser-compat-data#7402 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed the cases where claimed support is being reduced only so far.
* Remove formEncType custom IDL This is a capitalization typo: mdn/browser-compat-data#7402 (comment) * Remove lowSrc custom IDL
I've reviewed again. The changes to api.HTMLImageElement.lowSrc and api.HTMLInputElement.formEncType were not correctly reverted, resulting in a null→true change remaining. I'll set them back to null and merge this PR. |
…Element.formEncType
This was implemented without a flag, and with the reflected IDL attribute, in WebKit 532.6: WebKit/WebKit@c205c4e https://github.com/WebKit/WebKit/blob/c205c4e7d40da38c5009df548c2316a358a4fca7/WebCore/Configurations/Version.xcconfig That maps to Chrome 5 + Safari 5 + iOS 4, which also matches exactly earlier updates based on collector test results: mdn#7243 mdn#7290 mdn#7402 The handled attribute tokens were "allow-same-origin", "allow-forms", and "allow-scripts", so update that data to match.
This was implemented without a flag, and with the reflected IDL attribute, in WebKit 532.6: WebKit/WebKit@c205c4e https://github.com/WebKit/WebKit/blob/c205c4e7d40da38c5009df548c2316a358a4fca7/WebCore/Configurations/Version.xcconfig That maps to Chrome 5 + Safari 5 + iOS 4, which also matches exactly earlier updates based on collector test results: mdn#7243 mdn#7290 mdn#7402 The handled attribute tokens were "allow-same-origin", "allow-forms", and "allow-scripts", so update that data to match. The "allow-popups" entry was bumped to pass the lint.
This was implemented without a flag, and with the reflected IDL attribute, in WebKit 532.6: WebKit/WebKit@c205c4e https://github.com/WebKit/WebKit/blob/c205c4e7d40da38c5009df548c2316a358a4fca7/WebCore/Configurations/Version.xcconfig That maps to Chrome 5 + Safari 5 + iOS 4, which also matches exactly earlier updates based on collector test results: #7243 #7290 #7402 The handled attribute tokens were "allow-same-origin", "allow-forms", and "allow-scripts", so update that data to match. The "allow-popups" entry was bumped to pass the lint.
This PR uses Safari (4-14) and Safari iOS (3-14) results from the mdn-bcd-collector project to correct the data for the HTML element APIs. All results were double-checked for accuracy.