-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fix WebKit versions in Safari #4470
Conversation
@ddbeck This throws a wrench into a lot of the PRs that I've submitted for Lena Horne. I'm going to be looking over them carefully, both open and merged, and make changes as needed. I would love your review and opinion on this one! |
OK, I'm confused and I don't understand the change proposed here. Can you explain this in a bit more detail? Chiefly, I'm not sure how the diff relates to the links or the screenshots and I'm not sure where to start looking to understand. |
Sorry for the confusion, I tend to have a hard time explaining my thoughts properly, especially when panicking even the slightest (one of the reasons I’m not a teacher). I've completely reworded my first comment and included some additional screenshots to help explain better! One of the reasons why this is critical information is because in my work for the 2019 KR, I’ve been using the WebKit versions when each commit was introduced to identify which Safari version was the first to support a CSS feature. Thus, since of the WebKit versions were incorrect for Safari 10+, my changes may have been listing support in too early of browser versions. For others who may go through a similar process, this would affect them in the same manner. |
Yes, that does help! A few more questions:
|
Oh, wait, sorry, I misread the diff. The second question is already answered |
All good, glad your second question was answered! (Actually in researching this, I found that iOS doesn’t particularly have a 10.1 or 10.2 release, nor an 11.1 release. I plan on fixing that up in a separate PR later on. :P) Regarding the extended version, I’ve noticed that CSS properties have been added in between WebKit versions as small as that. I figured including the extended version would assist in any changes to Safari data and the review process! Happy to remove the last parts if they’re undesired. 😉 |
OK, great. It looks like these are the features that changed in the past three weeks which touch any of the versions affected by your diff (I didn't break this down by browser, though I could, if you like). In other words, these are the features which had an affected version three weeks ago and don't now, or did not have an affected version three weeks ago and do now:
|
Oh I've made a mistake, the previous list is for Desktop only. I've put complete lists in this gist: https://gist.github.com/ddbeck/e371d8e6e571be5a915a83c7e0957359 |
Good news: out of all the merged PRs, only two entries for the |
Excellent, thank you @vinyldarkscratch! |
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.
🎉
* Set Chromium/Safari versions for various CSS selectors * Fix Safari version based upon #4470
* Set Chromium/Safari versions for various CSS selectors (part 2) * Fix Opera Android versions * Fix Safari versions based upon #4470
* Set Chromium/Safari versions for misc. CSS properties * Fix Chrome/Opera Android versions * Fix Safari versions based upon #4470
Sorry for the necropost, but I figured out the root cause of the user agent inaccuracy. Apparently the WebKit developers stopped bumping the WebKit version intentionally, both to discourage user agent sniffing and to make fingerprinting more difficult. Source: https://bugs.webkit.org/show_bug.cgi?id=180365 (archive; the original was down when I looked this up) |
Unfortunately, it looks like Safari's user agent string was lying to us regarding the Safari versions -- which I had a hunch about when originally adding browser engines. After further review, I compared the release tags with the Safari releases, and found that...well, good thing we caught it sooner than later, right?
By looking in the SVN repository for WebKit and comparing the tags (aka each WebKit release) to the releases (aka each Safari release) by their ChangeLog files, I have determined that the WebKit version outputted by the user agent string outputted by Safari has been incorrect for Safari 10+ (and possibly Safari 9, though I’m unable to confirm this as the repo doesn't have release tags for Safari 9.X):
In the About panel, I’ve found that the numbers next to the Safari version contain the minor version of macOS (AKA "10.XX"), followed by WebKit version:
The screenshot below shows that the ChangeLog file for Safari 12.1 exactly matches that for WebKit 607.2.6.1.1, even though the user agent claims it’s running WebKit 605.1.15:
Boo to bad UA strings...☹️