fix(fullscreen): handleMediaUpdated / rootNode race condition (#817)

fixes epicweb-dev/epicshop#189

the async logic in handleMediaUpdated was causing rootNode to be
undefined when trying to add event listeners.

this bug was only apparent when rapidly connecting / disconnecting media
like in many modern JS frameworks

.github/workflows/cd.yml

@@ -47,13 +47,12 @@ jobs:
       CONVENTIONAL_GITHUB_RELEASER_TOKEN: ${{ secrets.GITHUB_TOKEN }}
 
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
         with:
           fetch-depth: 0 # Fetch all history for all tags and branches
-      - name: Use Node.js ${{ matrix.node-version }}
-        uses: actions/setup-node@v3
+      - uses: actions/setup-node@v4
         with:
-          node-version: ${{ matrix.node-version }}
+          node-version: 20
           # this line is required for the setup-node action to be able to run the npm publish below.
           registry-url: 'https://registry.npmjs.org'
       - uses: fregante/setup-git-user@v1

.github/workflows/ci.yml

@@ -7,11 +7,10 @@ jobs:
     runs-on: ubuntu-latest
 
     steps:
-      - uses: actions/checkout@v3
-      - name: Use Node.js ${{ matrix.node-version }}
-        uses: actions/setup-node@v3
+      - uses: actions/checkout@v4
+      - uses: actions/setup-node@v4
         with:
-          node-version: ${{ matrix.node-version }}
+          node-version: 20
           cache: yarn
           # this line is required for the setup-node action to be able to run the npm publish below.
           registry-url: 'https://registry.npmjs.org'

package.json

@@ -93,6 +93,9 @@
     "sinon": "^14.0.1",
     "typescript": "^4.8.4"
   },
+  "resolutions": {
+    "source-map": "^0.7.4"
+  },
   "prettier": {
     "trailingComma": "es5",
     "tabWidth": 2,

src/js/media-container.js

@@ -324,10 +324,9 @@ class MediaContainer extends globalThis.HTMLElement {
           // No need to inject anything if media=null
           if (media) {
             mutation.addedNodes.forEach((node) => {
-              if (node === media && media !== this.#currentMedia) {
+              if (node === media) {
                 // Update all controls with new media if this is the new media
-                this.handleMediaUpdated(media)
-                  .then((media) => this.mediaSetCallback(media));
+                this.handleMediaUpdated(media);
               }
             });
           }
@@ -377,10 +376,7 @@ class MediaContainer extends globalThis.HTMLElement {
           }
           return;
         }
-        if (this.media && this.media !== this.#currentMedia) {
-          this.handleMediaUpdated(this.media)
-            .then((media) => this.mediaSetCallback(media));
-        }
+        this.handleMediaUpdated(this.media);
       });
     }
   }
@@ -419,29 +415,21 @@ class MediaContainer extends globalThis.HTMLElement {
    * @param {HTMLMediaElement} media
    */
   async handleMediaUpdated(media) {
-    this.#currentMedia = media;
-
-    const rejectMediaPromise = (media) => {
-      this.#currentMedia = null;
+    // Anything "falsy" couldn't act as a media element.
+    if (!media) return;
 
-      console.error(
-        'Media Chrome: Media element set with slot="media" does not appear to be compatible.',
-        media
-      );
-      return Promise.reject(media);
-    };
-
-    // Anything "falsy" couldn't act as a media element. Reject.
-    if (!media) {
-      return rejectMediaPromise(media);
-    }
+    this.#currentMedia = media;
 
     // Custom element. Wait until it's defined before resolving
     if (media.localName.includes('-')) {
       await globalThis.customElements.whenDefined(media.localName);
     }
 
-    return media;
+    // Even if we are not connected to the DOM after this await still call mediaSetCallback
+    // so the media state is already computed once, then when the container is connected
+    // to the DOM mediaSetCallback is called again to attach the root node event listeners.
+
+    this.mediaSetCallback(media);
   }
 
   connectedCallback() {
@@ -450,10 +438,7 @@ class MediaContainer extends globalThis.HTMLElement {
     this.setAttribute('role', 'region');
     this.setAttribute('aria-label', label);
 
-    if (this.media && this.media !== this.#currentMedia) {
-      this.handleMediaUpdated(this.media)
-        .then((media) => this.mediaSetCallback(media));
-    }
+    this.handleMediaUpdated(this.media);
 
     // Assume user is inactive until they're not (aka userinactive by default is true)
     // This allows things like autoplay and programmatic playing to also initiate hiding controls (CJP)
@@ -485,10 +470,11 @@ class MediaContainer extends globalThis.HTMLElement {
   mediaSetCallback(media) {} // eslint-disable-line
 
   /**
-   * @abstract
    * @param {HTMLMediaElement} media
    */
-  mediaUnsetCallback(media) {} // eslint-disable-line
+  mediaUnsetCallback(media) { // eslint-disable-line
+    this.#currentMedia = null;
+  }
 
   handleEvent(event) {
     switch (event.type) {

src/js/media-controller.js

@@ -101,7 +101,7 @@ class MediaController extends MediaContainer {
         e.stopPropagation();
 
         if (!this.media) {
-          console.warn('MediaController: No media available.');
+          console.warn('Media Chrome: No media available.');
           return;
         }
 
@@ -136,7 +136,7 @@ class MediaController extends MediaContainer {
     if (attrName === Attributes.NO_HOTKEYS) {
       if (newValue !== oldValue && newValue === '') {
         if (this.hasAttribute(Attributes.HOTKEYS)) {
-          console.warn('Both `hotkeys` and `nohotkeys` have been set. All hotkeys will be disabled.');
+          console.warn('Media Chrome: Both `hotkeys` and `nohotkeys` have been set. All hotkeys will be disabled.');
         }
         this.disableHotkeys();
 
@@ -170,14 +170,14 @@ class MediaController extends MediaContainer {
   }
 
   connectedCallback() {
-    // mediaSetCallback() is called in super.connectedCallback();
-    super.connectedCallback();
-
     // getRootNode() in disconnectedCallback returns the media-controller element itself
     // but we need the HTMLDocument or ShadowRoot if media-controller is in a shadow DOM.
     // We store the correct root node here so we can access it later.
     this.#rootNode = /** @type HTMLDocument | ShadowRoot */ (this.getRootNode());
 
+    // mediaSetCallback() is called in super.connectedCallback();
+    super.connectedCallback();
+
     this.enableHotkeys();
   }
 
@@ -267,7 +267,7 @@ class MediaController extends MediaContainer {
         const volPref = globalThis.localStorage.getItem('media-chrome-pref-volume');
         if (volPref !== null) media.volume = volPref;
       } catch (e) {
-        console.debug('Error getting volume pref', e);
+        console.debug('Media Chrome: Error getting volume pref', e);
       }
     }
   }

yarn.lock

@@ -3145,10 +3145,10 @@ slice-ansi@^4.0.0:
     astral-regex "^2.0.0"
     is-fullwidth-code-point "^3.0.0"
 
-source-map@^0.7.3:
-  version "0.7.3"
-  resolved "https://registry.npmjs.org/source-map/-/source-map-0.7.3.tgz"
-  integrity sha512-CkCj6giN3S+n9qrYiBTX5gystlENnRW5jZeNLHpe6aue+SrHcG5VYwujhW9s4dY31mEGsxBDrHR6oI69fTXsaQ==
+source-map@^0.7.3, source-map@^0.7.4:
+  version "0.7.4"
+  resolved "https://registry.yarnpkg.com/source-map/-/source-map-0.7.4.tgz#a9bbe705c9d8846f4e08ff6765acf0f1b0898656"
+  integrity sha512-l3BikUxvPOcn5E74dZiq5BGsTb5yEwhaTSzccU6t4sDOH8NWJCstKO5QT2CvtFoK6F0saL7p9xHAqHOlCPJygA==
 
 spdx-correct@^3.0.0:
   version "3.1.1"