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

Update Safari support for DOMTokenList API #4628

Merged
merged 8 commits into from
Aug 20, 2019
Merged

Update Safari support for DOMTokenList API #4628

merged 8 commits into from
Aug 20, 2019

Conversation

Added info for DOMTokenList methods and properties for Safari Desktop and Mobile.
https://developer.apple.com/documentation/webkitjs/domtokenlist
Tested on Mobile Safari 12.1.2
@queengooborg queengooborg added the data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Aug 16, 2019
@queengooborg
Copy link
Contributor

Thanks for your PR! It looks like you have some linter errors with some of the version numbers. In BCD, we use the most simplistic semantic versioning (AKA “5.0” would be “5”). Can you update your PR accordingly?

I’ll also be reviewing the changes themselves soon, so I may give an official review too. 😉

@queengooborg queengooborg self-requested a review August 16, 2019 17:52
@mcmimik
Copy link
Contributor Author

mcmimik commented Aug 16, 2019

Done 👌🏻

@queengooborg
Copy link
Contributor

queengooborg commented Aug 18, 2019

Whoops, I told you a slight fib about the version numbers, haha -- Samsung Internet doesn't follow what I told you. Sorry about that! I went ahead and fixed that up real quick. 😛

I also apologize for the delay on my review. I've found that the SDK versions within the Apple Developer documentation are not the most trustworthy source for Safari versions -- it mentions DOMTokenList is for Safari Desktop 10.0+, yet I've found support in Safari 8.0.

Copy link
Contributor

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

Alright, so I figured out the story behind DOMTokenList, unfortunately my assumption was correct: Apple's documentation doesn't give us the correct version numbers. I've gone through the API using both SauceLabs and BrowserStack to determine when the API and its members was actually supported. Here's my findings:

DOMTokenList - Safari 6
.replace - Safari 10.1
.supports - Safari 10.1
.length - Safari 6
.value - Safari 10
.item - Safari 6
.contains - Safari 6
.add - Safari 6
.remove - Safari 6
.toggle - Safari 6
.entries - Safari 10.1
.forEach - Safari 10.1
.keys - Safari 10.1
.values - Safari 10.1

May we update the PR accordingly? Thank you for your work on this PR, by the way!

@mcmimik
Copy link
Contributor Author

mcmimik commented Aug 18, 2019

Wow, how much insights. Well, I decided to double-check everything as well, here are my results: DOMTokenList support was added in iOS Safari 5.1 (userAgent: Mozilla/5.0 (iPad; CPU OS 5_0 like Mac OS X) AppleWebKit/534.46 (KHTML, like Gecko) Version/5.1 Mobile/9A334 Safari/7534.48.3). I've updated also other values, pls, check it up.

iOS Safari 10: Mozilla/5.0 (iPhone; CPU iPhone OS 10_3_3 like Mac OS X) AppleWebKit/603.3.8 (KHTML, like Gecko) Version/10.0 Mobile/14G60 Safari/602.1
iOS Safari 12: Mozilla/5.0 (iPhone; CPU iPhone OS 12_1_4 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0 Mobile/15E148 Safari/604.1
Safari Desktop 5.1: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/534.57.7 (KHTML, like Gecko) Version/5.1.5 Safari/534.55.3
Safari Desktop 6.2: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/600.8.9 (KHTML, like Gecko) Version/6.2.8 Safari/537.85.17
Safari Desktop 10: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/603.3.8 (KHTML, like Gecko) Version/10.1.2 Safari/603.3.8
Safari Desktop 12: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_5) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.1.1 Safari/605.1.15

@mcmimik
Copy link
Contributor Author

mcmimik commented Aug 18, 2019

Mmm, there is no 6.2 version for some reason, but it really was. Maybe we can add it? Or we can suppose force argument was added in 6.1...
10.0 fixed.

Copy link
Contributor

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

Thanks for researching this even more! I'd say that we should set it to 6.1, since the WebKit versions of both are so similar. I don't have access to Safari 5.1 myself, so I'll take your word for it. 😉

(Something that may help reduce confusion: I've found that Safari's user agent string does not specify the correct WebKit version. I've been able to identify what they actually are by comparing their source code within the WebKit repo -- feel free to review browsers/safari.json to see the real values!)

api/DOMTokenList.json Show resolved Hide resolved
api/DOMTokenList.json Outdated Show resolved Hide resolved
api/DOMTokenList.json Outdated Show resolved Hide resolved
@mcmimik
Copy link
Contributor Author

mcmimik commented Aug 19, 2019

I have an access to Safari 5.1 thanks to BrowserStack :)

I've been able to identify what they actually are by comparing their source code within the WebKit repo

Oh well. It's intriguing. I've found that replace method was added in Webkit 603.1.2. Btw iOS versions in this repo are based on "Safari Engine wrapper" version (Safari/602.1), not the original Webkit version (AppleWebKit/603.3.8).

The real problem is that there were a few versions (sic) of iOS Safari version "10.0". Thus, iOS 10.3 beta with the build number 14E5230e has 603.1.20 Webkit's version, look at this user agent:
Mozilla/5.0 (iPhone; CPU iPhone OS 10_3 like Mac OS X) AppleWebKit/603.1.20 (KHTML, like Gecko) Version/10.0 Mobile/14E5230e Safari/602.1
iOS 10.2.1 comes with the Safari based on Webkit 602.4.6:
Mozilla/5.0 (iPhone; CPU iPhone OS 10_2_1 like Mac OS X) AppleWebKit/602.4.6 (KHTML, like Gecko) Version/10.0 Mobile/14D27 Safari/602.1

It seems like Apple just forgot to update "Version/10.0" number 🙂. AFAIK we have no possibility to write some note about "Safari 10.0 starting with iOS 10.3", so I can change iOS for replace method to 10.1.

@queengooborg queengooborg self-requested a review August 19, 2019 18:38
@mcmimik
Copy link
Contributor Author

mcmimik commented Aug 20, 2019

There's more. I noticed 10.1 and 10.2 versions of iOS Safari in browsers/safari_ios.json. But in fact, there were only iOS updates with these numbers, not Safari. Webkit.org says about new version of Safari without iOS version number's mention:

A new version of Safari shipped with the release of iOS 10.3 and macOS Sierra 10.12.4. Safari on iOS 10.3 and Safari 10.1 on macOS adds many important web features and improvements from WebKit that we are incredibly excited about.

We can find 10.3(.2,.3) iOS Safari builds in Webkit sources, it's strange, but there are no 10.1 and 10.2 versions there.

Now I'm only confused by the iOS Safari's version number in MDN. Is it the number from user agent string (Version/10.0)? Or is it the iOS build's number (10.3.4)?

The first one means we should specify "11", because the next version update (after "10") in the user agent string was only in iOS 11. Even iOS 10.3.4 has "Version/10.0":
Mozilla/5.0 (iPad; CPU OS 10_3_4 like Mac OS X) AppleWebKit/603.3.8 (KHTML, like Gecko) Version/10.0 Mobile/14G61 Safari/602.1.

The second one means we should set the "10.3" version, because iOS 10.3 has the most relevant Webkit engine version 603.1.30 with the replace method, as I wrote before. Oh, and this is confirmed by Webkit sources, the method appears for the first time in 10.3 (find "replace" on the page and compare with 10.0). The same situation with the Mac Safari 10.1's sources.

@queengooborg
Copy link
Contributor

queengooborg commented Aug 20, 2019

Now I'm only confused by the iOS Safari's version number in MDN. Is it the number from user agent string (Version/10.0)? Or is it the iOS build's number (10.3.4)?

For Safari iOS, we are using the iOS version as requested by Apple themselves, hence why I said that Safari iOS should be "10.3" when Safari Desktop is 10.1. Regarding the user agent strings...well, to put it into simple terms: they're flat out wrong. Not only do they state the wrong Safari versions, but they even announce incorrect WebKit versions. I came across this discovery while adding the engine versions to the schema -- we thought that user agents were the perfect historical data, and normally they would be, but not in Safari's case... ☹️

I noticed 10.1 and 10.2 versions of iOS Safari in browsers/safari_ios.json [...] We can find 10.3(.2,.3) iOS Safari builds in Webkit sources, it's strange, but there are no 10.1 and 10.2 versions there.

Since we've only really cared about the major and minor releases of browsers, and iOS 10.1/10.2 were based off of Safari 10.0.x anyways, I feel that we should probably get rid of them at this point. (There's a lot of work described in #2006 for all of this.)

The second one means we should set the "10.3" version

Yes, we should set line 82 to "10.3" as iOS 10.3 released with Safari 10.1. 😉 (I was trying to hint at that in my review comment, but re-reading it, I realize that I didn't do a great job at it. 😝)

Copy link
Contributor

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

So yeah, we just need to set Safari iOS to 10.3 in line 82 and then we're good to go!

api/DOMTokenList.json Outdated Show resolved Hide resolved
Co-Authored-By: Queen Vinyl Darkscratch <vinyldarkscratch@gmail.com>
@mcmimik
Copy link
Contributor Author

mcmimik commented Aug 20, 2019

Well, we've done a good job 👍 I'm only confused with "Changes requested" label on this page, should I do something else for now?

Copy link
Contributor

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

No need to worry about that "changes requested" flag -- that's just because I've formally requested changes and have not submitted a new review just yet. But now that I'm here, I shall change that and submit formal approval instead!

Thanks for all your hard work on this PR. Let's get this merged in right away! 👍

@queengooborg queengooborg changed the title Updated DOMTokenList in Safari Update Safari support for DOMTokenList API Aug 20, 2019
@queengooborg queengooborg merged commit aaf67cf into mdn:master Aug 20, 2019
@mcmimik mcmimik deleted the patch-3 branch August 20, 2019 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants