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

feat: add a safe getComputedStyle to videojs. #3664

Merged
merged 2 commits into from
Nov 3, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/js/control-bar/progress-control/mouse-time-display.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
/**
* @file mouse-time-display.js
*/
import window from 'global/window';
import Component from '../../component.js';
import * as Dom from '../../utils/dom.js';
import * as Fn from '../../utils/fn.js';
import formatTime from '../../utils/format-time.js';
import throttle from 'lodash-compat/function/throttle';
import computedStyle from '../../utils/get-computed-style.js';

/**
* The Mouse Time Display component shows the time you will seek to
Expand Down Expand Up @@ -71,7 +71,7 @@ class MouseTimeDisplay extends Component {
if (this.keepTooltipsInside) {
const clampedPosition = this.clampPosition_(position);
const difference = position - clampedPosition + 1;
const tooltipWidth = parseFloat(window.getComputedStyle(this.tooltip).width);
const tooltipWidth = parseFloat(computedStyle(this.tooltip, 'width'));
const tooltipWidthHalf = tooltipWidth / 2;

this.tooltip.innerHTML = time;
Expand All @@ -98,8 +98,8 @@ class MouseTimeDisplay extends Component {
return position;
}

const playerWidth = parseFloat(window.getComputedStyle(this.player().el()).width);
const tooltipWidth = parseFloat(window.getComputedStyle(this.tooltip).width);
const playerWidth = parseFloat(computedStyle(this.player().el(), 'width'));
const tooltipWidth = parseFloat(computedStyle(this.tooltip, 'width'));
const tooltipWidthHalf = tooltipWidth / 2;
let actualPosition = position;

Expand Down
6 changes: 3 additions & 3 deletions src/js/control-bar/progress-control/seek-bar.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
/**
* @file seek-bar.js
*/
import window from 'global/window';
import Slider from '../../slider/slider.js';
import Component from '../../component.js';
import * as Fn from '../../utils/fn.js';
import formatTime from '../../utils/format-time.js';
import computedStyle from '../../utils/get-computed-style.js';

import './load-progress-bar.js';
import './play-progress-bar.js';
Expand Down Expand Up @@ -65,8 +65,8 @@ class SeekBar extends Slider {
this.updateAriaAttributes(this.tooltipProgressBar.el_);
this.tooltipProgressBar.el_.style.width = this.bar.el_.style.width;

const playerWidth = parseFloat(window.getComputedStyle(this.player().el()).width);
const tooltipWidth = parseFloat(window.getComputedStyle(this.tooltipProgressBar.tooltip).width);
const playerWidth = parseFloat(computedStyle(this.player().el(), 'width'));
const tooltipWidth = parseFloat(computedStyle(this.tooltipProgressBar.tooltip, 'width'));
const tooltipStyle = this.tooltipProgressBar.el().style;

tooltipStyle.maxWidth = Math.floor(playerWidth - (tooltipWidth / 2)) + 'px';
Expand Down
21 changes: 21 additions & 0 deletions src/js/utils/get-computed-style.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/**
* A safe getComputedStyle with an IE8 fallback.
*
* This is because in Firefox, if the player is loaded in an iframe with `display:none`,
* then `getComputedStyle` returns `null`, so, we do a null-check to make sure
* that the player doesn't break in these cases.
* See https://bugzilla.mozilla.org/show_bug.cgi?id=548397 for more details.
*
* @function getComputedStyle
* @param el the element you want the computed style of
* @param prop the property name you want
*/
export default function getComputedStyle(el, prop) {
if (typeof getComputedStyle === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like we may want to check for el or prop being null here and return empty sting if that happens.

Copy link
Member

Choose a reason for hiding this comment

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

Also, shouldn't this be window.getComputedStyle?

I think this'll detect the module-level function instead of the global one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it'll be safer to do window..

I guess checking that el and prop are available is good. I'll refrain from doing other type checking, though.

const cs = getComputedStyle(el);

return cs ? cs[prop] : '';
}

return el.currentStyle[prop];
}
15 changes: 15 additions & 0 deletions src/js/video.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import log from './utils/log.js';
import * as Dom from './utils/dom.js';
import * as browser from './utils/browser.js';
import * as Url from './utils/url.js';
import computedStyle from './utils/get-computed-style.js';
import extendFn from './extend.js';
import merge from 'lodash-compat/object/merge';
import xhr from 'xhr';
Expand Down Expand Up @@ -719,6 +720,20 @@ videojs.appendContent = Dom.appendContent;
*/
videojs.insertContent = Dom.insertContent;

/**
* A safe getComputedStyle with an IE8 fallback.
*
* This is because in Firefox, if the player is loaded in an iframe with `display:none`,
* then `getComputedStyle` returns `null`, so, we do a null-check to make sure
* that the player doesn't break in these cases.
* See https://bugzilla.mozilla.org/show_bug.cgi?id=548397 for more details.
*
* @function getComputedStyle
* @param el the element you want the computed style of
* @param prop the property name you want
*/
videojs.getComputedStyle = computedStyle;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to call it getComputedStyle? Should it be getComputedStyle throughout the code rather than computedStyle?

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 it should be computedStyle so we don't confuse it with the global function.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll do that.


/*
* Custom Universal Module Definition (UMD)
*
Expand Down