Merge pull request #1039 from friday/poster-race-conditions

Fix poster race conditions

6 files changed, 63 insertions, 35 deletions

diff --git a/src/js/plugins/vimeo.js b/src/js/plugins/vimeo.js
index 312d53cf..09339229 100644
--- a/src/js/plugins/vimeo.js
+++ b/src/js/plugins/vimeo.js
@@ -119,8 +119,11 @@ const vimeo = {
         iframe.setAttribute('allowtransparency', '');
         iframe.setAttribute('allow', 'autoplay');
 
+        // Get poster, if already set
+        const { poster } = player;
+
         // Inject the package
-        const wrapper = createElement('div', { class: player.config.classNames.embedContainer });
+        const wrapper = createElement('div', { poster, class: player.config.classNames.embedContainer });
         wrapper.appendChild(iframe);
         player.media = replaceElement(wrapper, player.media);
 
@@ -137,7 +140,7 @@ const vimeo = {
             url.pathname = `${url.pathname.split('_')[0]}.jpg`;
 
             // Set and show poster
-            ui.setPoster.call(player, url.href);
+            ui.setPoster.call(player, url.href).catch(() => {});
         });
 
         // Setup instance
diff --git a/src/js/plugins/youtube.js b/src/js/plugins/youtube.js
index e383e50f..65b6db75 100644
--- a/src/js/plugins/youtube.js
+++ b/src/js/plugins/youtube.js
@@ -158,10 +158,15 @@ const youtube = {
         // Replace the <iframe> with a <div> due to YouTube API issues
         const videoId = parseId(source);
         const id = generateId(player.provider);
-        const container = createElement('div', { id });
+
+        // Get poster, if already set
+        const { poster } = player;
+
+        // Replace media element
+        const container = createElement('div', { id, poster });
         player.media = replaceElement(container, player.media);
 
-        // Set poster image
+        // Id to poster wrapper
         const posterSrc = format => `https://img.youtube.com/vi/${videoId}/${format}default.jpg`;
 
         // Check thumbnail images in order of quality, but reject fallback thumbnails (120px wide)
@@ -174,7 +179,8 @@ const youtube = {
                 if (!posterSrc.includes('maxres')) {
                     player.elements.poster.style.backgroundSize = 'cover';
                 }
-            });
+            })
+            .catch(() => {});
 
         // Setup instance
         // https://developers.google.com/youtube/iframe_api_reference
diff --git a/src/js/plyr.js b/src/js/plyr.js
index 0217ded8..dcbe384b 100644
--- a/src/js/plyr.js
+++ b/src/js/plyr.js
@@ -790,7 +790,7 @@ class Plyr {
             return;
         }
 
-        ui.setPoster.call(this, input);
+        ui.setPoster.call(this, input, false).catch(() => {});
     }
 
     /**
diff --git a/src/js/ui.js b/src/js/ui.js
index 285739a7..5d7a6ae3 100644
--- a/src/js/ui.js
+++ b/src/js/ui.js
@@ -8,7 +8,7 @@ import i18n from './i18n';
 import support from './support';
 import browser from './utils/browser';
 import { getElement, toggleClass, toggleState } from './utils/elements';
-import { triggerEvent } from './utils/events';
+import { ready, triggerEvent } from './utils/events';
 import is from './utils/is';
 import loadImage from './utils/loadImage';
 
@@ -109,8 +109,8 @@ const ui = {
         ui.setTitle.call(this);
 
         // Assure the poster image is set, if the property was added before the element was created
-        if (this.poster && this.elements.poster && !this.elements.poster.style.backgroundImage) {
-            ui.setPoster.call(this, this.poster);
+        if (this.poster) {
+            ui.setPoster.call(this, this.poster, false).catch(() => {});
         }
 
         // Manually set the duration if user has overridden it.
@@ -163,32 +163,43 @@ const ui = {
     },
 
     // Set the poster image (async)
-    setPoster(poster) {
-        // Set property regardless of validity
-        this.media.setAttribute('poster', poster);
-
-        // Bail if element is missing
-        if (!is.element(this.elements.poster)) {
-            return Promise.reject();
+    // Used internally for the poster setter, with the passive option forced to false
+    setPoster(poster, passive = true) {
+        // Don't override if call is passive
+        if (passive && this.poster) {
+            return Promise.reject(new Error('Poster already set'));
         }
 
-        // Load the image, and set poster if successful
-        const loadPromise = loadImage(poster).then(() => {
-            this.elements.poster.style.backgroundImage = `url('${poster}')`;
-            Object.assign(this.elements.poster.style, {
-                backgroundImage: `url('${poster}')`,
-                // Reset backgroundSize as well (since it can be set to "cover" for padded thumbnails for youtube)
-                backgroundSize: '',
-            });
-            ui.togglePoster.call(this, true);
-            return poster;
-        });
-
-        // Hide the element if the poster can't be loaded (otherwise it will just be a black element covering the video)
-        loadPromise.catch(() => ui.togglePoster.call(this, false));
+        // Set property synchronously to respect the call order
+        this.media.setAttribute('poster', poster);
 
-        // Return the promise so the caller can use it as well
-        return loadPromise;
+        // Wait until ui is ready
+        return ready.call(this)
+            // Load image
+            .then(() => loadImage(poster))
+            .catch(err => {
+                // Hide poster on error unless it's been set by another call
+                if (poster === this.poster) {
+                    ui.togglePoster.call(this, false);
+                }
+                // Rethrow
+                throw err;
+            })
+            .then(() => {
+                // Prevent race conditions
+                if (poster !== this.poster) {
+                    throw new Error('setPoster cancelled by later call to setPoster');
+                }
+            })
+            .then(() => {
+                Object.assign(this.elements.poster.style, {
+                    backgroundImage: `url('${poster}')`,
+                    // Reset backgroundSize as well (since it can be set to "cover" for padded thumbnails for youtube)
+                    backgroundSize: '',
+                });
+                ui.togglePoster.call(this, true);
+                return poster;
+            });
     },
 
     // Check playing state
diff --git a/src/js/utils/elements.js b/src/js/utils/elements.js
index 39b944d2..2d314ed8 100644
--- a/src/js/utils/elements.js
+++ b/src/js/utils/elements.js
@@ -42,9 +42,11 @@ export function setAttributes(element, attributes) {
         return;
     }
 
-    Object.entries(attributes).forEach(([key, value]) => {
-        element.setAttribute(key, value);
-    });
+    // Assume null and undefined attributes should be left out,
+    // Setting them would otherwise convert them to "null" and "undefined"
+    Object.entries(attributes)
+        .filter(([, value]) => !is.nullOrUndefined(value))
+        .forEach(([key, value]) => element.setAttribute(key, value));
 }
 
 // Create a DocumentFragment
diff --git a/src/js/utils/events.js b/src/js/utils/events.js
index 1e940c71..9009d1cc 100644
--- a/src/js/utils/events.js
+++ b/src/js/utils/events.js
@@ -111,3 +111,9 @@ export function unbindListeners() {
         this.eventListeners = [];
     }
 }
+
+// Run method when / if player is ready
+export function ready () {
+    return new Promise(resolve => this.ready ? setTimeout(resolve, 0) : on.call(this, this.elements.container, 'ready', resolve))
+        .then(() => {});
+}