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

fix: add use of the safe version of getComputedStyle #6073

Merged
merged 1 commit into from Jul 29, 2019

Conversation

TVS-Bruno
Copy link
Contributor

@TVS-Bruno TVS-Bruno commented Jun 24, 2019

Description

Add use of the safe version of getComputedStyle in the currentDimension method of Component's class.
This will prevent a null exception when a video.js is implemented in a 'display:none' iframe on Firefox (<62).

This is a fix in continuation of the MR #3664 regarding a bug in Firefox https://bugzilla.mozilla.org/show_bug.cgi?id=548397

Specific Changes proposed

  • use the safe version of getComputedStyle already present in video.js
  • small improvement of the safe version of the getComputedStyle

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created
  • Reviewed by Two Core Contributors

Add use of the safe version of computedStyle in the currentDimension method of Component's class.
This will prevent a null exception when a video.js is implemented in a 'display:none' iframe on Firefox (<62).

This is a fix in continuation of the MR#3664 regarding a bug in Firefox https://bugzilla.mozilla.org/show_bug.cgi?id=548397
@welcome
Copy link

welcome bot commented Jun 24, 2019

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@@ -26,9 +26,9 @@ function computedStyle(el, prop) {
}

if (typeof window.getComputedStyle === 'function') {
const cs = window.getComputedStyle(el);
const computedStyleValue = window.getComputedStyle(el);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this should just be called style? That is what they use on mdn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to this page?
https://developer.mozilla.org/fr/docs/Web/API/Window/getComputedStyle

They also use cs in the examples, but I'm not a fan of very short abbreviations or too much generic term where you have to go back to the declaration to understand exactly the variable.

Do you want me to change my commit to use the name style?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine.

@@ -26,9 +26,9 @@ function computedStyle(el, prop) {
}

if (typeof window.getComputedStyle === 'function') {
const cs = window.getComputedStyle(el);
const computedStyleValue = window.getComputedStyle(el);
Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine.

@gkatsev gkatsev merged commit 20cae21 into videojs:master Jul 29, 2019
@welcome
Copy link

welcome bot commented Jul 29, 2019

Congrats on merging your first pull request! 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants