-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Changes from 2 commits
663c2f6
8634e30
d3a3821
f66d061
9776d37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. There’s no 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 commentThe 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. |
||
}, | ||
"safari_ios": { | ||
"version_added": true | ||
"version_added": false | ||
}, | ||
"samsunginternet_android": { | ||
"version_added": true | ||
|
@@ -268,10 +268,10 @@ | |
"version_added": true | ||
}, | ||
"safari": { | ||
"version_added": true | ||
"version_added": false | ||
}, | ||
"safari_ios": { | ||
"version_added": true | ||
"version_added": false | ||
}, | ||
"samsunginternet_android": { | ||
"version_added": true | ||
|
@@ -604,10 +604,10 @@ | |
"version_added": true | ||
}, | ||
"safari": { | ||
"version_added": true | ||
"version_added": false | ||
}, | ||
"safari_ios": { | ||
"version_added": true | ||
"version_added": false | ||
}, | ||
"samsunginternet_android": { | ||
"version_added": true | ||
|
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 amedia
member forHTMLAnchorElement
, 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.