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

Deal with global css overrides #1880

Open
gkatsev opened this issue Feb 19, 2015 · 23 comments
Open

Deal with global css overrides #1880

gkatsev opened this issue Feb 19, 2015 · 23 comments
Labels
confirmed enhancement pinned Things that stalebot shouldn't close automatically

Comments

@gkatsev
Copy link
Member

gkatsev commented Feb 19, 2015

There are issues, currently, specifically with the captions settings menu, where if it lives on a page with Foundation (or maybe other css frameworks), it will look all weird.
There are two solutions I can think of, in no particular order:

  • Apply a css reset to the .video-js class and its descendents.
  • Use the iframe trick from things like the facebook like button.

The css-reset thing is a bit of an arms race, though, if done right, it could last a long time.
The iframe trick is pretty interesting. It involves creating a programmatic iframe and putting all your elements in there. Having the iframe means that the css of the outer page doesn't bleed over into our styles but it means that it's more difficult for users to style it.

@mmcc
Copy link
Member

mmcc commented Feb 19, 2015

@heff and I were actually talking about a reset yesterday, so I'm leaning that direction. I don't love increasing specificity by chaining the same class, but that might be the best way to do a reliable reset in this case.

@gkatsev
Copy link
Member Author

gkatsev commented Feb 19, 2015

Actually, I just thought up of another thing: web components and shadow DOM.
By default, CSS doesn't go into shadow DOM but there's now a selector that allows you to pierce shadow dom: >>>.

@mmcc
Copy link
Member

mmcc commented Feb 19, 2015

IE8. /me cries

@heff
Copy link
Member

heff commented Feb 19, 2015

What's the difference between the settings UI elements and the existing player UI elements where we're not running into these issues?

@mmcc
Copy link
Member

mmcc commented Feb 19, 2015

I can't speak to the settings UI elements, but for the new base theme, part of the issue is the fact that we're prepending the styles to the head. If I include the stylesheet after the reveal.js styles, things are fine, but if I let VJS automatically prepend, things get borked.

@heff
Copy link
Member

heff commented Feb 19, 2015

That's interesting. We definitely don't want it to matter what order you put those stylesheets in. Does that happen with the old skin too?

@gkatsev
Copy link
Member Author

gkatsev commented Feb 19, 2015

I'm sure it does.

@gkatsev
Copy link
Member Author

gkatsev commented Feb 19, 2015

I really wish that scoped stylesheets were actually a thing in more than 0.5% of all browsers.

@heff
Copy link
Member

heff commented Feb 19, 2015

I'd rather be sure it's not a regression before we make a decision like adding a CSS reset based on it. And that's still a different issue to me than the captions settings UI, and why they break while our existing UI elements don't.

We've had surprisingly few issues with the existing skin and specificity, so before we throw our hands in the air and add a generalized reset CSS, I think we should understand the problems more specifically.

@gkatsev
Copy link
Member Author

gkatsev commented Feb 19, 2015

Just to be clear, not suggesting a general css reset but rather a specific css reset to make sure all our styles will work as we expected them to work regardless of what other css is on the page.

the issue with the captions settings menu is that it uses general elements like select with very minimal, very non-specific styling. CSS frameworks like Foundation apply styles on on the tag names directly which override our specificity.

The reason why the controls themselves are ok is because we're using divs with heavy styling and we also use the pseudo-elements associated with those divs, which are generally neglected by css frameworks.

@mmcc
Copy link
Member

mmcc commented Feb 19, 2015

I can't imagine that aggressively resetting .video-js and the base elements contained would be a regression unless people were previously taking advantage of the limited specificity previously.

Not only are those things generally neglected by CSS frameworks, I've also never seen Video.js included before those frameworks in the wild (super scientific: purely based on what I've observed). The order I almost always see is something like this:

  • Framework (Bootstrap, Foundation, etc)
  • JS Libs (Video.js, etc)
  • Custom CSS

If we're prepending the CSS, that means that we're getting hit by some of the really general div / video styling / resets that some frameworks (Reveal.js in my experience) do.

@dmlap
Copy link
Member

dmlap commented Feb 19, 2015

I've run into issues with bootstrap as well. Given we haven't heard too many issues with specificity and video.js, I lean towards targeted resets. I think iframes should be removed from the running because then customizing the video.js skin from the containing page gets really hairy.

@gkatsev
Copy link
Member Author

gkatsev commented Mar 28, 2016

Looks like list styles are also a problem as seen in #3168.

@chemoish
Copy link
Member

chemoish commented Jun 23, 2016

My use case is to try and reuse some app elements within the player so my issue is reversed.

.video-js button is more specific (0110) than my .button (0100)

Tangentially, the move toward css-modules discourages nesting and, in turn, limits specificity (this is my case, but I am sure this is extremely edgy).

The only way I think I can get around it is by:

.button
{
  // styles

  :global(.video-js) &
  {
    // duplicate styles above
  }
}

// Style

.button-default
{
  composes: button;

  // additional styles

  :global(.video-js) &
  {
    // duplicate additional styles above
  }
}

Is it possible to move away from general elements (button, select, li, etc.)?

This might help with some of the specificity wars as .class (0100) is more specific than tag (0010).

Additionally, this would allow for easier, more targeted customization, but it still won't fix library order, reset, or specificity wars (But I don't think that battle can be won).

#3168 mentions:

.full-text ul li {}
// vs
.vjs-menu li {}
// vs my fictitious example
.vjs-menu-item {}

There is no way video.js can guard against it and I don't think it should? The only way to reset is to win another specificity war?

I think leveraging .vjs- should be good enough.

Other examples:

.video-js button {}
// vs my fictitious example
.vjs-button {}

.vjs-menu li.vjs-menu-title {}
// vs my fictitious example
.vjs-menu-title {}

.vjs-menu-button-popup .vjs-menu .vjs-menu-content {}
// vs my fictitious example
.vjs-menu-button-popup .vjs-menu-content {}

2 cents. Might be more trouble than its worth. Until then will continue the specificity wars.

@misteroneill
Copy link
Member

@chemoish I don't have a ton to add here except that I am in full agreement that CSS should always, always, always be entirely class-based for this exact reason (easy specificity math); so, 👍

@gkatsev
Copy link
Member Author

gkatsev commented Jul 6, 2016

I agree that videojs needs to have less specificity. However, we also need to make sure that for 80% of usecases, a user can just include videojs and the style and it works without modification.
Potentially, the solution here is shadow DOM (but it isn't support very well yet) since regular css doesn't cross the shadom DOM barrier but you can select through the barrier (with >>>).
I really need to read about css modules, maybe they solve or address these problems already.

@misteroneill
Copy link
Member

To be clear, I don't think it needs less specificity necessarily; just make the specificity easier to reason about.

@gkatsev
Copy link
Member Author

gkatsev commented Jul 6, 2016

I do think we need less specificity. There's some stuff in our css that's kind of silly and unnecessary. Either because we have a bunch of .foo .bar .baz .qux .quux:hover or because we have .video-js button like mentioned above.

@misteroneill
Copy link
Member

That's true. I was pointing out that I think having CSS selectors that are easy to grok in terms of specificity is more important to me than reducing specificity. But there are opportunities for both for sure.

There's also the question of what effect any CSS changes have on versioning. For example, if we make a selector more/less specific is that a breaking change? I mean, it could break someone's UI...

@chemoish
Copy link
Member

@gkatsev gkatsev added the pinned Things that stalebot shouldn't close automatically label Jul 2, 2018
@DennisJohnsen
Copy link

I currently found that the dots from my unordered lists suddenly disapeard. Located the issue to be from videojs via this css:

ul li,
ol li {
    list-style-type: none;
}

I know the last comment was back in 2016, but is there any plans to add .video-js in front of each selector to avoid global hitting elements, like this?

.video-js ul li,
.video-js ol li {
    list-style-type: none;
}

@gkatsev
Copy link
Member Author

gkatsev commented Jun 25, 2019

@DennisJohnsen I am not finding any usage of that in version 7, 6, or even 5 of Video.js.
I do see usage of list-style, but it's only for .vjs-menu li.

This issue is for dealing with outside styles affecting Video.js. If we are inadvertently affecting outside styles, we'll definitely update the css selectors as it isn't our intention to have our styles bleed out of our container.

@DennisJohnsen
Copy link

Cool, sorry about that. I really need to check up on which version I’m running then :P because my current version def have the above css bleeding, including some for .

But let me verify what version I’m using first.. which is probably what I should have done first :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed enhancement pinned Things that stalebot shouldn't close automatically
Projects
None yet
Development

No branches or pull requests

7 participants