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

user agent match for Edge is not correct in platform.js #115

Open
jessegreenberg opened this issue Apr 8, 2022 · 4 comments
Open

user agent match for Edge is not correct in platform.js #115

jessegreenberg opened this issue Apr 8, 2022 · 4 comments
Assignees

Comments

@jessegreenberg
Copy link
Contributor

Noticed while working on #90, the user agent for Edge is
'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/100.0.0.0 Safari/537.36 Edg/100.0.0.0'

but platform.js is looking for "Edge".

phet-core/js/platform.js

Lines 82 to 83 in e3a8916

// Whether the browser is Microsoft Edge
edge: !!ua.match( /Edge\// ),

Replacing with "Edg" fixes it. There are two usages of platform.edge in the project, both are platform workarounds in scenery. Maybe they probably aren't necessary anymore if the platform.edge checks haven't been working.

@jonathanolson do you have a recommendation for how to proceed?

@jessegreenberg
Copy link
Contributor Author

This also means that the platform.chromium check is returning true for Edge. That may be the right thing to do in the long term, but we are trying to weed out Edge in the implementation currently

chromium: ( /chrom(e|ium)/ ).test( ua.toLowerCase() ) && !ua.match( /Edge\// )

@jonathanolson
Copy link
Contributor

Perhaps we should check what platform checks use it? If it's for platform-specific bugs, I'm fine changing it to look for Edg

@jessegreenberg
Copy link
Contributor Author

I was able to remove the Edge workarounds that I added. There are a few left in scenery internals in Text.ts, TextSVGDRawable.js and Input.ts. I had a worry that that the remaining workarounds might actually be harmful for Chromium based Edge and we could have problems if we just change platform.js?

@jonathanolson
Copy link
Contributor

I had a worry that that the remaining workarounds might actually be harmful for Chromium based Edge and we could have problems if we just change platform.js?

Can we identify Chromium-based Edge independently? If we can avoid using the workarounds, that sounds beneficial.

The other side is... presumably we don't support Edge based on non-Chromium anymore, so perhaps we just remove the workarounds and test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants