amp-script: improve illegal mutation messaging (#27834)

* amp-script: improve error messages for illegal mutations * small messaging cleanups after testing * remove accidentally included file * toggleLoading: fix race * add a test * fix e2e tests * restore old behavior * use mutating instead of hydrating
ampproject · Apr 21, 2020 · c00919b · c00919b
1 parent 1098976
commit c00919b
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 28 deletions.
diff --git a/extensions/amp-script/0.1/amp-script.js b/extensions/amp-script/0.1/amp-script.js
@@ -419,49 +419,57 @@ export class AmpScript extends AMP.BaseElement {
       // Emit an error message for each mutation type, including count.
       Object.keys(errors).forEach((type) => {
         const count = errors[type];
-        user().error(
-          TAG,
-          'Dropped %sx "%s" mutation(s); user gesture is required with [layout=container].',
-          count,
-          this.mutationTypeToString_(type)
-        );
+        user().error(TAG, this.mutationTypeToErrorMessage_(type, count));
       });
-    });
 
-    // TODO(amphtml): flush(false) already filters all user-visible mutations,
-    // so we could just remove this code block for gentler failure mode.
-    if (!allowMutation && phase == Phase.MUTATING) {
-      this.workerDom_.terminate();
+      if (disallowedTypes.length > 0 && phase === Phase.MUTATING) {
+        this.workerDom_.terminate();
 
-      this.element.classList.remove('i-amphtml-hydrated');
-      this.element.classList.add('i-amphtml-broken');
+        this.element.classList.remove('i-amphtml-hydrated');
+        this.element.classList.add('i-amphtml-broken');
 
-      user().error(
-        TAG,
-        '%s was terminated due to illegal mutation.',
-        this.debugId_
-      );
-    }
+        user().error(
+          TAG,
+          '%s was terminated due to illegal mutation.',
+          this.debugId_
+        );
+      }
+    });
   }
 
   /**
    * @param {string} type
+   * @param {number} count
    * @return {string}
    */
-  mutationTypeToString_(type) {
+  mutationTypeToErrorMessage_(type, count) {
+    let target;
+
     // Matches TransferrableMutationType in worker-dom#src/transfer/TransferrableMutation.ts.
     switch (type) {
+      // Attributes and Properties
       case '0':
-        return 'ATTRIBUTES';
+      case '3':
+        target = 'DOM element attributes or styles';
+        break;
+      // Character data
       case '1':
-        return 'CHARACTER_DATA';
+        target = 'textContent or the like';
+        break;
+      // Child list
       case '2':
-        return 'CHILD_LIST';
-      case '3':
-        return 'PROPERTIES';
+        target = 'DOM element children, innerHTML, or the like';
+        break;
+      // Other
       default:
-        return 'OTHER';
+        target = 'the DOM';
+        break;
     }
+
+    return (
+      `Blocked ${count} attempts to modify ${target}.` +
+      ' For variable-sized <amp-script> containers, a user action has to happen first.'
+    );
   }
 }
 

diff --git a/src/custom-element.js b/src/custom-element.js
@@ -1226,7 +1226,8 @@ function createBaseCustomElementClass(win) {
             if (
               this.isInViewport_ &&
               this.ownerDocument &&
-              this.ownerDocument.defaultView
+              this.ownerDocument.defaultView &&
+              this.layoutCount_ === 0 // Ensures that layoutCallback hasn't completed in this 100ms window.
             ) {
               this.toggleLoading(true, {startTime: loadingStartTime});
             }

diff --git a/test/unit/test-custom-element.js b/test/unit/test-custom-element.js
@@ -1995,8 +1995,8 @@ describes.realWin('CustomElement', {amp: true}, (env) => {
       it('should NOT turn on when enters viewport but already laid out', () => {
         stubInA4A(false);
         const toggle = env.sandbox.spy(element, 'toggleLoading');
-        element.layoutCount_ = 1;
         element.viewportCallback(true);
+        element.layoutCount_ = 1;
         clock.tick(1000);
         expect(toggle).to.have.not.been.called;
       });