From 09b22cf96c6994b45080f2b75108a36e76e47568 Mon Sep 17 00:00:00 2001 From: Mingze Date: Tue, 17 Sep 2019 11:38:01 -0700 Subject: [PATCH] fix(viewer): Pause audio instead of muting it if autoplay is prevented (#1068) * fix(viewer): Pause audio instead of muting it if autoplay is prevented * fix(viewer): clean up * fix(viewer): small change * fix(viewer): avoid duplicating autoplay logic * fix(viewer): fix typo * perf(sidebar): force play to always return a promise * perf(sidebar): remove await async * perf(sidebar): minor changes --- src/lib/viewers/media/MP3Viewer.js | 9 ++++ src/lib/viewers/media/MediaBaseViewer.js | 54 ++++++++++--------- src/lib/viewers/media/VideoBaseViewer.js | 10 ++++ .../viewers/media/__tests__/MP3Viewer-test.js | 8 +++ .../media/__tests__/MediaBaseViewer-test.js | 30 +++++------ .../media/__tests__/VideoBaseViewer-test.js | 12 +++++ 6 files changed, 82 insertions(+), 41 deletions(-) diff --git a/src/lib/viewers/media/MP3Viewer.js b/src/lib/viewers/media/MP3Viewer.js index 6f35a3dc6..9bdcd4047 100644 --- a/src/lib/viewers/media/MP3Viewer.js +++ b/src/lib/viewers/media/MP3Viewer.js @@ -34,6 +34,15 @@ class MP3Viewer extends MediaBaseViewer { this.mediaControls.show(); this.mediaControls.resizeTimeScrubber(); } + + /** + * Auto-play was prevented, pause the audio + * + * @override + */ + handleAutoplayFail = () => { + this.pause(); + }; } export default MP3Viewer; diff --git a/src/lib/viewers/media/MediaBaseViewer.js b/src/lib/viewers/media/MediaBaseViewer.js index 0a823442e..0d2f48df7 100644 --- a/src/lib/viewers/media/MediaBaseViewer.js +++ b/src/lib/viewers/media/MediaBaseViewer.js @@ -19,6 +19,7 @@ const TIMESTAMP_UNIT_NAME = 'timestamp'; const INITIAL_TIME_IN_SECONDS = 0; const ONE_MINUTE_IN_SECONDS = 60; const ONE_HOUR_IN_SECONDS = 60 * ONE_MINUTE_IN_SECONDS; +const PLAY_PROMISE_NOT_SUPPORTED = 'play_promise_not_supported'; class MediaBaseViewer extends BaseViewer { /** @@ -303,35 +304,28 @@ class MediaBaseViewer extends BaseViewer { } /** - * Determines if media should autoplay based on cached settings value. + * Handler for autoplay failure + * Overridden in child class + * + * @protected + */ + handleAutoplayFail = () => {}; + + /** + * Autoplay the media * * @private - * @emits volume * @return {Promise} */ autoplay() { - // Play may return a promise depending on browser support. This promise - // will resolve when playback starts. If it fails, we mute the video - // and try to play again. - // https://webkit.org/blog/7734/auto-play-policy-changes-for-macos/ - const autoPlayPromise = this.mediaEl.play(); - - if (autoPlayPromise && typeof autoPlayPromise.then === 'function') { - this.handleRate(); - return autoPlayPromise - .then(() => { - this.handleVolume(); - }) - .catch(() => { - // Auto-play was prevented, try muted play - this.setVolume(0); - this.mediaEl.play(); - }); - } - - // Fallback to traditional autoplay tag if play does not return a promise - this.mediaEl.autoplay = true; - return Promise.resolve(); + return this.play().catch(error => { + if (error.message === PLAY_PROMISE_NOT_SUPPORTED) { + // Fallback to traditional autoplay tag if mediaEl.play does not return a promise + this.mediaEl.autoplay = true; + } else { + this.handleAutoplayFail(); + } + }); } /** @@ -570,7 +564,7 @@ class MediaBaseViewer extends BaseViewer { * @param {number} start - start time in seconds * @param {number} end - end time in seconds * @emits play - * @return {void} + * @return {Promise} */ play(start, end) { const hasValidStart = this.isValidTime(start); @@ -584,10 +578,18 @@ class MediaBaseViewer extends BaseViewer { this.setMediaTime(start); } if (arguments.length === 0 || hasValidStart) { - this.mediaEl.play(); + // Play may return a promise depending on browser support. This promise + // will resolve when playback starts. + // https://webkit.org/blog/7734/auto-play-policy-changes-for-macos/ + const playPromise = this.mediaEl.play(); this.handleRate(); this.handleVolume(); + + return playPromise && typeof playPromise.then === 'function' + ? playPromise + : Promise.reject(new Error(PLAY_PROMISE_NOT_SUPPORTED)); } + return Promise.resolve(); } /** diff --git a/src/lib/viewers/media/VideoBaseViewer.js b/src/lib/viewers/media/VideoBaseViewer.js index 9e57b2895..543fa48a7 100644 --- a/src/lib/viewers/media/VideoBaseViewer.js +++ b/src/lib/viewers/media/VideoBaseViewer.js @@ -234,6 +234,16 @@ class VideoBaseViewer extends MediaBaseViewer { onKeydown(key) { return super.onKeydown(key); } + + /** + * Auto-play was prevented, try muted play + * + * @override + */ + handleAutoplayFail = () => { + this.setVolume(0); + this.play().catch(this.pause); + }; } export default VideoBaseViewer; diff --git a/src/lib/viewers/media/__tests__/MP3Viewer-test.js b/src/lib/viewers/media/__tests__/MP3Viewer-test.js index e38edbc1e..e8ee67d85 100644 --- a/src/lib/viewers/media/__tests__/MP3Viewer-test.js +++ b/src/lib/viewers/media/__tests__/MP3Viewer-test.js @@ -74,4 +74,12 @@ describe('lib/viewers/media/MP3Viewer', () => { mp3.loadUI(); }); }); + + describe('handleAutoplayFail()', () => { + it('should call pause', () => { + sandbox.stub(mp3, 'pause'); + mp3.handleAutoplayFail(); + expect(mp3.pause).to.be.called; + }); + }); }); diff --git a/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js b/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js index 21900ac75..6216d8bbb 100644 --- a/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js +++ b/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js @@ -272,32 +272,32 @@ describe('lib/viewers/media/MediaBaseViewer', () => { }); describe('autoplay()', () => { - beforeEach(() => { - media.mediaEl = { - play: sandbox.stub().returns(Promise.resolve()), - }; + const PLAY_PROMISE_NOT_SUPPORTED = 'play_promise_not_supported'; - sandbox.stub(media, 'isAutoplayEnabled').returns(true); - sandbox.stub(Browser, 'isIOS').returns(false); + beforeEach(() => { + media.mediaEl = {}; + media.play = sandbox.stub().returns(Promise.resolve()); }); it('should set autoplay if setting is enabled and handle the promise if it is a valid promise', () => { media.autoplay(); - expect(media.mediaEl.play).to.be.called; + expect(media.play).to.be.called; expect(media.mediaEl.autoplay).to.be.undefined; }); - it('should set autoplay to true if play does not return a promise', () => { - media.mediaEl.play.returns(undefined); - media.autoplay(); - expect(media.mediaEl.autoplay).to.be.true; + it('should set autoplay to true if mediaEl.play does not return a promise', done => { + media.play.returns(Promise.reject(new Error(PLAY_PROMISE_NOT_SUPPORTED))); + media.autoplay().then(() => { + expect(media.mediaEl.autoplay).to.be.true; + done(); + }); }); - it('should muted autoplay if the promise is rejected', done => { - sandbox.stub(media, 'setVolume'); - media.mediaEl.play = sandbox.stub().returns(Promise.reject()); + it('should call handleAutoplayFail if the promise is rejected', done => { + sandbox.stub(media, 'handleAutoplayFail'); + media.play.returns(Promise.reject(new Error('NotAllowedError'))); media.autoplay().then(() => { - expect(media.setVolume).to.be.calledWith(0); + expect(media.handleAutoplayFail).to.be.called; done(); }); }); diff --git a/src/lib/viewers/media/__tests__/VideoBaseViewer-test.js b/src/lib/viewers/media/__tests__/VideoBaseViewer-test.js index ff32c51bc..3ffd111bf 100644 --- a/src/lib/viewers/media/__tests__/VideoBaseViewer-test.js +++ b/src/lib/viewers/media/__tests__/VideoBaseViewer-test.js @@ -244,4 +244,16 @@ describe('lib/viewers/media/VideoBaseViewer', () => { expect(videoBase.rootEl.classList.contains('bp-dark')).to.be.true; }); }); + + describe('handleAutoplayFail()', () => { + it('should mute and play again', () => { + sandbox.stub(videoBase, 'setVolume'); + videoBase.play = sandbox.stub().returns(Promise.reject()); + + videoBase.handleAutoplayFail(); + + expect(videoBase.setVolume).to.be.calledWith(0); + expect(videoBase.play).to.be.called; + }); + }); });