🐛 Report zero visibility for zero size element (ampproject#30105)

* check belement size before setting visible percentage * fix test * nit
ed-bird · Dec 10, 2020 · ffb0bca · ffb0bca
1 parent c86f892
commit ffb0bca
Show file tree

Hide file tree

Showing 3 changed files with 76 additions and 4 deletions.
diff --git a/extensions/amp-analytics/0.1/test/test-amp-analytics.js b/extensions/amp-analytics/0.1/test/test-amp-analytics.js
@@ -1468,8 +1468,11 @@ describes.realWin(
     describe('Sandbox AMP Analytics Element', () => {
       beforeEach(() => {
         // Unfortunately need to fake sandbox analytics element's parent
-        // to an AMP element
+        // to an AMP element.
+        // Set the doc width/height to 1 to trigger visible event.
         doc.body.classList.add('i-amphtml-element');
+        doc.body.style.minWidth = '1px';
+        doc.body.style.minHeight = '1px';
       });
 
       afterEach(() => {
@@ -1751,6 +1754,10 @@ describes.realWin(
     describe('parentPostMessage', () => {
       let postMessageSpy;
 
+      beforeEach(() => {
+        doc.body.style.minWidth = '1px';
+        doc.body.style.minHeight = '1px';
+      });
       function waitForParentPostMessage(opt_max) {
         if (postMessageSpy.callCount) {
           return Promise.resolve();

diff --git a/extensions/amp-analytics/0.1/test/test-visibility-manager.js b/extensions/amp-analytics/0.1/test/test-visibility-manager.js
@@ -202,6 +202,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, (env) => {
         target: otherTarget,
         intersectionRatio: 0.3,
         intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+        boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
       },
     ]);
     expect(root.getRootVisibility()).to.equal(0);
@@ -212,11 +213,13 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, (env) => {
         target: otherTarget,
         intersectionRatio: 0.5,
         intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+        boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
       },
       {
         target: win.document.documentElement,
         intersectionRatio: 0.3,
         intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+        boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
       },
     ]);
     expect(root.getRootVisibility()).to.equal(0.3);
@@ -227,6 +230,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, (env) => {
         target: win.document.documentElement,
         intersectionRatio: 0,
         intersectionRect: layoutRectLtwh(0, 0, 0, 0),
+        boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
       },
     ]);
     expect(root.getRootVisibility()).to.equal(0);
@@ -525,6 +529,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, (env) => {
         target,
         intersectionRatio: 0.3,
         intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+        boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
       },
     ]);
     expect(model.getVisibility_()).to.equal(0.3);
@@ -569,6 +574,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, (env) => {
         target,
         intersectionRatio: 0.3,
         intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+        boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
       },
     ]);
     expect(model.getVisibility_()).to.equal(0.3);
@@ -579,6 +585,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, (env) => {
         target,
         intersectionRatio: -0.01,
         intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+        boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
       },
     ]);
     expect(model.getVisibility_()).to.equal(0);
@@ -588,6 +595,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, (env) => {
         target,
         intersectionRatio: -1000,
         intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+        boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
       },
     ]);
     expect(model.getVisibility_()).to.equal(0);
@@ -598,6 +606,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, (env) => {
         target,
         intersectionRatio: 1.01,
         intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+        boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
       },
     ]);
     expect(model.getVisibility_()).to.equal(1);
@@ -607,11 +616,55 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, (env) => {
         target,
         intersectionRatio: 1000,
         intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+        boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
       },
     ]);
     expect(model.getVisibility_()).to.equal(1);
   });
 
+  it('should protect from zero size element', () => {
+    const target = win.document.createElement('div');
+    root.listenElement(target, {}, null, null, eventResolver);
+    expect(root.models_).to.have.length(1);
+    const model = root.models_[0];
+
+    const inOb = root.getIntersectionObserver_();
+    expect(model.getVisibility_()).to.equal(0);
+
+    // Valid element size
+    inOb.callback([
+      {
+        target,
+        intersectionRatio: 1,
+        intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+        boundingClientRect: layoutRectLtwh(1, 1, 10, 10),
+      },
+    ]);
+    expect(model.getVisibility_()).to.equal(1);
+
+    // zero element size.
+    inOb.callback([
+      {
+        target,
+        intersectionRatio: 1,
+        intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+        boundingClientRect: layoutRectLtwh(1, 1, 0, 10),
+      },
+    ]);
+    expect(model.getVisibility_()).to.equal(0);
+
+    // zero element size, high resolution screen.
+    inOb.callback([
+      {
+        target,
+        intersectionRatio: 0.8,
+        intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+        boundingClientRect: layoutRectLtwh(1, 1, 10, 0.452),
+      },
+    ]);
+    expect(model.getVisibility_()).to.equal(0);
+  });
+
   it('should listen on a element with different specs', () => {
     clock.tick(1);
     const inOb = root.getIntersectionObserver_();
@@ -637,6 +690,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, (env) => {
         target,
         intersectionRatio: 0.3,
         intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+        boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
       },
     ]);
     expect(model1.getVisibility_()).to.equal(0.3);
@@ -712,6 +766,7 @@ describes.fakeWin('VisibilityManagerForDoc', {amp: true}, (env) => {
         target,
         intersectionRatio: 0.3,
         intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+        boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
       },
     ]);
 
@@ -895,6 +950,7 @@ describes.realWin(
           target: embed.host,
           intersectionRatio: 0.5,
           intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+          boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
         },
       ]);
       expect(root.getRootVisibility()).to.equal(0.5);
@@ -907,6 +963,7 @@ describes.realWin(
           target: otherTarget,
           intersectionRatio: 0.45,
           intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+          boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
         },
       ]);
       expect(root.getRootVisibility()).to.equal(0.5);
@@ -933,6 +990,7 @@ describes.realWin(
           target: embed.host,
           intersectionRatio: 0,
           intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+          boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
         },
       ]);
       expect(root.getRootVisibility()).to.equal(0);
@@ -945,6 +1003,7 @@ describes.realWin(
           target: otherTarget,
           intersectionRatio: 0.55,
           intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+          boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
         },
       ]);
       expect(root.getRootVisibility()).to.equal(0);
@@ -957,6 +1016,7 @@ describes.realWin(
           target: embed.host,
           intersectionRatio: 0.7,
           intersectionRect: layoutRectLtwh(0, 0, 1, 1),
+          boundingClientRect: layoutRectLtwh(1, 1, 1, 1),
         },
       ]);
       expect(root.getRootVisibility()).to.equal(0.7);

diff --git a/extensions/amp-analytics/0.1/visibility-manager.js b/extensions/amp-analytics/0.1/visibility-manager.js
@@ -872,7 +872,6 @@ export class VisibilityManagerForDoc extends VisibilityManager {
         Number(intersection.width),
         Number(intersection.height)
       );
-      // TODO(#29618): Remove after ampim investigation
       let {boundingClientRect} = change;
       boundingClientRect =
         boundingClientRect &&
@@ -892,7 +891,6 @@ export class VisibilityManagerForDoc extends VisibilityManager {
   }
 
   /**
-   * TODO(#29618): Clean up boundingClientRect after ampim investigation
    * @param {!Element} target
    * @param {number} intersectionRatio
    * @param {!../../../src/layout-rect.LayoutRectDef} intersectionRect
@@ -905,9 +903,16 @@ export class VisibilityManagerForDoc extends VisibilityManager {
     intersectionRect,
     boundingClientRect
   ) {
-    intersectionRatio = Math.min(Math.max(intersectionRatio, 0), 1);
     const id = getElementId(target);
     const trackedElement = this.trackedElements_[id];
+    if (boundingClientRect.width < 1 || boundingClientRect.height < 1) {
+      // Set intersectionRatio to 0 when the element is not visible.
+      // Use < 1 because the width/height can
+      // be a double value on high resolution screen
+      intersectionRatio = 0;
+    } else {
+      intersectionRatio = Math.min(Math.max(intersectionRatio, 0), 1);
+    }
     if (trackedElement) {
       trackedElement.intersectionRatio = intersectionRatio;
       trackedElement.intersectionRect = intersectionRect;