-
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
Update compat data for CSS resolution media query and types #4226
Conversation
Here is a list of Chrome's unit values: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/cssom/css_unit_values.idl |
@jpmedley Doesn't seem to reflect Chrome's support of e.g. In my tests the |
I changed the structure of the |
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.
Hi @fvsch and welcome to BCD! Thank you for starting this PR, there's some good improvements in here, though there are a few structural issues to work through before we merge. Some comments inline and some responses to your PR description below.
First, CSS types are a weird thing. The way we structure type data is useful to consumers who need data for types that are used in many properties or at-rules, but less so for one-offs, as <resolution>
appears to be (AFAICT, it's only used for the resolution
media feature). I sympathize with your confusion.
But what does supporting a CSS type mean? Does it mean that the
resolution
media query works (nope, not in Safari)? Does that mean that all the units for the<resolution>
type are supported (nope, not in most browsers)? Does that mean that at least one unit is supported (nope, not in Safari)?
Generally, it's the last of your examples: one or more of the units are supported (in one or more places where you'd expect that unit to be supported, according to a spec). In this case, it should mean some <resolution>
type value is supported in the browser somewhere. Usually this isn't that weird (consider <length>
type values and their units appearing to be supported with many properties at once). Sadly, this is a weird case.
So I opted to rename the
<resolution>
label todpi and dpcm units
, because in practice they're the first resolution types supported in browsers (both in IE9) and it seems they landed together in all browsers.
You have the right idea in spelling out dpi
and dpcm
data specifically, but structurally we still need the parent <resolution>
feature (mainly for consistency with other types). I've described this in more detail in line comments.
The material version data you've updated here is great though. I really appreciate the work you've done there already. Once we tidy this up, this will be an excellent improvement to the data. Thank you again!
Hi @fvsch, do you have any plans to revisit this PR? |
@ddbeck Definitely! I’ll probably be too busy this week, but next week is looking good. |
Hey again @fvsch, are you planning on returning to this PR? |
Quick question, does it make sense to remove details about very very old (and unsupported) releases of evergreen browsers, such as:
could be just:
|
This has been something that's been brought up around this repository with some mixed feelings towards it. However, one of the goals of BCD is to provide the most accurate information possible. Removing data regarding older browser versions works against that, and in my opinion, doesn't show just how old some properties really are. There are some cases where older data is plotted to be stripped out, such as with pre-Chromium WebView. Overall, I'd say, let's leave the data regarding older Firefox versions? |
So I rewrote this PR based on the work done in #4369 What I'm trying to do now:
The upside of marking Safari as non-supporting is that it reduces developer confusion: less risk of using say The downside is that after this update, https://developer.mozilla.org/en-US/docs/Web/CSS/@media/resolution and https://developer.mozilla.org/en-US/docs/Web/CSS/resolution would suggest that Safari doesn't support resolution matching in CSS at all. My proposed fix would be to add notes to both those pages linking to https://developer.mozilla.org/en-US/docs/Web/CSS/@media/-webkit-device-pixel-ratio |
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 for your patience while I got back to this. A couple minor points to address here, but this is a really solid clean up. Thank you!
css/at-rules/media.json
Outdated
@@ -1299,16 +1299,18 @@ | |||
"version_added": "9" | |||
}, | |||
"opera": { | |||
"version_added": true | |||
"version_added": "9.5" |
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.
Given the updated version for Chrome, I suspect there was a discontinuity on this feature in Opera:
- Supported from 9.5 and removed in 15 (Blink 28)
- Added in 16 (Blink 29)
(Opera for Android would be a similar story, except the change over happened at 14, not 15.)
See also: https://github.com/mdn/browser-compat-data/blob/master/browsers/opera.json
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.
Are there examples of how to encode this information that I could follow?
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.
Sure! You can use an array like this:
browser-compat-data/api/NavigatorGeolocation.json
Lines 25 to 33 in f53ea02
"opera": [ | |
{ | |
"version_added": "16" | |
}, | |
{ | |
"version_added": "10", | |
"version_removed": "15" | |
} | |
], |
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.
@fvsch are you planning to follow up on this?
css/types/resolution.json
Outdated
"version_added": "9" | ||
}, | ||
"opera": { | ||
"version_added": "9.5" |
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.
Discontinuity, as mentioned above
css/types/resolution.json
Outdated
"version_added": "9" | ||
}, | ||
"opera": { | ||
"version_added": "9.5" |
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.
Discontinuity, as mentioned above
@ddbeck I've updated this to take the Opera discontinuity into account. Is this good to go now? |
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 Thanks for pushing this over the line! |
Fixes #4224
Changes:
resolution
media query. Adds information about Safari support (not supported, but adds info about-webkit-device-pixel-ratio
).Related MDN page: https://developer.mozilla.org/en-US/docs/Web/CSS/@media/resolution
<resolution>
type.Related MDN page: https://developer.mozilla.org/en-US/docs/Web/CSS/resolution
<resolution>
type. But what does supporting a CSS type mean? Does it mean that theresolution
media query works (nope, not in Safari)? Does that mean that all the units for the<resolution>
type are supported (nope, not in most browsers)? Does that mean that at least one unit is supported (nope, not in Safari)?<resolution>
label todpi and dpcm units
, because in practice they're the first resolution types supported in browsers (both in IE9) and it seems they landed together in all browsers.<resolution>
type, especially for thedppx
unit, for thex
alias, and for Edge and Android browsers (after running a bunch of tests on BrowserStack; I didn't identify specific versions fordppx
support in Opera and Samsung Internet).Test case (doesn't inclue
-webkit-device-pixel-ratio
):https://fvsch.github.io/test-pages/css-mq-resolution
Linting: passed locally.