Don't add style elements if VIDEOJS_NO_DYNAMIC_STYLE is set to true

[gkatsev]

This fixes #3051 (and #2867) by making sure that there's a way to make videojs doesn't add style elements to the DOM. This means that it will not have any default styles like default size dimensions. It also means that if the player is configured with dimensions, they will not be applied.

<video width=600 height=300></video>

That will end up looking like (assuming the videojs css is still included):

And player.width() and player.height() will return 600 and 300 respectively.

TODO:

• Update skins guide with this information as well as the fact that base_styles.js isn't currently wired up
• Add tests to make sure this is working correctly
• Add tests to make sure that videojs doesn't accidentally add any style elements to the DOM when VIDEOJS_NO_BASE_THEME is set to true

[gkatsev]

I'm not certain that there's a good way of doing the 3rd TODO task above.

[gkatsev]

this section is the main addition. The rest is mostly to keep lines from getting too long in the editor.

[gkatsev]

As part of this, it means that player.width() and player.height() will become no-ops because they edit the stylesheet that no longer gets included, however, that doesn't seem correct. I believe these methods need to still work even if VIDEOJS_NO_BASE_THEME is set.
What do @videojs/core-committers think?
I believe it's possible to have it work and be css overridable, even if the videojs css is included anyway.

[heff]

It seems like if a user is setting VIDEOJS_NO_BASE_THEME it's a pretty clear indicator that they're using CSS to style the player and wouldn't be relying on player.width/height, or are technical enough not to need those. So to me it doesn't seem worth any extra complexity in the code to support that use case (assuming there's extra complexity). It could just be documented and/or throw a warning if the two are used together. I feel like we've done a lot of work to hold up the contract that the player will work like the video element out of the box, and if someone sets VIDEOJS_NO_BASE_THEME then they're intentionally stepping outside that contract (and they should understand that). My 2c.

[gkatsev]

@heff does that mean that we should have a different mechanism for telling videojs to not use add style elements?
If player.width() and player.height() are analogous to video.width and video.height it means that they must always work.

[heff]

I'm saying that I think using VIDEOJS_NO_BASE_THEME plus player.width() is an unlikely use case and could be considered breaking out of the contract where player.width() must always work like video.width. VIDEOJS_NO_BASE_THEME == "taking player sizing into your own hands". If you feel more strongly that player.width() should still uphold that contract in this use case, that's 100% cool with me.

[gkatsev]

@heff so, I think we should take a step back a second to talk about what we're trying to accomplish and what is happening. Hopefully, the following writeup will clear it up and we can discuss what action we should take.

What is the scenario?

videojs does not currently add the theme to the page by itself, a user is required to include either their own styles or the videojs styles (or both).
Videojs adds a style element for default player dimensions (.video-js { width: 300px; height: 150px; }). I've tried to look exactly why we do this but I could not find it. I do know there was a reason.
videojs adds a style element to the HEAD for the current player dimensions. When a user calls player.width() it then updates that stylesheet.

The Problem

In a lot of cases, these dynamically inserted style elements are causing issues.
They can cause an issue when a user is using virtual DOM (like in #3051) or it can cause an issue in IE where IE uses insertion order for breaking ties in selector specificity.

The need

There needs to be a way to make videojs not insert any style element into the DOM. This is possibly tangential to making videojs not insert a whole theme (which it doesn't do right now anyway).

Current solution

The current solution piggy backs on the previously proposed VIDEOJS_NO_BASE_THEME property to not insert these elements.

Possible requirements

• It is possible to have videojs not insert any style elements dynamically into the page
• player#width and player#height continue to function

I think it makes sense to have player#width and player#height to not work when VIDEOJS_NO_BASE_THEME is set, so, this leads me to think that we need a different signal to tell videojs to just not have dynamic stylesheets.

[heff]

Ha, yeah, this is a lot to wrap your head around. I can't say I grasp on the entire issue yet, but a separate signal seems like it could make sense.

Here's the earlier research that led to the default 300x150.
http://blog.heff.me/post/75620807175/default-video-element-sizing

[yareckon]

Hey, would love to have this as separate flag. We use the base styling for things like the progress bar, but constrain the player ourselves with wrappers. The styles in head are requiring us to increase specificity in our code a couple of places where we would rather not.

[gkatsev]

@videojs/core-committers I think we want to do this under a separate flag like VIDEOS_NO_DYNAMIC_STYLE or something. Thoughts? I'd like to finish this up this week, if possible.

[misteroneill]

I think it makes a ton of sense to not piggy-back on VIDEOJS_NO_BASE_THEME and go with a new flag like VIDEOS_NO_DYNAMIC_STYLE. Would this new flag trigger video.js to use a more old-school "update the el().style" approach or simply not do any sizing?

I agree that width() and height() ought to work in some capacity even with VIDEOS_NO_DYNAMIC_STYLE.

[gkatsev]

I think that the width and height function should continue to work. I would want to experiment with maybe just setting the width and height attributes on the video element directly for when the dynamic style els are disabled.

[gkatsev]

I think this is good to go for review by @heff and other @videojs/core-committers.
When VIDEOJS_NO_DYNAMIC_STYLE is set it will not add any dynamic style elements to the DOM. In addition, player.width and player.height will apply the size to the tech's element as width and height attributes, so, assuming no videojs sizes are included, it will be sized properly.

[misteroneill]

I feel like "dom" should be capitalized since it's an acronym.

[gkatsev]

It should.

[heff]

This is a clever way to handle it. lgtm

[gkatsev]

Alright, this is ready for final review @heff @videojs/core-committers

[heff]

lgtm!

[chemoish]

styles element => style element?

#donthitme

[gkatsev]

Heh, I think both are technically correct though a bit ambiguous. I was going for ((default styles) element) as opposed to a (default (style element)).
Maybe I should just change it to default styles style element?