♻️ Consolidate players as .i-amphtml-media-component (#26967)

- `.i-amphtml-video-component` is gone, using existing `.i-amphtml-video-interface` instead. - All media (audio/video) components are now `.i-amphtml-media-component`. Note: video players that add `.i-amphtml-media-component` independently do NOT implement the `VideoInterface` and therefore do not subscribe to the manager.
ampproject · Feb 26, 2020 · f930778 · f930778
1 parent a18848c
commit f930778
Show file tree

Hide file tree

Showing 15 changed files with 67 additions and 5 deletions.
diff --git a/css/video-autoplay.css b/css/video-autoplay.css
@@ -35,7 +35,7 @@ i-amphtml-video-mask {
 }
 
 amp-video[controls] .amp-video-eq,
-.i-amphtml-video-component:not(amp-video) .amp-video-eq,
+.i-amphtml-video-interface:not(amp-video) .amp-video-eq,
 amp-story .amp-video-eq {
   display: flex;
 }

diff --git a/extensions/amp-audio/0.1/amp-audio.js b/extensions/amp-audio/0.1/amp-audio.js
@@ -27,6 +27,7 @@ import {closestAncestorElementBySelector} from '../../../src/dom';
 import {dev, user} from '../../../src/log';
 import {getMode} from '../../../src/mode';
 import {listen} from '../../../src/event-helper';
+import {setIsMediaComponent} from '../../../src/video-interface';
 import {triggerAnalyticsEvent} from '../../../src/analytics';
 
 const TAG = 'amp-audio';
@@ -63,6 +64,8 @@ export class AmpAudio extends AMP.BaseElement {
       this.buildAudioElement();
     }
 
+    setIsMediaComponent(this.element);
+
     this.registerAction('play', this.play_.bind(this));
     this.registerAction('pause', this.pause_.bind(this));
   }

diff --git a/extensions/amp-connatix-player/0.1/amp-connatix-player.js b/extensions/amp-connatix-player/0.1/amp-connatix-player.js
@@ -20,6 +20,7 @@ import {dict} from '../../../src/utils/object';
 import {getData} from '../../../src/event-helper';
 import {isLayoutSizeDefined} from '../../../src/layout';
 import {removeElement} from '../../../src/dom';
+import {setIsMediaComponent} from '../../../src/video-interface';
 import {userAssert} from '../../../src/log';
 
 export class AmpConnatixPlayer extends AMP.BaseElement {
@@ -93,6 +94,8 @@ export class AmpConnatixPlayer extends AMP.BaseElement {
   buildCallback() {
     const {element} = this;
 
+    setIsMediaComponent(element);
+
     // Player id is mandatory
     this.playerId_ = userAssert(
       element.getAttribute('data-player-id'),

diff --git a/extensions/amp-hulu/0.1/amp-hulu.js b/extensions/amp-hulu/0.1/amp-hulu.js
@@ -18,6 +18,7 @@ import {Services} from '../../../src/services';
 import {devAssert, userAssert} from '../../../src/log';
 import {isLayoutSizeDefined} from '../../../src/layout';
 import {removeElement} from '../../../src/dom';
+import {setIsMediaComponent} from '../../../src/video-interface';
 
 class AmpHulu extends AMP.BaseElement {
   /** @param {!AmpElement} element */
@@ -74,6 +75,8 @@ class AmpHulu extends AMP.BaseElement {
 
   /** @override */
   buildCallback() {
+    setIsMediaComponent(this.element);
+
     this.eid_ = userAssert(
       this.element.getAttribute('data-eid'),
       'The data-eid attribute is required for <amp-hulu> %s',

diff --git a/extensions/amp-izlesene/0.1/amp-izlesene.js b/extensions/amp-izlesene/0.1/amp-izlesene.js
@@ -20,6 +20,7 @@ import {devAssert, userAssert} from '../../../src/log';
 import {dict} from '../../../src/utils/object';
 import {getDataParamsFromAttributes} from '../../../src/dom';
 import {isLayoutSizeDefined} from '../../../src/layout';
+import {setIsMediaComponent} from '../../../src/video-interface';
 
 class AmpIzlesene extends AMP.BaseElement {
   /**
@@ -60,6 +61,8 @@ class AmpIzlesene extends AMP.BaseElement {
 
   /** @override */
   buildCallback() {
+    setIsMediaComponent(this.element);
+
     this.videoid_ = userAssert(
       this.element.getAttribute('data-videoid'),
       'The data-videoid attribute is required for <amp-izlesene> %s',

diff --git a/extensions/amp-jwplayer/0.1/amp-jwplayer.js b/extensions/amp-jwplayer/0.1/amp-jwplayer.js
@@ -19,6 +19,7 @@ import {addParamsToUrl} from '../../../src/url';
 import {dict} from '../../../src/utils/object';
 import {isLayoutSizeDefined} from '../../../src/layout';
 import {removeElement} from '../../../src/dom';
+import {setIsMediaComponent} from '../../../src/video-interface';
 import {userAssert} from '../../../src/log';
 
 class AmpJWPlayer extends AMP.BaseElement {
@@ -74,6 +75,8 @@ class AmpJWPlayer extends AMP.BaseElement {
 
   /** @override */
   buildCallback() {
+    setIsMediaComponent(this.element);
+
     const {element} = this;
     this.contentid_ = userAssert(
       element.getAttribute('data-playlist-id') ||

diff --git a/extensions/amp-kaltura-player/0.1/amp-kaltura-player.js b/extensions/amp-kaltura-player/0.1/amp-kaltura-player.js
@@ -19,6 +19,7 @@ import {addParamsToUrl} from '../../../src/url';
 import {dict} from '../../../src/utils/object';
 import {getDataParamsFromAttributes} from '../../../src/dom';
 import {isLayoutSizeDefined} from '../../../src/layout';
+import {setIsMediaComponent} from '../../../src/video-interface';
 import {userAssert} from '../../../src/log';
 
 class AmpKaltura extends AMP.BaseElement {
@@ -61,6 +62,8 @@ class AmpKaltura extends AMP.BaseElement {
       this.element
     );
 
+    setIsMediaComponent(this.element);
+
     this.entryId_ = this.element.getAttribute('data-entryid') || 'default';
   }
 

diff --git a/extensions/amp-megaphone/0.1/amp-megaphone.js b/extensions/amp-megaphone/0.1/amp-megaphone.js
@@ -34,6 +34,7 @@ import {getData, listen} from '../../../src/event-helper';
 import {isLayoutSizeFixed} from '../../../src/layout';
 import {isObject} from '../../../src/types';
 import {removeElement} from '../../../src/dom';
+import {setIsMediaComponent} from '../../../src/video-interface';
 import {startsWith} from '../../../src/string';
 import {tryParseJson} from '../../../src/json';
 import {userAssert} from '../../../src/log';
@@ -78,6 +79,7 @@ class AmpMegaphone extends AMP.BaseElement {
 
   /** @override */
   buildCallback() {
+    setIsMediaComponent(this.element);
     this.updateBaseUrl_();
   }
 

diff --git a/extensions/amp-o2-player/0.1/amp-o2-player.js b/extensions/amp-o2-player/0.1/amp-o2-player.js
@@ -17,6 +17,7 @@
 import {Services} from '../../../src/services';
 import {dict} from '../../../src/utils/object';
 import {isLayoutSizeDefined} from '../../../src/layout';
+import {setIsMediaComponent} from '../../../src/video-interface';
 import {userAssert} from '../../../src/log';
 
 class AmpO2Player extends AMP.BaseElement {
@@ -59,6 +60,8 @@ class AmpO2Player extends AMP.BaseElement {
 
   /** @override */
   buildCallback() {
+    setIsMediaComponent(this.element);
+
     this.pid_ = userAssert(
       this.element.getAttribute('data-pid'),
       'data-pid attribute is required for <amp-o2-player> %s',

diff --git a/extensions/amp-reach-player/0.1/amp-reach-player.js b/extensions/amp-reach-player/0.1/amp-reach-player.js
@@ -16,6 +16,7 @@
 
 import {Services} from '../../../src/services';
 import {isLayoutSizeDefined} from '../../../src/layout';
+import {setIsMediaComponent} from '../../../src/video-interface';
 
 class AmpReachPlayer extends AMP.BaseElement {
   /** @param {!AmpElement} element */
@@ -43,6 +44,11 @@ class AmpReachPlayer extends AMP.BaseElement {
     return isLayoutSizeDefined(layout);
   }
 
+  /** @override */
+  buildCallback() {
+    setIsMediaComponent(this.element);
+  }
+
   /** @override */
   layoutCallback() {
     const embedId = this.element.getAttribute('data-embed-id') || 'default';

diff --git a/extensions/amp-soundcloud/0.1/amp-soundcloud.js b/extensions/amp-soundcloud/0.1/amp-soundcloud.js
@@ -30,6 +30,7 @@
 import {Services} from '../../../src/services';
 import {dict} from '../../../src/utils/object';
 import {isLayoutSizeDefined} from '../../../src/layout';
+import {setIsMediaComponent} from '../../../src/video-interface';
 import {userAssert} from '../../../src/log';
 
 class AmpSoundcloud extends AMP.BaseElement {
@@ -58,6 +59,11 @@ class AmpSoundcloud extends AMP.BaseElement {
     return isLayoutSizeDefined(layout);
   }
 
+  /** @override */
+  buildCallback() {
+    setIsMediaComponent(this.element);
+  }
+
   /**@override*/
   layoutCallback() {
     const height = this.element.getAttribute('height');

diff --git a/extensions/amp-springboard-player/0.1/amp-springboard-player.js b/extensions/amp-springboard-player/0.1/amp-springboard-player.js
@@ -16,6 +16,7 @@
 
 import {Services} from '../../../src/services';
 import {isLayoutSizeDefined} from '../../../src/layout';
+import {setIsMediaComponent} from '../../../src/video-interface';
 import {userAssert} from '../../../src/log';
 
 class AmpSpringboardPlayer extends AMP.BaseElement {
@@ -66,6 +67,8 @@ class AmpSpringboardPlayer extends AMP.BaseElement {
 
   /** @override */
   buildCallback() {
+    setIsMediaComponent(this.element);
+
     this.mode_ = userAssert(
       this.element.getAttribute('data-mode'),
       'The data-mode attribute is required for <amp-springboard-player> %s',

diff --git a/src/service/video-manager-impl.js b/src/service/video-manager-impl.js
@@ -29,6 +29,7 @@ import {
   VideoAttributes,
   VideoEvents,
   VideoServiceSignals,
+  setIsMediaComponent,
   userInteractedWith,
   videoAnalyticsCustomEventTypeKey,
 } from '../video-interface';
@@ -192,7 +193,7 @@ export class VideoManager {
     const {element} = entry.video;
     element.dispatchCustomEvent(VideoEvents.REGISTERED);
 
-    element.classList.add('i-amphtml-video-component');
+    setIsMediaComponent(element);
 
     // Unlike events, signals are permanent. We can wait for `REGISTERED` at any
     // moment in the element's lifecycle and the promise will resolve

diff --git a/src/video-interface.js b/src/video-interface.js
@@ -560,3 +560,26 @@ export function delegateAutoplay(video) {
 export function userInteractedWith(video) {
   video.signals().signal(VideoServiceSignals.USER_INTERACTED);
 }
+
+/**
+ * Classname that media components should annotate themselves with.
+ * This applies to all video and audio playback components, regardless of
+ * whether they implement a common interface or not.
+ *
+ * TODO(go.amp.dev/issue/26984): This isn't exclusive to video, but there's no
+ * better place to put this now due to OWNERShip. Move.
+ */
+export const MEDIA_COMPONENT_CLASSNAME = 'i-amphtml-media-component';
+
+/**
+ * Annotates media component element with a common classname.
+ * This applies to all video and audio playback components, regardless of
+ * whether they implement a common interface or not.
+ * @param {!Element} element
+ *
+ * TODO(go.amp.dev/issue/26984): This isn't exclusive to video, but there's no
+ * better place to put this now due to OWNERShip. Move.
+ */
+export function setIsMediaComponent(element) {
+  element.classList.add(MEDIA_COMPONENT_CLASSNAME);
+}
diff --git a/test/visual-diff/visual-tests b/test/visual-diff/visual-tests
@@ -772,21 +772,21 @@
       "name": "amp-video-docking (left-to-right)",
       "viewport": {"width": 800, "height": 600},
       "interactive_tests": "examples/visual-tests/amp-video-docking/amp-video-docking.js",
-      "loading_complete_selectors": [".i-amphtml-video-component"],
+      "loading_complete_selectors": [".i-amphtml-video-interface"],
     },
     {
       "url": "examples/visual-tests/amp-video-docking/rtl.html",
       "name": "amp-video-docking (right-to-left)",
       "viewport": {"width": 800, "height": 600},
       "interactive_tests": "examples/visual-tests/amp-video-docking/amp-video-docking.js",
-      "loading_complete_selectors": [".i-amphtml-video-component"],
+      "loading_complete_selectors": [".i-amphtml-video-interface"],
     },
     {
       "url": "examples/visual-tests/amp-video-docking/slot.html",
       "name": "amp-video-docking (slot)",
       "viewport": {"width": 800, "height": 600},
       "interactive_tests": "examples/visual-tests/amp-video-docking/amp-video-docking.js",
-      "loading_complete_selectors": [".i-amphtml-video-component"],
+      "loading_complete_selectors": [".i-amphtml-video-interface"],
     },
   ]
 }