fix: throw error on muted resolution rejection during autoplay #7293
Conversation
|
💖 Thanks for opening this pull request! 💖 Things that will help get your PR across the finish line:
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. |
Codecov Report
@@ Coverage Diff @@
## main #7293 +/- ##
=======================================
Coverage 79.44% 79.45%
=======================================
Files 115 115
Lines 7264 7266 +2
Branches 1745 1746 +1
=======================================
+ Hits 5771 5773 +2
Misses 1493 1493
Continue to review full report at Codecov.
|
|
The test does catch this. Without wrapping the |
| fn(); | ||
| } catch (err) { | ||
| return this; | ||
| } | ||
| return this; |
There was a problem hiding this comment.
Maybe this (line 46) should return this.resolvePromise here instead of this? I believe that way if a catch() throws it will trigger the next catch, and if it doesn't throw it will trigger the next then().
There was a problem hiding this comment.
Returning this.resolvePromise here changes the expectations of the rejectPromise, this may end up requiring a lot of other test changes that may not be worth it right now.
| return mutedPromise.catch(restoreMuted); | ||
| return mutedPromise.catch(err => { | ||
| restoreMuted(); | ||
| throw new Error(`Rejection at manualAutoplay. Restoring muted value. ${err ? err : ''}`); |
There was a problem hiding this comment.
If I remove this line that throws the error, I would expect these tests to fail since the promise on line 1479 should resolve and result in an 'autoplay-success' event and increment this.counts.success rather than this.counts.failure, however the test still passes which suggests it isn't capturing the test case exactly. It might be necessary to modify this.rejectPromise for these tests. I've shared one idea in another comment but I'm not positive it will work.
There was a problem hiding this comment.
Maybe what we need is a separate promise fake for these two tests?
There was a problem hiding this comment.
I tried it locally, and I think it might be too complicated to get it in now.
There was a problem hiding this comment.
In that case how about adding a TODO comment to address the false positive issue with these two tests?
There was a problem hiding this comment.
Spent some time playing around with sinon's promises, and probably needs a complete rewrite of the test to use that.
I'll create an issue for this.
|
Congrats on merging your first pull request! 🎉🎉🎉 |
…js#7293) Previously, when autoplay was set to any or muted, we would accidentally swallow the autoplay rejection when we reset the muted state back to what it was. Instead, we want to re-throw the error. To get it working, we also had to update our tests to try/catch in our fake promise.
Description
When autoplay is set to
anyormutedand muted() returnsfalse, video.js attempts the following workflow:autoplay-successtrueautoplay-successautoplay-failureevent would be triggered in the final .catch.Since we .catch at multiple points along the promise chain, it's possible for step 4 to occur while still resolving to the following .then method, incorrectly triggering an
autoplay-successevent, instead of the expectedautoplay-failure.Specific Changes proposed
autoplay-failureevent to be triggered in the final .catch in the promise chain.Requirements Checklist