Register default actions programmatically. (#19032)

* address choumx comments * fix * add extern, fix check types * address choumx comments * fix test * add another test * address choumx comments * address choumx comments * fix * more fixes * remove activate and activationTrust from base-element * lint * check types fix * fix test
ampproject · Dec 20, 2018 · 5aa553e · 5aa553e
1 parent e61bcd1
commit 5aa553e
Show file tree

Hide file tree

Showing 18 changed files with 213 additions and 138 deletions.
diff --git a/build-system/amp.extern.js b/build-system/amp.extern.js
@@ -561,6 +561,8 @@ SomeBaseElementLikeClass.prototype.inViewport_;
 
 SomeBaseElementLikeClass.prototype.actionMap_;
 
+SomeBaseElementLikeClass.prototype.defaultActionAlias_;
+
 AMP.BaseTemplate;
 
 AMP.RealTimeConfigManager;

diff --git a/extensions/amp-animation/0.1/amp-animation.js b/extensions/amp-animation/0.1/amp-animation.js
@@ -154,8 +154,10 @@ export class AmpAnimation extends AMP.BaseElement {
     }
 
     // Actions.
-    this.registerAction('start',
-        this.startAction_.bind(this), ActionTrust.LOW);
+    this.registerDefaultAction(
+        this.startAction_.bind(this),
+        'start',
+        ActionTrust.LOW);
     this.registerAction('restart',
         this.restartAction_.bind(this), ActionTrust.LOW);
     this.registerAction('pause',
@@ -195,15 +197,11 @@ export class AmpAnimation extends AMP.BaseElement {
     this.setVisible_(false);
   }
 
-  /** @override */
-  activate(invocation) {
-    return this.startAction_(invocation);
-  }
-
   /**
    * @param {?../../../src/service/action-impl.ActionInvocation=} opt_invocation
    * @return {?Promise}
    * @private
+   * @visibleForTesting
    */
   startAction_(opt_invocation) {
     // The animation has been triggered, but there's no guarantee that it

diff --git a/extensions/amp-animation/0.1/test/test-amp-animation.js b/extensions/amp-animation/0.1/test/test-amp-animation.js
@@ -15,10 +15,10 @@
  */
 
 import {AmpAnimation} from '../amp-animation';
+import {DEFAULT_ACTION} from '../../../../src/action-constants';
 import {WebAnimationPlayState} from '../web-animation-types';
 import {WebAnimationRunner} from '../web-animations';
 
-
 describes.sandboxed('AmpAnimation', {}, () => {
 
   function createAnimInWindow(win, attrs, config) {
@@ -166,7 +166,7 @@ describes.sandboxed('AmpAnimation', {}, () => {
     it('should trigger animation, but not start when invisible', function* () {
       const anim = yield createAnim({trigger: 'visibility'}, {duration: 1001});
       const startStub = sandbox.stub(anim, 'startOrResume_');
-      anim.activate();
+      anim.startAction_();
       expect(anim.triggered_).to.be.true;
       expect(startStub).to.not.be.called;
     });
@@ -175,7 +175,7 @@ describes.sandboxed('AmpAnimation', {}, () => {
       const anim = yield createAnim({trigger: 'visibility'}, {duration: 1001});
       const startStub = sandbox.stub(anim, 'startOrResume_');
       viewer.setVisibilityState_('visible');
-      anim.activate();
+      anim.startAction_();
       expect(anim.triggered_).to.be.true;
       expect(startStub).to.be.calledOnce;
     });
@@ -184,7 +184,7 @@ describes.sandboxed('AmpAnimation', {}, () => {
       const anim = yield createAnim({trigger: 'visibility'}, {duration: 1001});
       const startStub = sandbox.stub(anim, 'startOrResume_');
       const pauseStub = sandbox.stub(anim, 'pause_');
-      anim.activate();
+      anim.startAction_();
       expect(anim.triggered_).to.be.true;
 
       // Go to visible state.
@@ -219,7 +219,7 @@ describes.sandboxed('AmpAnimation', {}, () => {
       const anim = yield createAnim({trigger: 'visibility'}, {duration: 1001});
       const startStub = sandbox.stub(anim, 'startOrResume_');
       const pauseStub = sandbox.stub(anim, 'pause_');
-      anim.activate();
+      anim.startAction_();
       anim.pausedByAction_ = true;
       expect(anim.triggered_).to.be.true;
 
@@ -232,7 +232,7 @@ describes.sandboxed('AmpAnimation', {}, () => {
     it('should create runner', function* () {
       const anim = yield createAnim({trigger: 'visibility'},
           {duration: 1001, animations: []});
-      anim.activate();
+      anim.startAction_();
       anim.visible_ = true;
       runnerMock.expects('start').once();
       runnerMock.expects('finish').never();
@@ -244,7 +244,7 @@ describes.sandboxed('AmpAnimation', {}, () => {
     it('should finish animation and runner', function* () {
       const anim = yield createAnim({trigger: 'visibility'},
           {duration: 1001, animations: []});
-      anim.activate();
+      anim.startAction_();
       anim.visible_ = true;
       runnerMock.expects('start').once();
       runnerMock.expects('finish').once();
@@ -257,7 +257,7 @@ describes.sandboxed('AmpAnimation', {}, () => {
     it('should pause/resume animation and runner', function* () {
       const anim = yield createAnim({trigger: 'visibility'},
           {duration: 1001, animations: []});
-      anim.activate();
+      anim.startAction_();
       anim.visible_ = true;
       runnerMock.expects('start').once();
       runnerMock.expects('pause').once();
@@ -273,7 +273,7 @@ describes.sandboxed('AmpAnimation', {}, () => {
     it('should finish when animation is complete', function* () {
       const anim = yield createAnim({trigger: 'visibility'},
           {duration: 1001, animations: []});
-      anim.activate();
+      anim.startAction_();
       anim.visible_ = true;
       yield anim.startOrResume_();
       expect(anim.triggered_).to.be.true;
@@ -302,7 +302,7 @@ describes.sandboxed('AmpAnimation', {}, () => {
         function* () {
           const anim = yield createAnim({trigger: 'visibility'},
               {duration: 1001, animations: []});
-          anim.activate();
+          anim.startAction_();
           anim.visible_ = true;
           yield anim.startOrResume_();
           expect(anim.runner_).to.exist;
@@ -331,7 +331,7 @@ describes.sandboxed('AmpAnimation', {}, () => {
         function* () {
           const anim = yield createAnim({trigger: 'visibility'},
               {duration: 1001, animations: []});
-          anim.activate();
+          anim.startAction_();
           anim.visible_ = true;
           yield anim.startOrResume_();
           expect(anim.runner_).to.exist;
@@ -354,7 +354,7 @@ describes.sandboxed('AmpAnimation', {}, () => {
     it('should ignore start when not triggered', function* () {
       const anim = yield createAnim({trigger: 'visibility'},
           {duration: 1001, animations: []});
-      anim.activate();
+      anim.startAction_();
       anim.visible_ = false;
       expect(anim.startOrResume_()).to.be.null;
     });
@@ -372,7 +372,7 @@ describes.sandboxed('AmpAnimation', {}, () => {
       it('should trigger activate', () => {
         const args = {};
         const invocation = {
-          method: 'activate',
+          method: DEFAULT_ACTION,
           args,
           satisfiesTrust: () => true,
         };

diff --git a/extensions/amp-bind/0.1/amp-state.js b/extensions/amp-bind/0.1/amp-state.js
@@ -45,15 +45,6 @@ export class AmpState extends AMP.BaseElement {
     return true;
   }
 
-  /** @override */
-  activate(unusedInvocation) {
-    // TODO(choumx): Remove this after a few weeks in production.
-    const TAG = this.getName_();
-    this.user().error(TAG,
-        'Please use AMP.setState() action explicitly, e.g. ' +
-        'on="submit-success:AMP.setState({myAmpState: event.response})"');
-  }
-
   /** @override */
   buildCallback() {
     toggle(this.element, /* opt_display */ false);

diff --git a/extensions/amp-image-lightbox/0.1/amp-image-lightbox.js b/extensions/amp-image-lightbox/0.1/amp-image-lightbox.js
@@ -736,12 +736,15 @@ class AmpImageLightbox extends AMP.BaseElement {
 
     /** @private {!Function} */
     this.boundCloseOnEscape_ = this.closeOnEscape_.bind(this);
+
+    this.registerDefaultAction(
+        invocation => this.open_(invocation), 'open');
   }
 
   /**
-  * Lazily builds the image-lightbox DOM on the first open.
-  * @private
-  * */
+   * Lazily builds the image-lightbox DOM on the first open.
+   * @private
+   */
   buildLightbox_() {
     if (this.container_) {
       return;
@@ -798,8 +801,11 @@ class AmpImageLightbox extends AMP.BaseElement {
     });
   }
 
-  /** @override */
-  activate(invocation) {
+  /**
+   * @param {?../../../src/service/action-impl.ActionInvocation=} invocation
+   * @private
+   */
+  open_(invocation) {
     if (this.active_) {
       return;
     }

diff --git a/extensions/amp-image-lightbox/0.1/test/test-amp-image-lightbox.js b/extensions/amp-image-lightbox/0.1/test/test-amp-image-lightbox.js
@@ -68,7 +68,7 @@ describes.realWin('amp-image-lightbox component', {
 
       const ampImage = doc.createElement('amp-img');
       ampImage.setAttribute('src', 'data:');
-      impl.activate({caller: ampImage});
+      impl.open_({caller: ampImage});
 
       const container = lightbox
           .querySelector('.i-amphtml-image-lightbox-container');
@@ -117,7 +117,7 @@ describes.realWin('amp-image-lightbox component', {
 
       const ampImage = doc.createElement('amp-img');
       ampImage.setAttribute('src', 'data:');
-      impl.activate({caller: ampImage});
+      impl.open_({caller: ampImage});
 
       expect(viewportOnChanged).to.be.calledOnce;
       expect(impl.unlistenViewport_).to.not.equal(null);
@@ -189,13 +189,13 @@ describes.realWin('amp-image-lightbox component', {
       ampImage.setAttribute('src', 'data:');
       ampImage.setAttribute('width', '100');
       ampImage.setAttribute('height', '100');
-      impl.activate({caller: ampImage});
+      impl.open_({caller: ampImage});
       impl.closeOnEscape_(new KeyboardEvent('keydown', {key: Keys.ESCAPE}));
       expect(setupCloseSpy).to.be.calledOnce;
 
       // Regression test: ensure escape event listener is bound properly
       expect(nullAddEventListenerSpy).to.have.not.been.called;
-      impl.activate({caller: ampImage});
+      impl.open_({caller: ampImage});
       expect(nullAddEventListenerSpy).to.have.not.been.called;
     });
   });
@@ -217,7 +217,7 @@ describes.realWin('amp-image-lightbox component', {
       const sourceElement = doc.createElement('amp-img');
       sourceElement.setAttribute('src', 'data:');
 
-      impl.activate({caller: sourceElement});
+      impl.open_({caller: sourceElement});
       impl.close();
 
       expect(tryFocus).to.be.calledOnce;

diff --git a/extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js b/extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js
@@ -14,7 +14,6 @@
  * limitations under the License.
  */
 
-
 import {Animation} from '../../../src/animation';
 import {CSS} from '../../../build/amp-lightbox-gallery-0.1.css';
 import {CommonSignals} from '../../../src/common-signals';
@@ -199,7 +198,9 @@ export class AmpLightboxGallery extends AMP.BaseElement {
         </div>`;
       this.element.appendChild(this.container_);
       this.manager_.maybeInit();
-      this.registerAction('open', invocation => this.activate(invocation));
+      this.registerDefaultAction(
+          invocation => this.open_(invocation),
+          'open');
     });
   }
 
@@ -733,18 +734,18 @@ export class AmpLightboxGallery extends AMP.BaseElement {
    *
    *  // Opens the element referenced by elementId
    *  on="tap:myLightboxGallery.open(id='<elementId>')
-   * @override
    * @param {!../../../src/service/action-impl.ActionInvocation} invocation
+   * @private
    */
-  activate(invocation) {
+  open_(invocation) {
     let target = invocation.caller;
     if (invocation.args && invocation.args['id']) {
       const targetId = invocation.args['id'];
       target = this.getAmpDoc().getElementById(targetId);
       user().assert(target,
           'amp-lightbox-gallery.open: element with id: %s not found', targetId);
     }
-    this.open_(dev().assertElement(target)).then(() => {
+    this.openLightboxGallery_(dev().assertElement(target)).then(() => {
       return this.history_.push(this.close_.bind(this));
     }).then(historyId => {
       this.historyId_ = historyId;
@@ -757,7 +758,7 @@ export class AmpLightboxGallery extends AMP.BaseElement {
    * @return {!Promise}
    * @private
    */
-  open_(element) {
+  openLightboxGallery_(element) {
     this.sourceElement_ = element;
     const lightboxGroupId = element.getAttribute('lightbox')
       || 'default';

diff --git a/extensions/amp-lightbox/0.1/amp-lightbox.js b/extensions/amp-lightbox/0.1/amp-lightbox.js
@@ -170,7 +170,9 @@ class AmpLightbox extends AMP.BaseElement {
     this.action_ = Services.actionServiceForDoc(this.element);
     this.maybeSetTransparentBody_();
 
-    this.registerAction('open', this.activate.bind(this));
+    this.registerDefaultAction(
+        unused => this.open_(),
+        'open');
     this.registerAction('close', this.close.bind(this));
   }
 
@@ -252,8 +254,10 @@ class AmpLightbox extends AMP.BaseElement {
     return Promise.resolve();
   }
 
-  /** @override */
-  activate() {
+  /**
+   * @private
+   */
+  open_() {
     if (this.active_) {
       return;
     }
@@ -271,7 +275,7 @@ class AmpLightbox extends AMP.BaseElement {
     const open = mutations['open'];
     if (open !== undefined) {
       if (open) {
-        this.activate();
+        this.open_();
       } else {
         this.close();
       }

diff --git a/extensions/amp-live-list/0.1/amp-live-list.js b/extensions/amp-live-list/0.1/amp-live-list.js
@@ -14,7 +14,6 @@
  * limitations under the License.
  */
 
-import {ActionTrust} from '../../../src/action-constants';
 import {AmpEvents} from '../../../src/amp-events';
 import {CSS} from '../../../build/amp-live-list-0.1.css';
 import {Layout} from '../../../src/layout';
@@ -233,8 +232,7 @@ export class AmpLiveList extends AMP.BaseElement {
     this.curNumOfLiveItems_ = this.validateLiveListItems_(
         this.itemsSlot_, true);
 
-    this.registerAction(
-        'update', this.updateAction_.bind(this), ActionTrust.HIGH);
+    this.registerDefaultAction(this.updateAction_.bind(this), 'update');
 
     if (!this.element.hasAttribute('aria-live')) {
       this.element.setAttribute('aria-live', 'polite');
@@ -255,11 +253,6 @@ export class AmpLiveList extends AMP.BaseElement {
     }
   }
 
-  /** @override */
-  activate() {
-    this.updateAction_();
-  }
-
   /** @override */
   update(updatedElement) {
     const container = this.getItemsSlot_(updatedElement);

diff --git a/extensions/amp-sidebar/0.1/amp-sidebar.js b/extensions/amp-sidebar/0.1/amp-sidebar.js
@@ -169,9 +169,8 @@ export class AmpSidebar extends AMP.BaseElement {
       this.close_();
     });
     element.appendChild(screenReaderCloseButton);
-
+    this.registerDefaultAction(invocation => this.open_(invocation), 'open');
     this.registerAction('toggle', this.toggle_.bind(this));
-    this.registerAction('open', this.open_.bind(this));
     this.registerAction('close', this.close_.bind(this));
 
     element.addEventListener('click', e => {
@@ -196,11 +195,6 @@ export class AmpSidebar extends AMP.BaseElement {
 
   }
 
-  /** @override */
-  activate(invocation) {
-    this.open_(invocation);
-  }
-
   /** @override */
   onLayoutMeasure() {
     this.getAmpDoc().whenReady().then(() => {

diff --git a/extensions/amp-sidebar/0.1/test/test-amp-sidebar.js b/extensions/amp-sidebar/0.1/test/test-amp-sidebar.js
@@ -558,7 +558,6 @@ describes.realWin('amp-sidebar 0.1 version', {
         });
         impl.scheduleLayout = sandbox.stub();
 
-        impl.buildCallback();
         impl.open_();
         expect(triggerSpy).to.be.calledOnce;
         expect(triggerSpy).to.be.calledWith(impl.element, 'sidebarOpen');