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

Is chrome on edge return true #3232

Closed
wants to merge 3 commits into from
Closed

Conversation

benjipott
Copy link
Contributor

Description

Fix videojs.isChrome() on windows 10 edge return true

Mozilla/5.0 (Windows NT 10.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2311.135 Safari/537.36 Edge/12.10136

@gkatsev
Copy link
Member

gkatsev commented Apr 1, 2016

LGTM.
Wondering if you were seeing any problems related to it or did you just notice that it was true?

@gkatsev gkatsev added patch This PR can be added to a patch release. needs: LGTM Needs one or more additional approvals labels Apr 1, 2016
@benjipott
Copy link
Contributor Author

Juste notice it, on windows 10 edge, if vjs / plugin test videojs.browser.IS_CHROME return true i think this const should return false

@gkatsev
Copy link
Member

gkatsev commented Apr 1, 2016

yeah, we should definitely have it return the right value.

@gkatsev gkatsev added minor This PR can be added to a minor release. It should not be added to a patch release. and removed patch This PR can be added to a patch release. labels Apr 1, 2016
@nickygerritsen
Copy link
Contributor

@benjipott I was thinking: because Edge has kinda a strange UA string, isn't it better to put some IS_EDGE check at the top and for others add an !IS_EDGE && ...?

I can imagine we later add a Safari check and it will break again, because Edge also contains Safari.

So i.e.:

export const IS_EDGE = (/Edge/i).test(USER_AGENT);
export const IS_CHROME = !IS_EDGE && (/Chrome/i).test(USER_AGENT);
// etc.

@gkatsev gkatsev added patch This PR can be added to a patch release. and removed minor This PR can be added to a minor release. It should not be added to a patch release. labels Apr 12, 2016
@nickygerritsen
Copy link
Contributor

OK LGTM :)

@misteroneill
Copy link
Member

LGTM as well 👍

@gkatsev gkatsev added confirmed and removed needs: LGTM Needs one or more additional approvals labels Apr 13, 2016
@gkatsev gkatsev closed this in fdd4d25 Apr 19, 2016
jgubman added a commit to jgubman/video.js that referenced this pull request Apr 26, 2016
* upstream/stable: (77 commits)
  v5.9.2
  @gkatsev grouped text track errors in the console, if we can. closes videojs#3259
  v5.9.1
  @gkatsev fixed text track tests for older IEs. closes videojs#3269
  revert 75116d4 adding chrome to travis (videojs#3254)
  @forbesjo added back the background color to the poster. closes videojs#3267
  @gkatsev fixed removeRemoteTextTracks not working with return value from addRemoteTextTracks. closes videojs#3253
  @gkatsev made the first emulated text track enabled by default. closes videojs#3248
  @mister-ben blacklisted Chrome for Android for playback rate support. closes videojs#3246
  @benjipott updated IS_CHROME to not be true on MS Edge. closes videojs#3232
  v5.9.0
  @andyearnshaw updated document event handlers to use el.ownerDocument. closes videojs#3230
  @chrisauclair added ARIA region and label to player element. closes videojs#3227
  @MCGallaspy added vttjs to the self-hosting guide. closes videojs#3229
  @forbesjo added chrome for PR tests. closes videojs#3235
  @OwenEdwards improved handling of deprecated use of Button component. closes videojs#3236
  v5.8.8
  @seescode fixed dragging on mute toggle changing the volume. Fixes videojs#3036. Closes videojs#3228
  @seescode fixed css failing on IE8 due to incorrect ie8 hack. Fixes videojs#3140. Closes videojs#3226.
  @vtytar fixed auto-setup failing if taking too long to load. Fixes videojs#2386. Closes videojs#3233.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants