Merge pull request #1025 from dvoytenko/resize3-scrolladj

Automatically adjust scrolling position when resizing elements above the viewport

src/resources.js

@@ -507,13 +507,23 @@ export class Resources {
    */
   mutateWork_() {
     // Read all necessary data before mutates.
+    // The height changing depends largely on the target element's position
+    // in the active viewport. We consider the active viewport the part of the
+    // visible viewport below 10% from the top and above 25% from the bottom.
+    // This is basically the portion of the viewport where the reader is most
+    // likely focused right now. The main goal is to avoid drastic UI changes
+    // in that part of the content. The elements below the active viewport are
+    // freely resized. The elements above the viewport are resized and request
+    // scroll adjustment to avoid active viewport changing without user's
+    // action. The elements in the active viewport are not resized and instead
+    // the overflow callbacks are called.
     const now = timer.now();
     const viewportRect = this.viewport_.getRect();
+    const topOffset = viewportRect.height / 10;
+    const bottomOffset = viewportRect.height / 4;
     const isScrollingStopped = (Math.abs(this.lastVelocity_) < 1e-2 &&
         now - this.lastScrollTime_ > MUTATE_DEFER_DELAY_ ||
         now - this.lastScrollTime_ > MUTATE_DEFER_DELAY_ * 2);
-    const offset = 10;
-    const bottomOffset = viewportRect.height / 4;
 
     if (this.deferredMutates_.length > 0) {
       log.fine(TAG_, 'deferred mutates:', this.deferredMutates_.length);
@@ -532,6 +542,7 @@ export class Resources {
 
       // Find minimum top position and run all mutates.
       let minTop = -1;
+      const scrollAdjSet = [];
       for (let i = 0; i < requestsChangeHeight.length; i++) {
         const request = requestsChangeHeight[i];
         const resource = request.resource;
@@ -554,11 +565,14 @@ export class Resources {
         } else if (box.bottom >= viewportRect.bottom - bottomOffset) {
           // 3. Elements under viewport are resized immediately.
           resize = true;
-        } else if (box.bottom <= viewportRect.top + offset) {
+        } else if (box.bottom <= viewportRect.top + topOffset) {
           // 4. Elements above the viewport can only be resized when scrolling
           // has stopped, otherwise defer util next cycle.
           if (isScrollingStopped) {
-            resize = true;
+            // These requests will be executed in the next animation cycle and
+            // adjust the scroll position.
+            resize = false;
+            scrollAdjSet.push(request);
           } else {
             // Defer till next cycle.
             this.requestsChangeHeight_.push(request);
@@ -587,8 +601,32 @@ export class Resources {
         this.relayoutTop_ = minTop;
       }
 
-      // TODO(dvoytenko): consider scroll adjustments when resizing is done
-      // above the current scrolling position.
+      // Execute scroll-adjusting resize requests, if any.
+      if (scrollAdjSet.length > 0) {
+        this.vsync_.run({
+          measure: state => {
+            state./*OK*/scrollHeight = this.viewport_./*OK*/getScrollHeight();
+            state./*OK*/scrollTop = this.viewport_./*OK*/getScrollTop();
+          },
+          mutate: state => {
+            let minTop = -1;
+            scrollAdjSet.forEach(request => {
+              const box = request.resource.getLayoutBox();
+              minTop = minTop == -1 ? box.top : Math.min(minTop, box.top);
+              request.resource./*OK*/changeHeight(request.newHeight);
+            });
+            if (minTop != -1) {
+              this.relayoutTop_ = minTop;
+            }
+            // Sync is necessary here to avoid UI jump in the next frame.
+            const newScrollHeight = this.viewport_./*OK*/getScrollHeight();
+            if (newScrollHeight > state./*OK*/scrollHeight) {
+              this.viewport_.setScrollTop(state./*OK*/scrollTop +
+                  (newScrollHeight - state./*OK*/scrollHeight));
+            }
+          }
+        });
+      }
     }
   }
 

src/viewport.js

@@ -190,13 +190,23 @@ export class Viewport {
   }
 
   /**
-   * Returns the scroll width of the content within the viewport.
+   * Returns the scroll width of the content of the document. Note that this
+   * method is not cached since we there's no indication when it might change.
    * @return {number}
    */
   getScrollWidth() {
     return this.binding_.getScrollWidth();
   }
 
+  /**
+   * Returns the scroll height of the content of the document. Note that this
+   * method is not cached since we there's no indication when it might change.
+   * @return {number}
+   */
+  getScrollHeight() {
+    return this.binding_.getScrollHeight();
+  }
+
   /**
    * Returns the rect of the viewport which includes scroll positions and size.
    * @return {!LayoutRect}
@@ -491,11 +501,17 @@ class ViewportBinding {
   getScrollLeft() {}
 
   /**
-   * Returns the scroll width of the content within the viewport.
+   * Returns the scroll width of the content of the document.
    * @return {number}
    */
   getScrollWidth() {}
 
+  /**
+   * Returns the scroll height of the content of the document.
+   * @return {number}
+   */
+  getScrollHeight() {}
+
   /**
    * Returns the rect of the element within the document.
    * @param {!Element} el
@@ -597,6 +613,11 @@ export class ViewportBindingNatural_ {
     return this.getScrollingElement_()./*OK*/scrollWidth;
   }
 
+  /** @override */
+  getScrollHeight() {
+    return this.getScrollingElement_()./*OK*/scrollHeight;
+  }
+
   /** @override */
   getLayoutRect(el) {
     const scrollTop = this.getScrollTop();
@@ -743,6 +764,21 @@ export class ViewportBindingNaturalIosEmbed_ {
     });
     documentBody.appendChild(this.scrollMoveEl_);
 
+    // Insert endPos element into DOM. See {@link getScrollHeight} for why
+    // this is needed.
+    this.endPosEl_ = this.win.document.createElement('div');
+    this.endPosEl_.id = '-amp-endpos';
+    setStyles(this.endPosEl_, {
+      width: 0,
+      height: 0,
+      visibility: 'hidden'
+    });
+    // TODO(dvoytenko): not only it should be at the bottom at setup time,
+    // but it must always be at the bottom. Consider using BODY "childList"
+    // mutations to track this. For now, however, this is ok since we don't
+    // allow arbitrary content inserted into BODY.
+    documentBody.appendChild(this.endPosEl_);
+
     documentBody.addEventListener('scroll', this.onScrolled_.bind(this));
   }
 
@@ -796,6 +832,21 @@ export class ViewportBindingNaturalIosEmbed_ {
     return Math.max(this.scrollWidth_, this.win./*OK*/innerWidth);
   }
 
+  /** @override */
+  getScrollHeight() {
+    // We have to use a special "tail" element on iOS due to the issues outlined
+    // in the {@link onScrolled_} method. Because we are forced to layout BODY
+    // with position:absolute, we can no longer use BODY's scrollHeight to
+    // determine scrolling height - it will always return the viewport height.
+    // Instead, we append the "tail" element as the last child of BODY and use
+    // it's viewport-relative position to calculate scrolling height.
+    if (!this.endPosEl_) {
+      return 0;
+    }
+    return Math.round(this.endPosEl_./*OK*/getBoundingClientRect().top -
+        this.scrollPosEl_./*OK*/getBoundingClientRect().top);
+  }
+
   /** @override */
   getLayoutRect(el) {
     const b = el./*OK*/getBoundingClientRect();
@@ -971,6 +1022,11 @@ export class ViewportBindingVirtual_ {
     return this.win.document.documentElement./*OK*/scrollWidth;
   }
 
+  /** @override */
+  getScrollHeight() {
+    return this.win.document.documentElement./*OK*/scrollHeight;
+  }
+
   /**
    * Returns the rect of the element within the document.
    * @param {!Element} el

test/functional/test-resources.js

@@ -454,6 +454,7 @@ describe('Resources changeHeight', () => {
 
   describe('requestChangeHeight rules when element is in viewport', () => {
     let overflowCallbackSpy;
+    let vsyncSpy;
 
     beforeEach(() => {
       overflowCallbackSpy = sinon.spy();
@@ -462,6 +463,12 @@ describe('Resources changeHeight', () => {
           {top: 0, left: 0, right: 100, bottom: 200, height: 200}).once();
       resource1.layoutBox_ = {top: 10, left: 0, right: 100, bottom: 50,
           height: 50};
+      vsyncSpy = sandbox.stub(resources.vsync_, 'run');
+    });
+
+    afterEach(() => {
+      vsyncSpy.reset();
+      vsyncSpy.restore();
     });
 
     it('should NOT change height and calls overflowCallback', () => {
@@ -550,6 +557,60 @@ describe('Resources changeHeight', () => {
       expect(resource1.changeHeight.callCount).to.equal(0);
       expect(overflowCallbackSpy.callCount).to.equal(0);
     });
+
+    it('should change height when above the vp and adjust scrolling', () => {
+      viewportMock.expects('getScrollHeight').returns(2999).once();
+      viewportMock.expects('getScrollTop').returns(1777).once();
+      resource1.layoutBox_ = {top: -1200, left: 0, right: 100, bottom: -1050,
+          height: 50};
+      resources.lastVelocity_ = 0;
+      clock.tick(5000);
+      resources.scheduleChangeHeight_(resource1, 111, false);
+      resources.mutateWork_();
+      expect(resources.requestsChangeHeight_.length).to.equal(0);
+      expect(resource1.changeHeight.callCount).to.equal(0);
+
+      expect(vsyncSpy.callCount).to.be.greaterThan(1);
+      const task = vsyncSpy.lastCall.args[0];
+      const state = {};
+      task.measure(state);
+      expect(state.scrollTop).to.equal(1777);
+      expect(state.scrollHeight).to.equal(2999);
+
+      viewportMock.expects('getScrollHeight').returns(3999).once();
+      viewportMock.expects('setScrollTop').withExactArgs(2777).once();
+      task.mutate(state);
+      expect(resource1.changeHeight.callCount).to.equal(1);
+      expect(resource1.changeHeight.firstCall.args[0]).to.equal(111);
+      expect(resources.relayoutTop_).to.equal(resource1.layoutBox_.top);
+    });
+
+    it('should NOT adjust scrolling if height did not increase', () => {
+      viewportMock.expects('getScrollHeight').returns(2999).once();
+      viewportMock.expects('getScrollTop').returns(1777).once();
+      resource1.layoutBox_ = {top: -1200, left: 0, right: 100, bottom: -1050,
+          height: 50};
+      resources.lastVelocity_ = 0;
+      clock.tick(5000);
+      resources.scheduleChangeHeight_(resource1, 111, false);
+      resources.mutateWork_();
+      expect(resources.requestsChangeHeight_.length).to.equal(0);
+      expect(resource1.changeHeight.callCount).to.equal(0);
+
+      expect(vsyncSpy.callCount).to.be.greaterThan(1);
+      const task = vsyncSpy.lastCall.args[0];
+      const state = {};
+      task.measure(state);
+      expect(state.scrollTop).to.equal(1777);
+      expect(state.scrollHeight).to.equal(2999);
+
+      viewportMock.expects('getScrollHeight').returns(2999).once();
+      viewportMock.expects('setScrollTop').never();
+      task.mutate(state);
+      expect(resource1.changeHeight.callCount).to.equal(1);
+      expect(resource1.changeHeight.firstCall.args[0]).to.equal(111);
+      expect(resources.relayoutTop_).to.equal(resource1.layoutBox_.top);
+    });
   });
 });
 

test/functional/test-viewport.js

@@ -193,6 +193,12 @@ describe('Viewport', () => {
     bindingMock.expects('getScrollWidth').withArgs().returns(111).once();
     expect(viewport.getScrollWidth()).to.equal(111);
   });
+
+  it('should deletegate scrollHeight', () => {
+    const bindingMock = sandbox.mock(binding);
+    bindingMock.expects('getScrollHeight').withArgs().returns(117).once();
+    expect(viewport.getScrollHeight()).to.equal(117);
+  });
 });
 
 
@@ -520,6 +526,16 @@ describe('ViewportBindingNatural', () => {
     expect(binding.getScrollWidth()).to.equal(117);
   });
 
+  it('should calculate scrollHeight from scrollElement', () => {
+    windowApi.pageYOffset = 11;
+    windowApi.document = {
+      scrollingElement: {
+        scrollHeight: 119
+      }
+    };
+    expect(binding.getScrollHeight()).to.equal(119);
+  });
+
   it('should update scrollTop on scrollElement', () => {
     windowApi.pageYOffset = 11;
     windowApi.document = {
@@ -582,6 +598,7 @@ describe('ViewportBindingNaturalIosEmbed', () => {
       documentElement: {style: {}},
       body: {
         scrollWidth: 777,
+        scrollHeight: 999,
         style: {},
         appendChild: child => {
           bodyChildren.push(child);
@@ -645,7 +662,7 @@ describe('ViewportBindingNaturalIosEmbed', () => {
     expect(body.style.right).to.equal(0);
     expect(body.style.bottom).to.equal(0);
 
-    expect(bodyChildren.length).to.equal(2);
+    expect(bodyChildren.length).to.equal(3);
 
     expect(bodyChildren[0].id).to.equal('-amp-scrollpos');
     expect(bodyChildren[0].style.position).to.equal('absolute');
@@ -662,6 +679,13 @@ describe('ViewportBindingNaturalIosEmbed', () => {
     expect(bodyChildren[1].style.width).to.equal(0);
     expect(bodyChildren[1].style.height).to.equal(0);
     expect(bodyChildren[1].style.visibility).to.equal('hidden');
+
+    expect(bodyChildren[2].id).to.equal('-amp-endpos');
+    expect(bodyChildren[2].style.position).to.be.undefined;
+    expect(bodyChildren[2].style.top).to.be.undefined;
+    expect(bodyChildren[2].style.width).to.equal(0);
+    expect(bodyChildren[2].style.height).to.equal(0);
+    expect(bodyChildren[2].style.visibility).to.equal('hidden');
   });
 
   it('should update padding on BODY', () => {
@@ -689,6 +713,16 @@ describe('ViewportBindingNaturalIosEmbed', () => {
     expect(binding.getScrollTop()).to.equal(17);
   });
 
+  it('should calculate scrollHeight from scrollpos/endpos elements', () => {
+    bodyChildren[0].getBoundingClientRect = () => {
+      return {top: -17, left: -11};
+    };
+    bodyChildren[2].getBoundingClientRect = () => {
+      return {top: 100, left: -11};
+    };
+    expect(binding.getScrollHeight()).to.equal(117);
+  });
+
   it('should update scroll position via moving element', () => {
     const moveEl = bodyChildren[1];
     binding.setScrollTop(17);