From 9b3a31acfc0462aa2cfdc8eb66a86b2b196d4444 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Sat, 28 Nov 2015 21:21:02 -0800 Subject: [PATCH] Automatically adjust scrolling position when resizing elements above the viewport --- src/resources.js | 50 ++++++++++++++++++++++--- src/viewport.js | 60 +++++++++++++++++++++++++++++- test/functional/test-resources.js | 61 +++++++++++++++++++++++++++++++ test/functional/test-viewport.js | 36 +++++++++++++++++- 4 files changed, 198 insertions(+), 9 deletions(-) diff --git a/src/resources.js b/src/resources.js index 6674843d1141..9cab1ca49be8 100644 --- a/src/resources.js +++ b/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)); + } + } + }); + } } } diff --git a/src/viewport.js b/src/viewport.js index 50a8ca99d51d..17fa4cd6847f 100644 --- a/src/viewport.js +++ b/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 diff --git a/test/functional/test-resources.js b/test/functional/test-resources.js index 5efdfa1d23b5..ffd457ebbb92 100644 --- a/test/functional/test-resources.js +++ b/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); + }); }); }); diff --git a/test/functional/test-viewport.js b/test/functional/test-viewport.js index 06348204eff3..509001b49ef5 100644 --- a/test/functional/test-viewport.js +++ b/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);