Skip to content

Extract Safari version instead of WebKit version #1127

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

Closed
wants to merge 7 commits into from

Conversation

lukasIO
Copy link

@lukasIO lukasIO commented May 16, 2023

Description
This PR changes the version extraction for Safari to look at the actual Safari version instead of the WebKit version.
Minor versions (e.g. 10.3) get parsed as float in order to keep straight forward version comparison as before.
In case there would be a patch version defined in the ua string (e.g. 10.3.4) the patch version gets dropped.

The oldest Safari versions I could find still work with that approach:

  • macOS Safari 4.0 - Mozilla/5.0 (Macintosh; U; Intel Mac 05 X 10_6_8; en-us) ApplewebKit/531.22.7 (KHTML, like Gecko) Version/4.0.5 Safari/531.22.7
  • iOS Safari 7.0 - Mozilla/5.0 (iPhone; CPU iPhone OS 7_1 like Mac OS X) AppleWebKit/537.51.2 (KHTML, like Gecko) Version/7.0 Mobile/11D167 Safari/9537.53

Purpose
fixes #1082

Safari has stopped updating WebKit versions in the user agent string. E.g. Safari 16.3 still shows AppleWebKit/605.1.15.

@fippo
Copy link
Member

fippo commented Jun 15, 2023

🤔 this would be a breaking change (which means losing 30% of users, see #1065)
I don't think the safari/webkit version is important enough for that, I never gated features on it.

@fippo fippo added the breaking change breaking change that requires a major version bump label Jun 27, 2023
@lukasIO
Copy link
Author

lukasIO commented Jul 7, 2023

thanks for the review!

I never gated features on it

maybe I'm misunderstanding what you mean by that, but there's https://github.com/webrtcHacks/adapter/blob/main/src/js/common_shim.js#L356 and #1082 which details an uncaught (by adapter) bug related to the version parsing for Safari 12.

@lukasIO
Copy link
Author

lukasIO commented Apr 25, 2024

@fippo was not including this in 9.0.0 an oversight or are you not planning to merge this altogether?

@fippo
Copy link
Member

fippo commented Mar 3, 2025

I think the main concern here is still that even with a breaking change this would folks to update their code... which will reduce the adoption rate (not great already). #1160 is the next breaking change so this might ride along. But the alternative would be to expose the Safari version with a different name and update the adapter code?

Copy link

@uxmaster uxmaster left a comment

Choose a reason for hiding this comment

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

Ockham razor ;-)

@uxmaster
Copy link

uxmaster commented Mar 3, 2025

BTW @fippo, I think nobody uses the adapter's Safari version, since it is useless being always 605 (I parse it myself for Safari while for other browsers I lean on adapter). Thus, this is not so breaking change as it looks.

@fippo
Copy link
Member

fippo commented Mar 3, 2025

I know it is useless but changing such behavior is a breaking change in semver for better or worse.

@lukasIO
Copy link
Author

lukasIO commented Mar 5, 2025

I think the main concern here is still that even with a breaking change this would folks to update their code...

I don't see how that's true if the current behaviour is just broken/useless as you mention. No one using a adapter today can rely on the Safari version parsing, so I don't see why they would have to change their code 🤔

But the alternative would be to expose the Safari version with a different name and update the adapter code?

Totally your call, I think everything is better than it not being useful at all, but exposing a custom property for the safari version (not sure if you had something else in mind) sounds less obvious to me than fixing the version parsing

@dagingaa
Copy link
Collaborator

Sorry for taking so long on weighing in here. I'd like to avoid feature gating based on user agent parsing altogether, and I think adding the proper Safari version now without a clear benefit to adapter is a breaking change I'd like to avoid.

Not saying we won't need this in the future, but until then I'd like us to keep it as stable as possible given the intention of this project. The bug you mentioned is valid, but also has a clear workaround. If anything that bug should be fixed by feature testing and fallback rather than user agent parsing to begin with, but I might be able to be swayed on that.

In any case, I love the contribution, and think we should leave it open. As @fippo mentioned we might include this with the next breaking change, or as part of fixing the above-mentioned bug.

@lukasIO
Copy link
Author

lukasIO commented Mar 21, 2025

yeah, definitely agree that this should only be included in a major version update!

An initial intention was to use adapter's browser parsing which is mentioned in the project's README:

You might want to use adapters browser detection which detects which webrtc quirks are required. ... for webrtc engine detection (which will for example detect Opera or the Chromium based Edge as 'chrome') and adapter.browserDetails.version for the version according to the user-agent string.

and that's not very useful for Safari as is.

@dagingaa
Copy link
Collaborator

Yeah I think we need to remove that from the README, from when this project was created to now, a lot has happened to browser detection, user agent strings etc. to the point where it's much better to use a library dedicated to this, rather than relying on adapter.

Still we need to support the legacy for a while longer, and probably do a major release at some point that will remove exposing these fields and provide a good alternative route for people that rely on this today.

@uxmaster
Copy link

@dagingaa, it seems you concentrate on a global view, while I think this should be treated just as a fix for #1082. When you fix the issue reported almost 4 years ago, you may then wonder about possible refactoring. First and foremost, the lib should just work, not be politically correct (according to not-use-the-user-agent-string orthodoxes).

@dagingaa
Copy link
Collaborator

dagingaa commented Mar 24, 2025

not be politically correct (according to not-use-the-user-agent-string orthodoxes).

The user agent string is not a reliable method to get information anymore, and thus should not be used as such. Further, relying on adapter for this is backwards and will actually just hurt your application rather than help it when there are good alternatives.

Internally, adapter needs to use any means necessary to add workarounds, such as checking the user agent string. We do that today, and will continue to do that tomorrow. However, exposing that to end users is a mistake I personally think we should stop doing. Changing the detection code everywhere means additional risk which I think is unnecessary.

it seems you concentrate on a global view, while I think this should be treated just as a fix for #1082.

This PR is changing the global view, thus we need to care about it (for the aforementioned reasons). Fixing #1082 in isolation should be a much simpler fix, which we very much welcome, but introducing a major release breaking change is a huge ask which does need to be taken into very careful consideration.

In the meantime we are lucky there exists a workaround.

@fippo
Copy link
Member

fippo commented Mar 25, 2025

#1082 is an issue that was fixed in libWebRTC back in late 2018. Safari has been slow updating it but that means 2019 in Safari 13 still. The issue was marginal in 2021 already, in 2025 I doubt it is relevant? (and if not my condolences to the users on that browser)

Using adapter for UA detection was convenient in the early days (see the 2015 version here), in particular since one wanted the "webrtc version" which was the same in Chrome and Opera. I haven't seen much use of that in recent years (and even the samples use it sparingly). For the most part one can write decent feature detection (please do not create a peerconnection for that... well, if you do in Safari I don't mind)

Is relying on adapters version something you should do in 20232025? Probably not👎
If version detection is broken for Safari and that discourages usage of adapter for this purpose then 👍
Do it yourself (like here) 👍

fippo added a commit that referenced this pull request Apr 10, 2025
so nobody has to worry about bugs in very old safari versions

Fixes #1082 with inspiration from #1127
fippo added a commit that referenced this pull request Apr 10, 2025
so nobody has to worry about bugs in very old safari versions

Fixes #1082 with inspiration from #1127
@kor0000

This comment was marked as off-topic.

@fippo
Copy link
Member

fippo commented Apr 18, 2025

#1161 is now part of v9.0.2

@fippo fippo closed this Apr 18, 2025
fippo added a commit that referenced this pull request Apr 18, 2025
so nobody has to worry about bugs in very old safari versions

Fixes #1082 with inspiration from #1127

Cherry-picked from
  b6095e0
  b702d86
and adapted for the 8.x version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change breaking change that requires a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Safari <=12.0 media establishing does not work due to removeExtmapAllowMixed shim not being applied
5 participants