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 audio preload test on iOS #1702

Merged
merged 2 commits into from Oct 28, 2015

Conversation

Projects
None yet
3 participants
@fonziemedia
Contributor

fonziemedia commented Oct 5, 2015

The audio preload test should now fail on iOS

Fix audio preload test on iOS
The audio preload test should now fail on iOS
@modernizr-savage

This comment has been minimized.

Show comment
Hide comment
@modernizr-savage

modernizr-savage Oct 5, 2015

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 8676c5d
Build details: https://travis-ci.org/modernizr-savage/Modernizr/builds/83800200

(Please note that this is a fully automated comment.)

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 8676c5d
Build details: https://travis-ci.org/modernizr-savage/Modernizr/builds/83800200

(Please note that this is a fully automated comment.)

Show outdated Hide outdated feature-detects/audio/preload.js
var elemStyle = elem.style;
function testpreload(arg) {
clearTimeout(timeout);

This comment has been minimized.

@ryanseddon

ryanseddon Oct 6, 2015

Member

Is this necessary a timeout will only run once

@ryanseddon

ryanseddon Oct 6, 2015

Member

Is this necessary a timeout will only run once

This comment has been minimized.

@fonziemedia

fonziemedia Oct 6, 2015

Contributor

It takes a while for the event listener to trigger and I thought cancelling the timeout here skips the (up to) 300ms damage. So if a browser supports audio pre loading and it's fast the timeout gets cancelled as soon as the event triggers. Or am I misinterpreting how the clearTimout function works?

@fonziemedia

fonziemedia Oct 6, 2015

Contributor

It takes a while for the event listener to trigger and I thought cancelling the timeout here skips the (up to) 300ms damage. So if a browser supports audio pre loading and it's fast the timeout gets cancelled as soon as the event triggers. Or am I misinterpreting how the clearTimout function works?

This comment has been minimized.

@ryanseddon

ryanseddon Oct 6, 2015

Member

Yes I see now that you have the event listener, no need to change this.

@ryanseddon

ryanseddon Oct 6, 2015

Member

Yes I see now that you have the event listener, no need to change this.

Show outdated Hide outdated feature-detects/audio/preload.js
var elem = createElement('audio');
var elemStyle = elem.style;
function testpreload(arg) {

This comment has been minimized.

@ryanseddon

ryanseddon Oct 6, 2015

Member

Better of calling this argument what it is event

@ryanseddon

ryanseddon Oct 6, 2015

Member

Better of calling this argument what it is event

This comment has been minimized.

@fonziemedia

fonziemedia Oct 6, 2015

Contributor

yeah makes sense!

@fonziemedia

fonziemedia Oct 6, 2015

Contributor

yeah makes sense!

Show outdated Hide outdated feature-detects/audio/preload.js
function testpreload(arg) {
clearTimeout(timeout);
var result = arg !== undefined && arg.type === 'loadeddata' ? true : false; //to avoid arg being 'undefined' and arg.type throwing out an error, see below comment

This comment has been minimized.

@ryanseddon

ryanseddon Oct 6, 2015

Member

You can simplify this:

var result = arg && arg.type === 'loadeddata';
@ryanseddon

ryanseddon Oct 6, 2015

Member

You can simplify this:

var result = arg && arg.type === 'loadeddata';

This comment has been minimized.

@fonziemedia

fonziemedia Oct 6, 2015

Contributor

I'll have a look but won't that throw 'undefined' on the first argument and an error on the second if the function is triggered on timeout (iOS)?

@fonziemedia

fonziemedia Oct 6, 2015

Contributor

I'll have a look but won't that throw 'undefined' on the first argument and an error on the second if the function is triggered on timeout (iOS)?

This comment has been minimized.

@ryanseddon

ryanseddon Oct 6, 2015

Member

Yep you're right ignore me

@ryanseddon

ryanseddon Oct 6, 2015

Member

Yep you're right ignore me

Modernizr.addAsyncTest(function() {
var timeout;
var waitTime = 300;
var elem = createElement('audio');

This comment has been minimized.

@ryanseddon

ryanseddon Oct 6, 2015

Member

Would new Audio() work the same rather than creating an element?

@ryanseddon

ryanseddon Oct 6, 2015

Member

Would new Audio() work the same rather than creating an element?

This comment has been minimized.

@fonziemedia

fonziemedia Oct 6, 2015

Contributor

Yes I prefer new Audio too and `elem.preload = 'auto' but wasn't sure if using it would break things (used the video/autoplay.js as a 'template')

@fonziemedia

fonziemedia Oct 6, 2015

Contributor

Yes I prefer new Audio too and `elem.preload = 'auto' but wasn't sure if using it would break things (used the video/autoplay.js as a 'template')

This comment has been minimized.

@ryanseddon

ryanseddon Oct 6, 2015

Member

Ok that makes sense to keep it consistent you're cleaning up the element anyway so it should be fine.

@ryanseddon

ryanseddon Oct 6, 2015

Member

Ok that makes sense to keep it consistent you're cleaning up the element anyway so it should be fine.

Show outdated Hide outdated feature-detects/audio/preload.js
"tags": ["audio", "media"],
"async" : true,
"warnings": ["This test is very large – only include it if you absolutely need it"],
"knownBugs": ["Not consistent in Safari and IE when run in Cow tests"]

This comment has been minimized.

@ryanseddon

ryanseddon Oct 6, 2015

Member

Not sure if this is necessary to mention that our test suite could give the incorrect results. Seems like we may need to fix that.

@ryanseddon

ryanseddon Oct 6, 2015

Member

Not sure if this is necessary to mention that our test suite could give the incorrect results. Seems like we may need to fix that.

@fonziemedia

This comment has been minimized.

Show comment
Hide comment
@fonziemedia

fonziemedia Oct 6, 2015

Contributor

Thanks for reviewing the code @ryanseddon I'll see if I can work on your suggestions tonight. Should I submit a new request or maybe edit this one? Also, what's the process going forward, will the PR be going through more testing before it's added to the master branch? Cheers

Contributor

fonziemedia commented Oct 6, 2015

Thanks for reviewing the code @ryanseddon I'll see if I can work on your suggestions tonight. Should I submit a new request or maybe edit this one? Also, what's the process going forward, will the PR be going through more testing before it's added to the master branch? Cheers

@ryanseddon

This comment has been minimized.

Show comment
Hide comment
@ryanseddon

ryanseddon Oct 6, 2015

Member

Edit this one and once it's good to go you can squash the commits into one meaniful one.

Member

ryanseddon commented Oct 6, 2015

Edit this one and once it's good to go you can squash the commits into one meaniful one.

Fix audio preload on iOS (reviewed)
-removed  "knownBugs" as test suite is currently being debugged;
-changed testpreload function parameter name to 'event';
-cleared/reviewed some comments.
@fonziemedia

This comment has been minimized.

Show comment
Hide comment
@fonziemedia

fonziemedia Oct 7, 2015

Contributor

Just pushed the amendments @ryanseddon

Cheers

Contributor

fonziemedia commented Oct 7, 2015

Just pushed the amendments @ryanseddon

Cheers

@modernizr-savage

This comment has been minimized.

Show comment
Hide comment
@modernizr-savage

modernizr-savage Oct 7, 2015

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 055ab50
Build details: https://travis-ci.org/modernizr-savage/Modernizr/builds/84158033

(Please note that this is a fully automated comment.)

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 055ab50
Build details: https://travis-ci.org/modernizr-savage/Modernizr/builds/84158033

(Please note that this is a fully automated comment.)

ryanseddon added a commit that referenced this pull request Oct 28, 2015

Merge pull request #1702 from fonziemedia/master
Fix audio preload test on iOS

@ryanseddon ryanseddon merged commit e5e8002 into Modernizr:master Oct 28, 2015

2 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ryanseddon

This comment has been minimized.

Show comment
Hide comment
@ryanseddon

ryanseddon Oct 28, 2015

Member

Thanks @fonziemedia sorry for the slow merge

Member

ryanseddon commented Oct 28, 2015

Thanks @fonziemedia sorry for the slow merge

@fonziemedia

This comment has been minimized.

Show comment
Hide comment
@fonziemedia

fonziemedia Oct 30, 2015

Contributor

That's great @ryanseddon :) thanks for the update. I think there might be improvements to be done on the video preload test following this. I'll see if I can get some free time in the next weeks to have a look. Cheers!

Contributor

fonziemedia commented Oct 30, 2015

That's great @ryanseddon :) thanks for the update. I think there might be improvements to be done on the video preload test following this. I'll see if I can get some free time in the next weeks to have a look. Cheers!

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