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

Ignore null, undefined, and NaN is dimension() #1449

Merged
merged 2 commits into from Sep 2, 2014

Conversation

Projects
None yet
2 participants
@gkatsev
Member

gkatsev commented Aug 25, 2014

Add vjs.isNaN to have a better cross browser isNaN checker.
Previously, only undefined was ignored, so, it tried setting the
dimension using null and NaN as values. In most browsers this isn't a
problem, but in particular on IE8, things break.
With this PR, all three of those values will be ignored.

Ignore null, undefined, and NaN is dimension()
Add vjs.isNaN to have a better cross browser isNaN checker.
Previously, only undefined was ignored, so, it tried setting the
dimension using null and NaN as values. In most browsers this isn't a
problem, but in particular on IE8, things break.
With this PR, all three of those values will be ignored.
@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Aug 25, 2014

Member

What I think we might want here is some type checking and errors? The undefined check there is just to choose between the setter and getter operations, not a type check. By adding null and isNaN checks there we'd be returning the dimension for those values instead of allow them to error however they would.

What was trying to set null and isNaN for the dimension?

Member

heff commented Aug 25, 2014

What I think we might want here is some type checking and errors? The undefined check there is just to choose between the setter and getter operations, not a type check. By adding null and isNaN checks there we'd be returning the dimension for those values instead of allow them to error however they would.

What was trying to set null and isNaN for the dimension?

@heff heff added the enhancement label Aug 25, 2014

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 26, 2014

Member

I rememer having issues with null previously. The NaN was coming from IE8.
But yeah, you are right that maybe we want better type checking around. I guess the question is whether we want any non-valid values to make this count as a getter rather than a setter.

Member

gkatsev commented Aug 26, 2014

I rememer having issues with null previously. The NaN was coming from IE8.
But yeah, you are right that maybe we want better type checking around. I guess the question is whether we want any non-valid values to make this count as a getter rather than a setter.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Aug 26, 2014

Member

I don't think we want a value other than undefined to count as a getter. It's the difference between prop() and prop(something).

What was causing IE8 to pass NaN as a dimension? No matter what we add here it's still just protection against code that's passing in bad values that it shouldn't be, and probably doesn't mean to be. If it's something in video.js that's trying to set NaN as a dimension, it's more important that we fix that first.

Our guide for how this should be handled is the video element API. It looks like if you set a width of NaN or null on the video element, it just changes the value to zero. No error thrown.
http://jsbin.com/deqoz/1

Member

heff commented Aug 26, 2014

I don't think we want a value other than undefined to count as a getter. It's the difference between prop() and prop(something).

What was causing IE8 to pass NaN as a dimension? No matter what we add here it's still just protection against code that's passing in bad values that it shouldn't be, and probably doesn't mean to be. If it's something in video.js that's trying to set NaN as a dimension, it's more important that we fix that first.

Our guide for how this should be handled is the video element API. It looks like if you set a width of NaN or null on the video element, it just changes the value to zero. No error thrown.
http://jsbin.com/deqoz/1

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 26, 2014

Member

While the code that was calling should definitely be fixed, videojs's code shouldn't fail hard when it gets bad input. This was a quick attempt to mitigate that.

Member

gkatsev commented Aug 26, 2014

While the code that was calling should definitely be fixed, videojs's code shouldn't fail hard when it gets bad input. This was a quick attempt to mitigate that.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Aug 26, 2014

Member

videojs's code shouldn't fail hard when it gets bad input

No, you're right. I wasn't trying to ignore that part so sorry if that's how it came across. Would you want to update this to match how the video element works? And do you think that's a good way to make it work?

Member

heff commented Aug 26, 2014

videojs's code shouldn't fail hard when it gets bad input

No, you're right. I wasn't trying to ignore that part so sorry if that's how it came across. Would you want to update this to match how the video element works? And do you think that's a good way to make it work?

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 2, 2014

Member

Would you be interested in updating this today and getting it into the release?

Member

heff commented Sep 2, 2014

Would you be interested in updating this today and getting it into the release?

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Sep 2, 2014

Member

Working on it.

Member

gkatsev commented Sep 2, 2014

Working on it.

NaN & null should be treated as 0 in dimension()
When dimension() receives a NaN or null value for the number, it will
treat it as zero.
Updated tests to reflect that as well.
@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Sep 2, 2014

Member

@heff updated.

Member

gkatsev commented Sep 2, 2014

@heff updated.

@heff heff merged commit df1944e into videojs:master Sep 2, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

heff added a commit that referenced this pull request Sep 2, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 2, 2014

Member

thanks!

Member

heff commented Sep 2, 2014

thanks!

@gkatsev gkatsev deleted the gkatsev:the-N-Zone branch Aug 12, 2015

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