feat: allow embeds via <video-js> element #4640

gkatsev · 2017-10-03T06:06:07Z

Description

Add the ability to initialize Video.js with an element named video-js. This is because sometimes, seeing the native element is undesirable, plus, it's cool to have our own element.
Can be used just like the video embed. For IE9-only we have a vjs-source element that's copied to a source element because IE9 is strict about keeping source elements inside of video elements.

TODO

tests need to be updated
fix tests in IE8
Limit this feature to IE10+ so we won't need a vjs-source element?
documentation
we should potentially update Player.getTagSettings instead of my workaround for sources to work properly with vjs-source.
We may need a vjs-track as well for IE9.

gkatsev · 2017-10-12T21:52:51Z

src/js/setup.js

@@ -31,6 +31,7 @@ const autoSetup = function() {
  // through each list of elements to build up a new, combined list of elements.
  const vids = document.getElementsByTagName('video');
  const audios = document.getElementsByTagName('audio');
+  const divs = document.getElementsByTagName('div');


this allows for using data-setup with divs. Though, this seems a bit generic, should we use a different trigger with divs?

gkatsev · 2017-10-12T21:54:49Z

sandbox/embeds.html.example

@@ -0,0 +1,165 @@
+<!DOCTYPE html>


this file will get autocopied into sandbox/embeds.html on the first npm start run by the user.
It has 18 different video.js embeds grouped by whether data-setup is used or not, how the source is added, and the 3 different embed types: video, video+player div wrapper, just div.

gkatsev · 2017-10-12T21:55:38Z

src/js/player.js

+
+    if (divEmbed) {
+      el = this.el_ = tag;
+      tag = this.tag = document.createElement('video');


we probably should try to be smarted about this. Maybe we need an audio element instead but we might not have a source at this point. Probably more than good enough for first release.

Isn't it possible that the div will contain a video element already?

Missed this previously. Yes but I will say it's misconfigured in that case.

gkatsev · 2017-10-12T21:56:17Z

So, other than adding a lot of tests, we should decide if we're ok with using data-setup on the div elements. Otherwise, this is pretty much good to go, I think.

gkatsev · 2017-10-12T21:57:17Z

sandbox/embeds.html.example

+    }
+
+    .wrapper {
+      display: grid;


yeah, grid!

gkatsev · 2017-10-13T17:52:03Z

In a grooming discussion, we talked about the div and data-setup issue. Because div is a lot more used and data-setup is a relatively generic name, we are at risk of trying to initialize a div that was not given to video.js. One suggestion that was brought up is to rename data-setup to be data-vjs-setup and keep data-setup for video/audio elements and perhaps deprecate it altogether.
What are your thoughts @videojs/core-committers?

heff · 2017-10-15T17:08:26Z

Is the div embed meant to take over as the primary embed method or is it an optional feature for React-like situations?

data-setup is definitely a convenience thing. It's specifically useful in situations where a user doesn't have access to add javascript but can add HTML (some CMSs). Other than that, if this is an optional feature I'd be in favor of dropping data-setup for the div and see if anyone misses it. Otherwise data-vjs-setup seems fine too.

What I'd really love to see is a videojs web component instead of a div, if that at all makes sense to do here.

gkatsev · 2017-10-16T18:30:21Z

Haven't thought about whether this should be the default yet. The current pain-point is that if you use the video tag, you end up quickly flashing the default video element before video.js gets initialized. So, this will solve it. However, this will also work better for the react-like situations compared to what we currently have with the "ingested player div".

I do like the idea of a web-component, but I think that isn't necessarily equivalent to a div embed. Maybe just a custom element? However, native custom element support is pretty small right now and I would prefer not to have to include a polyfill for it.

ldayananda · 2017-10-17T01:34:04Z

I'm in favor of using data-vjs-setup and deprecating data-setup

mister-ben · 2017-10-17T19:54:59Z

data-vjs-setup sounds like a sensible way to go.

gkatsev · 2017-10-18T19:18:49Z

The broken PR was interesting because it revealed that IE9 doesn't allow using a source element outside of a video element. We'd want to make sure that this is documented well before this PR is merged.

gkatsev · 2017-10-19T19:41:14Z

At the video.js TSC meeting, we talked about whether we can just use a video-js element and a vjs-source element. At first glance is that we can but with the div embed we can just re-use that as the player div and create a tech video element inside of it. However, it seems like we won't be able to do that with the video-js element. Will investigate further.

heff · 2017-10-20T17:20:05Z

I thought custom elements were basically just renamed divs unless you subclassed a native element.

gkatsev · 2017-10-23T18:17:22Z

Oh, I think I figured out my issue. I hadn't updated the code to look for video-js over div and then I didn't actually copy the child elements over properly.

gkatsev

So, with these changes, you can now use a video-js element! And also vjs-source elements (this mostly as a workaround for IE9).
moved things left to do to the top post

gkatsev · 2017-10-23T19:24:36Z

src/js/player.js

@@ -374,6 +375,8 @@ class Player extends Component {

    this.el_ = this.createEl();

+    this.options_.sources = Player.getTagSettings(this.tag).sources || this.options_.sources;


we might want to just update getTagSettings to know about vjs-source as well, instead of this, though, this seems to work.

This breaks some tests, so, removing and updating getTagSettings.

gkatsev · 2017-10-23T19:24:59Z

src/js/setup.js

@@ -31,6 +31,7 @@ const autoSetup = function() {
  // through each list of elements to build up a new, combined list of elements.
  const vids = document.getElementsByTagName('video');
  const audios = document.getElementsByTagName('audio');
+  const divs = document.getElementsByTagName('video-js');


we now look for video-js elements for data-setup, and so we definitely can keep data-setup now.

gkatsev · 2017-10-23T19:39:32Z

@videojs/core-committers Should we only allow video-js in browsers newer than IE9? Then we won't even need to have vjs-source support.

misteroneill

Looks good overall. I wonder if some of the variable names should be changed so they're not called divs... although, I can't think of a better option off the top of my head!

gkatsev · 2017-10-24T20:11:57Z

Also, do we want the elements to be called video-js and vjs-source or just videojs and vjssource?

mister-ben · 2017-10-26T11:01:31Z

Custom element names should contain a hyphen for namespacing.

video-* is probably too likely to conflict with something eventually. videojs-player might be safer.

gkatsev · 2017-10-26T15:09:10Z

Yup, probably best to keep the hyphen, though it isn't technically a real custom element.
I don't think that any potential conflicts that video-js can have are big enough and having it match the name is super nice, which is why I was asking about whether we should just go with videojs at the element name.

gkatsev · 2017-10-26T21:30:43Z

docs/guides/embeds.md

@@ -0,0 +1,83 @@
+# How to Embed the Video.js player


after I added this page I remember that we have the setup.md file. This page can probably stay as is but setup.md probably should be updated a bit as well and point to this page.

Linked from setup.md now.

misteroneill

Some minor grammatical points and one question about class.

misteroneill · 2017-11-07T19:32:14Z

docs/guides/embeds.md

@@ -0,0 +1,85 @@
+# How to Embed the Video.js player
+
+Video.js is meant to be an ehnacement to the video element in HTML5 so for years, it's embed code has been just a `<video>` element.


Typo ehnacement and it's should be its

misteroneill · 2017-11-07T19:33:22Z

docs/guides/embeds.md

+Video.js is meant to be an ehnacement to the video element in HTML5 so for years, it's embed code has been just a `<video>` element.
+Video.js then wraps the video element in a div that is used for us to place controls and anything else that's required for the player.
+
+For a long time this was enough. In 2016, a "div ingest" which allows the developer to give Video.js a player div to use instead of making it's own.


Second sentence is incomplete. Maybe needs was added somewhere?

oops, yes, something like that.

misteroneill · 2017-11-07T19:35:23Z

docs/guides/embeds.md

+
+```html
+<!-- via data-setup -->
+<video-js id="vid1" class="video-js" data-setup='{}'>


Should you need the class="video-js"? Could we add that if it's missing?

Could make sense to add it for the video-js element. Adding it unconditionally could potentially cause stylistic bugs.

misteroneill · 2017-11-07T19:36:27Z

docs/guides/embeds.md

+- IE9 doesn't support having `source` elements outside of the `video` element, thus, the `video-js` embed will not work there. Though, if the source is set later, it should still work.
+
+## data-setup
+This is an ease-of-use method for having Video.js set up the player automatically. It is an html attribute and it takes a JSON string representation of the [player options](/docs/guides/options.md) as the value.


html should be all caps.

pagarwal123 · 2017-11-07T20:49:59Z

QA-LGTM

misteroneill · 2017-11-07T21:35:11Z

src/js/player.js

+
+      if (el.className.indexOf('video-js') === -1) {
+        el.className += ' video-js';
+      }


Should we use dom.hasClass() here? Wouldn't this leave room for false positives like if it had video-js-xyz?

And addClass() maybe?

oh, yeah, I forgot we had helpers for this. Didn't use the player methods because it isn't quite available yet.

This reverts commit ef079fb.

This reverts commit 3b9338c.

…parately.

A we retained a lot of references to DOM elements in various components. Here we clear it up. Also, make sure that we remove unused listeners as they can retain objects as well. Update evented mixin to null out the eventBusEl_ after the component is disposed. Add a feature for components, to tell it not to auto-initialize the evented mixin. Re-enable the tests that were removed in #4640.

gkatsev commented Oct 12, 2017

View reviewed changes

sandbox/embeds.html.example

}

.wrapper {

display: grid;

Copy link

Member Author

gkatsev Oct 12, 2017

Choose a reason for hiding this comment

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

yeah, grid!

gkatsev commented Oct 23, 2017

View reviewed changes

gkatsev changed the title ~~WIP initial div embed PR~~ feat: allow embeds via <video-js> element Oct 23, 2017

gkatsev changed the title ~~feat: allow embeds via <video-js> element~~ feat: allow embeds via <video-js> element Oct 23, 2017

misteroneill reviewed Oct 24, 2017

View reviewed changes

gkatsev commented Oct 26, 2017

View reviewed changes

This was referenced Oct 27, 2017

getting error "TypeError: this.el_[prop] is not a function" #4694

Closed

Log a warning if the element given to Video.js isn't in the DOM #4697

Closed

gkatsev added the needs: LGTM Needs one or more additional approvals label Nov 7, 2017

misteroneill reviewed Nov 7, 2017

View reviewed changes

misteroneill approved these changes Nov 7, 2017

View reviewed changes

gkatsev added 24 commits November 13, 2017 13:56

prettify separator comment

c760cef

remove broken test as there's no need to duplicate it

48b3b51

use video-js not div

ab66e99

copy over vjs-source els as source els

8cf40f2

reparse tag to get sources

f2dfc2f

fix linting

59ce1dc

switch tests from div to video-js

e6d541a

update embeds.html.example to use video-js

8ffac5c

update embeds sandbox to title things correctly

96a68f4

don't reset options_.sources

bc35b83

update getTagNSettings to support vjs-source

8df4bd8

add video-js to IE8 element shims

eb10ec9

revert unnecessary test change

7a493c5

add documentation

1a69dc6

Revert "copy over vjs-source els as source els"

612013a

This reverts commit ef079fb.

Revert "update getTagNSettings to support vjs-source"

3eb38c6

This reverts commit 3b9338c.

update embeds.md

4bb535e

link to embeds.md from setup.md

8c45c2c

remove vjs-source from shims

5b621a7

reference the child properly

992781d

doc updates

8fd67a9

auto-add video-js class with video-js embed

e91952f

use our Dom helpers

4473462

comment out tests on IE8 to unblock PR so we can fix tests/memleak se…

787299b

…parately.

gkatsev force-pushed the div-embed branch from 6bacce3 to 787299b Compare November 13, 2017 18:58

gkatsev merged commit d8aadd5 into master Nov 13, 2017

gkatsev removed the needs: LGTM Needs one or more additional approvals label Nov 13, 2017

gkatsev deleted the div-embed branch November 13, 2017 19:20

gkatsev mentioned this pull request Dec 22, 2017

Flash of unstyled HTML player #4838

Closed

		@@ -374,6 +375,8 @@ class Player extends Component {

		this.el_ = this.createEl();

		this.options_.sources = Player.getTagSettings(this.tag).sources \|\| this.options_.sources;

		@@ -0,0 +1,85 @@
		# How to Embed the Video.js player

		Video.js is meant to be an ehnacement to the video element in HTML5 so for years, it's embed code has been just a `<video>` element.

feat: allow embeds via <video-js> element #4640

feat: allow embeds via <video-js> element #4640

Conversation

gkatsev commented Oct 3, 2017 • edited Loading

Description

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Oct 12, 2017

Choose a reason for hiding this comment

gkatsev commented Oct 13, 2017

heff commented Oct 15, 2017

gkatsev commented Oct 16, 2017

ldayananda commented Oct 17, 2017

mister-ben commented Oct 17, 2017

gkatsev commented Oct 18, 2017

gkatsev commented Oct 19, 2017

heff commented Oct 20, 2017 via email

gkatsev commented Oct 23, 2017

gkatsev left a comment • edited Loading

Choose a reason for hiding this comment

gkatsev Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Oct 23, 2017

misteroneill left a comment

Choose a reason for hiding this comment

gkatsev commented Oct 24, 2017

mister-ben commented Oct 26, 2017

gkatsev commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misteroneill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pagarwal123 commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Oct 3, 2017 •

edited

Loading

gkatsev left a comment •

edited

Loading

gkatsev Oct 23, 2017 •

edited

Loading