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

Mirror Chrome -> WebView for css/* #4637

Merged
merged 3 commits into from
Aug 29, 2019

Conversation

queengooborg
Copy link
Contributor

@queengooborg queengooborg commented Aug 18, 2019

This PR mirrors the values of Chrome Desktop to WebView Android is true or null, to help reduce data inconsistencies and reach towards the 2019 Key Result. Notes have been adapted to ensure they are not mentioning Chrome versions earlier than 37.

The script that performed the majority of changes can be found here: https://github.com/vinyldarkscratch/bcd-toolkit/blob/master/fix.py

@queengooborg queengooborg added data:css Compat data for CSS features. https://developer.mozilla.org/docs/Web/CSS bulk_update An update to a mass amount of data, or scripts/linters related to such changes labels Aug 18, 2019
@ddbeck
Copy link
Collaborator

ddbeck commented Aug 19, 2019

I keep thinking we might want a generic and tested mirroring script so simple cases can be done through a perfunctory bulk process and then we could break out the special cases (notes, partials, etc.) that could get a real review. This seems like a likely candidate for that sort thing.

(Which reminds me I need to review your other bulk update scripts. Sorry for the delay!)

@queengooborg
Copy link
Contributor Author

I'm on board for that idea, yeah!

@queengooborg queengooborg changed the title Mirror Chrome to WebView for CSS Mirror Chrome -> WebView for css/* Aug 21, 2019
@queengooborg
Copy link
Contributor Author

I've just posted a link to the mirroring script I wrote (It's in Python, but I promise it doesn't bite!), which takes a browser as an argument and copies values from the applicable parent browser. I think we could easily make it a runnable JavaScript script called "mirror", and add it to this repository. The current three PRs that are open and perform this mirroring function, I feel should be merged ahead of time since they a) work towards the 2019 Key Result, and b) are blocking changes for our feature sorting script, but after that, perhaps perform said script on folders like API, JavaScript, etc.?

Copy link
Contributor

@jpmedley jpmedley left a comment

Choose a reason for hiding this comment

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

I found a few issues in a related UA for the items you changed. Let me know if you'd rather do a separate PR.

css/properties/box-decoration-break.json Show resolved Hide resolved
css/properties/border-top-right-radius.json Outdated Show resolved Hide resolved
css/properties/border-top-left-radius.json Outdated Show resolved Hide resolved
css/properties/border-bottom-right-radius.json Outdated Show resolved Hide resolved
@queengooborg
Copy link
Contributor Author

queengooborg commented Aug 23, 2019

Thanks for reviewing @jpmedley, however since this PR focuses on WebView-related changes, I think we should focus on Chrome Android changes in another PR/issue. (This is especially since this mirroring script is planned to be the next bulk update.) How does the WebView stuff look to you?

@queengooborg
Copy link
Contributor Author

Actually something I just realized is that the WebView mirroring comes from Chrome Android, so some of the Chrome Android stuff may need to be taken care of first. Does anything stand out as a potential issue with WebView data (i.e. anything that wouldn't be "37 or lower")?

@jpmedley
Copy link
Contributor

I made comments as per your request. I unresolved the issues because I wasn't sure if you would get notified otherwise.

@Elchi3
Copy link
Member

Elchi3 commented Aug 29, 2019

I've started reviewing this, but npm run traverse webview_android css returns 83 results. Is this only a partial mirroring?

@queengooborg
Copy link
Contributor Author

A few of them were due to the fact that WebView had no statements, but the other part was arrays. Two things I should probably revise in my fix script!

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

I manually went through this and couldn't find consistency errors. Thanks everyone!

@Elchi3 Elchi3 merged commit 8dae2ca into mdn:master Aug 29, 2019
@queengooborg queengooborg deleted the css/mirror/webview-android branch August 29, 2019 17:59
@cincodenada
Copy link

cincodenada commented Feb 16, 2021

I landed here tracking where this ≤37 came from on :last-child, and am not much clearer on what it means after reading this. Without some context I think ≤37 makes for a confusing end-user experience. It implies that WebView stopped supporting :last-child (for example) after version 37, which seems unlikely, so I expect something else is going on here. Maybe WebView switched to some other backend after version 37?

There's even one case where a feature is listed as both added and removed in ≤37, which I guess is possible, but it seems odd.

If there is something else going on her I would like to propose an improvement to make things clearer, but I can't figure out what is trying to be communicated by these changes in the first place. Can someone who is more familiar with the context here help explain what the intent here is? Sorry if I'm missing something obvious. For context, I'm a fairly experienced web developer who hasn't done much Android work and thus I am not specifically familiar with WebView or the state thereof.

@ddbeck
Copy link
Collaborator

ddbeck commented Feb 17, 2021

@cincodenada For more information about this notation, please see the schema documentation on Ranged versions. The summary is that ≤ values express uncertainty about the exact version a feature was added or removed.

In the cases you've called out, a version_added value of ≤37 means, "added to this browser in a version less than or equal to 37" (or put in other words, "added in version 37 but possibly earlier"). A version_removed value of ≤37 means, "removed in in a version less than or equal to 37" (or put in other words, "removed in version 37 but possibly earlier"). Thus, the pair:

"version_added": "≤37",
"version_removed": "≤37"

means, roughly, "supported for some releases between the first release and version 37, but the starting and ending values are unknown to us."

If you know with more certainty about when a particular feature was supported or removed from a browser—if you can help us eliminate this uncertainty—I'd welcome PRs or issues amending that data. Thank you!

@jpmedley
Copy link
Contributor

@cincodenada I work on Chrome at Google. There is some weirdness about WebView and it's relation to Chrome and Android. For starters, 37 was when WebView became a Chromium fork, which it was not before. I've seen enough incorrect PRs about those early versions that I encourage you to ping me before starting a PR. My email is in my GitHub profile.

@cincodenada
Copy link

cincodenada commented Feb 17, 2021

Okay, the Ranged Versions info is the piece I was missing here - I didn't understand that this was an expression of partial information/ambiguity about precisely what version. That makes sense and I see how it's useful to have more information.

Thanks for the lucid examples, @ddbeck, those are very helpful and they all make good sense to me now. I unfortunately don't have more details to add. @jpmedley thanks for the extra info on WebView details, that helps fill in the context as well.

With my better understanding, the browser-compat data itself makes plenty of sense, and I think is the best it can be given the information we have! I think the tooltip on MDN's docs in this situation could be clearer, and that's a different project - from some searching it looks like that's over at https://github.com/mdn/yari, so I'll see if it makes sense to open an PR over there.

I appreciate the quick and informative responses, and y'all taking the time to clarify the context here even though my question was tangential to this specific issue. Thanks for helping make the web a better place 🙂

foolip added a commit to foolip/browser-compat-data that referenced this pull request Mar 29, 2021
This is the result of bulk mirroring:
mdn#4637

However, the versions can be made more precise with fairly high
confidence. Assuming that Safari 4 is correct, that was based on WebKit
530.17, exactly the same version as WebView 2. Both were in 2009 and
shouldn't be off by much, even if there's a chance of off-by-one here.

The removal is straightforward: Chrome 36 maps to WebView 37.

Part of mdn#9610
ddbeck pushed a commit that referenced this pull request Mar 30, 2021
This is the result of bulk mirroring:
#4637

However, the versions can be made more precise with fairly high
confidence. Assuming that Safari 4 is correct, that was based on WebKit
530.17, exactly the same version as WebView 2. Both were in 2009 and
shouldn't be off by much, even if there's a chance of off-by-one here.

The removal is straightforward: Chrome 36 maps to WebView 37.

Part of #9610
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bulk_update An update to a mass amount of data, or scripts/linters related to such changes data:css Compat data for CSS features. https://developer.mozilla.org/docs/Web/CSS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants