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

return already created player when given an element #3006

Closed

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Jan 15, 2016

As part of the tech 2.0 work, we decided to remote the player out of the tech. One of the changes in it nulled out tag.player. This is an issue because videojs(el) uses tag.player to see whether we have already created, so, if you try to get the player again with the same el, it'll actually create a brand new player for you.
An example:

let el = document.querySelector('#my-video');
console.log(el.id); // "my-video"
let player = videojs(el);
console.log(el.id); // "my-video_html5_api"
let anotherPlayer = videojs(el);
console.log(el.id); // "my-video_html5_api_html5_api"
console.log(player == anotherPlayer); // false

The expected result should be something like this:

let el = document.querySelector('#my-video');
console.log(el.id); // "my-video"
let player = videojs(el);
console.log(el.id); // "my-video_html5_api"
let anotherPlayer = videojs(el);
console.log(el.id); // "my-video_html5_api"
console.log(player == anotherPlayer); // true

There are two ways I can think off right now to fix this:

  1. revert the changes that nulled out tag.player
  2. when given an element and tag.player in null, we would want to parse out the id to remove the stuff like html5_api and then return videojs.players[parsedId(tag.id)] if it exists, otherwise actually go ahead and create a player

I feel like solution number 2 is more correct but it is also a lot more complicated and more prone to issues. Included below right now are two failing tests for the above usecase.
@videojs/core-committers @dconnolly thoughts?

@heff
Copy link
Member

heff commented Jan 15, 2016

Since the video tag is the only tech that doubles as an embed code, you could look at it from the embed code side and add tag.player in the player init, just for this purpose.

@gkatsev
Copy link
Member Author

gkatsev commented Jan 15, 2016

In a conversation with @heff, we came to a simpler solution than 2 without resorting to 1. Instead, we're going to attach the playerId to the tag and use that to get back player references.

@heff
Copy link
Member

heff commented Jan 15, 2016

lgtm!

@gkatsev gkatsev closed this in 6977b0e Jan 15, 2016
@gkatsev gkatsev deleted the already-created-player-with-el branch January 15, 2016 20:36
jgubman added a commit to jgubman/video.js that referenced this pull request Jan 27, 2016
* 'master' of https://github.com/videojs/video.js: (38 commits)
  v5.6.0
  @gkatsev updated to latest videojs-ie8 shim. closes videojs#3042
  @hubdotcom changed URLs in README to be protocol-relative. closes videojs#3040
  @CoWinkKeyDinkInc fixed table in Tracks guide. Replaced some single quotes with double quotes. closes videojs#2946
  @aril-spetalen added language support for Norwegian (nb and nn). closes videojs#3021
  @vitor-faiante updated the guides. closes videojs#2781
  @gkatsev checked muted status when updating volume bar level. closes videojs#3037
  @OwenEdwards fixed double-localization of mute toggle control text. closes videojs#3017
  @mister-ben made $primary-foreground-color a !default sass var. closes videojs#3003
  @OwenEdwards Fixed volume menu keyboard access. closes videojs#3034
  @OwenEdwards Fixed menu keyboard access and ARIA labeling for screen readers. closes videojs#3033
  @OwenEdwards added ClickableComponent. Fixed keyboard operation of buttons. closes videojs#3032
  v5.5.3
  @rcrooks fixed a couple of docs link and a jsdoc comment. closes videojs#2987
  @mister-ben updated CDN urls in setup guide. closes videojs#2984
  @gkasev updated vjs to correctly return already created player when given an element. closes videojs#3006
  v5.5.2
  make sure that styleEl_ is in DOM before removing on dispose. closes videojs#3004
  v5.5.1
  @gkatsev fixed sass if else for icons. closes videojs#2988
  ...
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

Successfully merging this pull request may close these issues.

2 participants