-
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
Browser compat for <html>, <base>, <head>, <link>, <style>, <title>, and <meta> elements #279
Conversation
30812c0
to
ff83b82
Compare
8534a07
to
83b4aab
Compare
I've rebased the PR (it was needed because of PR #276) |
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.
A few things for you to fix.
I'm still thinking about the general structure of this. Doing a few experiments with the macro to see what's best.
html/elements/base.json
Outdated
}, | ||
"ie": { | ||
"version_added": true, | ||
"notes": "Before Internet Explorer 7, <base> could be positioned anywhere in the document and the nearest value of <base> was used." |
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 needs to be <code><base></code>
(2 times).
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.
Fixed (and put this in the present tense). Will be in the next rebased PR.
compat-data-schema.md
Outdated
@@ -11,7 +11,7 @@ JSON file containing the compatibility data. | |||
|
|||
- [api/](https://github.com/mdn/browser-compat-data/tree/master/api) contains data for each [Web API](https://developer.mozilla.org/en-US/docs/Web/API) interface. | |||
|
|||
- [css/](https://github.com/mdn/browser-compat-data/tree/master/css) contains data for [CSS](https://developer.mozilla.org/en-US/docs/Web/CSS) properties, selectors and at-rules. | |||
- [html/](https://github.com/mdn/browser-compat-data/tree/master/html) contains data for [HTML](https://developer.mozilla.org/en-US/docs/Web/HTML) elements, attributes and global attributes. |
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.
Don't replace the CSS sentence, add the HTML one.
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.
Fixed. (Once I upgrade the PR of course)
html/elements/base.json
Outdated
} | ||
}, | ||
"relative_url": { | ||
"desc": "Support of relative URIs.", |
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.
"Support of" is unnecessary. Let's keep this concise: "Relative URIs" and the key could be "relative_uris", too.
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.
Fixed. Will be in the next rebased PR.
html/elements/link.json
Outdated
"deprecated": false | ||
} | ||
}, | ||
"alternate stylesheet": { |
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.
"alternate_stylesheets"
as a key and desc: "alternate stylesheets"
? Unsure if I like space in feature keys.
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.
Fixed. Will be in the next rebased PR. I put desc: "Alternative stylesheets."
html/elements/link.json
Outdated
}, | ||
"firefox": { | ||
"version_added": "39", | ||
"notes": "Before Firefox 41, it didn't obey the crossorigin attribute." |
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.
Lets put crossorigin in <code>
and use present tense.
"notes": "Prior to Firefox 41, the <code>crossorigin</code> attribute isn't obeyed."
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.
Fixed. (Once I upgrade the PR of course)
html/elements/meta.json
Outdated
"deprecated": false | ||
} | ||
}, | ||
"name=referer": { |
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.
Shouldn't this be a sub feature of name
?
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.
It already was (and is).
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.
@Elchi3 Maybe you want me to rename it in "referer" or "referer value"…
html/elements/style.json
Outdated
"version_removed": "35.0", | ||
"flag": { | ||
"type": "preference", | ||
"name": "Enable <style scoped>", |
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.
"name": "Enable <style scoped>"
(we probably need to fix the KS macro to html escape everything).
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.
Fixed. Will be in the next rebased PR.
Note that you can't automate escaping as <code> and <a> are ambiguous.
Besides the comments above, I think there is agreement that your original structure (structure 1 in issue #283) is the one we should use. |
@Elchi3 I've updated the PR with all the changes requested. Ready for a new review. |
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.
Thanks, lets take this in and I will try to convert it to the new format using my conversion script. It will be another good set for testing. But please be on hold for more migration for now, though. It is enough HTML test data for the moment.
Inconsistency pointed out by @gsnedders here: mdn#18368 Most of this is originally from wiki migration: mdn#279 The Chromium implementation was in M34: https://chromium.googlesource.com/chromium/src/+/21e65a63bf8e3947ff4b4d34cb73866c413106e2 https://www.chromium.org/developers/calendar/ There was no use of "crossorigin" in HTMLLinkElement.cpp before this point, and https://crbug.com/178787 doesn't say anything to suggest there was partial support, so assume the old data was wrong. For Edge and Safari, just trust the data for HTMLLinkElement.
* Update <link crossorigin> data to match API Inconsistency pointed out by @gsnedders here: #18368 Most of this is originally from wiki migration: #279 The Chromium implementation was in M34: https://chromium.googlesource.com/chromium/src/+/21e65a63bf8e3947ff4b4d34cb73866c413106e2 https://www.chromium.org/developers/calendar/ There was no use of "crossorigin" in HTMLLinkElement.cpp before this point, and https://crbug.com/178787 doesn't say anything to suggest there was partial support, so assume the old data was wrong. For Edge and Safari, just trust the data for HTMLLinkElement. * Update link.json * Update html/elements/link.json Co-authored-by: Florian Scholz <fs@florianscholz.com>
Added compat data for the element (first html element with compat data).
A few points: