search

phetsims / phet-core

Core utilities used by all PhET simulations.
MIT License
8 stars 6 forks source link

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

Open jessegreenberg opened 2 years ago

jessegreenberg commented 2 years ago

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".

https://github.com/phetsims/phet-core/blob/e3a8916b3cbf1e1b1d9fb763d2bd26436cf1b42d/js/platform.js#L82-L83

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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?