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

video-js.css corrupt #3013

Closed
ghost opened this issue Jan 20, 2016 · 15 comments
Closed

video-js.css corrupt #3013

ghost opened this issue Jan 20, 2016 · 15 comments

Comments

@ghost
Copy link

ghost commented Jan 20, 2016

I found several errors in http://vjs.zencdn.net/5.4.6/video-js.css:
content: "";
@media �screen
Maybe its related to character encoding?
Can you provide me minified CSS with UTF-8 encoding?

@gkatsev
Copy link
Member

gkatsev commented Jan 20, 2016

None of those are errors and that file is fully utf-8 compliant. The issue is that sass converts the unicode codepoints into utf8 characters but when you view the css directly, it looks like a square because you don't have the font loaded in. We do have a fix in a prerelease version of videojs that will go around sass and have the final output of the css have the codepoints in them directly.
As for the @media clause, apparently, it's a trick to keep IE8 inline: https://github.com/videojs/video.js/blob/master/src/css/components/_control-bar.scss#L45-L52
Thanks, hope that helps clear things up.

@gkatsev gkatsev closed this as completed Jan 20, 2016
@ghost
Copy link
Author

ghost commented Jan 21, 2016

Thank you very much i really appreciate your work.

@thomaspeeters
Copy link

@media �screen is causing unexpected behaviour in IE11 on Windows 7 for me. It is supposed to be @media \0screen, which should target only IE8, but it appears to be broken.

Currently, the rule is applied in IE8 and higher, instead of just IE8. Changing the to \0 in the CSS fixed the issue for me.

The expected behaviour is for the @media rule to not be applied in IE11, but the actual behaviour is that it is applied.
Perhaps it would be better to separate IE8-specific CSS in the same way IE8-specific JS is in a separate file?

For clarity's sake I should add how I encountered this bug. I am overriding the default CSS so the controls aren't hidden during playback when the player is inactive.

These are my overriding rules:

.video-js .vjs-control-bar {
    display: flex;
}

.vjs-has-started.vjs-user-inactive.vjs-playing .vjs-control-bar {
    visibility: visible;
    opacity: 1;
}

In Chrome and Firefox this results in a controlbar which is always visible, in IE11 this results in all the button icons disappearing when the player enters the inactive state.
Adding the following overriding rule did not work, for a reason unknown to me.

@media screen {
    .vjs-user-inactive.vjs-playing .vjs-control-bar :before {
        content: inherit; }
}

An option to disable automatically hiding the controls also seems like an elegant solution to me.

@thomaspeeters
Copy link

I just updated to the latest version (5.7.1) and noticed this issue is still present. Please re-open this issue, the IE8 media query is still broken.

@gkatsev
Copy link
Member

gkatsev commented Feb 16, 2016

Yes, you're correct. We need to somehow address the @media \0screen issue. Re-opening.

@gkatsev gkatsev reopened this Feb 16, 2016
@gkatsev
Copy link
Member

gkatsev commented Feb 16, 2016

Just wanted to comment that there's no easy fix because sass is doing weird stuff to it.

@thomaspeeters
Copy link

Thanks for re-opening this issue.

Would it be feasible to separate the IE8 code into a separate CSS file, like it is done for the IE8 JS? You could distribute it as videojs-ie8.css in the ie8/ folder along with videojs-ie8.js.
It seems logical to include it only when IE8 support is necesarry. It wouldn't solve the Sass issue, but it would sort of "quarantine" the problem and reduce the amount of potential problems.

If it is just a few lines of CSS, it might even be easier to drop Sass for the IE8 CSS and just use plain CSS.

What do you think?

@gkatsev
Copy link
Member

gkatsev commented Feb 17, 2016

Yeah, I'm thinking that the easiest way to solve this (since we can't wait for sass to solve it, they've had an issue related to it open for months) is to pull out that block into a separate css file and then concatenate it as part of the build.

@thomaspeeters
Copy link

Exactly. It's not the most ideal solution, but by the time Sass fixes the issue IE8 support might be irrelevant.

@gkatsev
Copy link
Member

gkatsev commented Feb 17, 2016

Would you be up for submitting a PR?

@thomaspeeters
Copy link

Honestly, I understand conceptually what needs to happen, but I have no idea how to execute it in this project (concatenating the separate CSS file during the build). I've never used Sass or Grunt.

@thasmo
Copy link

thasmo commented May 20, 2016

@erikyuzwa
Copy link
Contributor

I submitted a PR for review @thasmo on this.
I'm coming from the assumption that when deployed to a site, you'd be including this separate generated css with that conditional ie8 markup.

@gkatsev
Copy link
Member

gkatsev commented Aug 5, 2016

This is fixed thanks to @erikyuzwa (#3380). Will be out in 5.12, when it gets released.

@gkatsev gkatsev closed this as completed Aug 5, 2016
@erikyuzwa
Copy link
Contributor

🙇 it's a team effort for sure - thanks @gkatsev 👍

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants