From b3633635d4e2432f214c1b7cf1a528468292e2d4 Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Sat, 2 Nov 2019 16:02:41 -0700 Subject: [PATCH 01/16] Manual management of document visibility --- .../amp-next-page/0.1/next-page-service.js | 85 +++++++++++++++---- 1 file changed, 67 insertions(+), 18 deletions(-) diff --git a/extensions/amp-next-page/0.1/next-page-service.js b/extensions/amp-next-page/0.1/next-page-service.js index 51dc3357c1eb..79aef1f546f6 100644 --- a/extensions/amp-next-page/0.1/next-page-service.js +++ b/extensions/amp-next-page/0.1/next-page-service.js @@ -18,6 +18,7 @@ import {CSS} from '../../../build/amp-next-page-0.1.css'; import {MultidocManager} from '../../../src/runtime'; import {PositionObserverFidelity} from '../../../src/service/position-observer/position-observer-worker'; import {Services} from '../../../src/services'; +import {VisibilityState} from '../../../src/visibility-state'; import {dev, user, userAssert} from '../../../src/log'; import {dict} from '../../../src/utils/object'; import {getAmpdoc} from '../../../src/service'; @@ -173,6 +174,16 @@ export class NextPageService { win.document.title, canonicalUrl ); + + // TODO(gharbiw) Establish parity with the shadow doc amp object + documentRef.amp = { + ampdoc: ampDoc, + url: win.document.location.href, + title: win.document.title, + canonicalUrl, + setVisibilityState: ampDoc.overrideVisibilityState.bind(ampDoc), + }; + this.documentRefs_.push(documentRef); this.activeDocumentRef_ = this.documentRefs_[0]; @@ -216,7 +227,9 @@ export class NextPageService { removeElement(item); } - const amp = this.multidocManager_.attachShadowDoc(shadowRoot, doc, '', {}); + const amp = this.multidocManager_.attachShadowDoc(shadowRoot, doc, '', { + visibilityState: VisibilityState.PRERENDER, + }); installStylesForDoc(amp.ampdoc, CSS, null, false, TAG); const body = amp.ampdoc.getBody(); @@ -457,24 +470,32 @@ export class NextPageService { return; } - let documentRef; + let documentIndex; let analyticsEvent = ''; - if (position.relativePos === 'top') { - documentRef = this.documentRefs_[i + 1]; - analyticsEvent = 'amp-next-page-scroll'; - } else if (position.relativePos === 'bottom') { - documentRef = this.documentRefs_[i]; - analyticsEvent = 'amp-next-page-scroll-back'; + switch (position.relativePos) { + case 'top': + documentIndex = i + 1; + analyticsEvent = 'amp-next-page-scroll'; + break; + case 'bottom': + documentIndex = i; + analyticsEvent = 'amp-next-page-scroll-back'; + break; + default: + break; } - if (documentRef && documentRef.amp) { + if ( + this.documentRefs_[documentIndex] && + this.documentRefs_[documentIndex].amp + ) { this.triggerAnalyticsEvent_( analyticsEvent, - documentRef.ampUrl, + this.documentRefs_[documentIndex].ampUrl, this.activeDocumentRef_.ampUrl ); - this.setActiveDocument_(documentRef); + this.setActiveDocument_(documentIndex); } } @@ -497,19 +518,47 @@ export class NextPageService { /** * Sets the specified document as active, updating the document title and URL. - * @param {!DocumentRef} documentRef Reference to the document to set as - * active. + * + * @param {number} documentIndex Index of the document to be activated * @private */ - setActiveDocument_(documentRef) { - const {amp} = documentRef; - this.win_.document.title = amp.title || ''; - this.activeDocumentRef_ = documentRef; - this.setActiveDocumentInHistory_(documentRef); + setActiveDocument_(documentIndex) { + this.documentRefs_.forEach((docRef, index) => { + const {amp} = docRef; + let updatedVisibilityState; + // Update the title and history + if (index == documentIndex) { + this.win_.document.title = amp.title || ''; + this.activeDocumentRef_ = docRef; + this.setActiveDocumentInHistory_(docRef); + updatedVisibilityState = VisibilityState.VISIBLE; + } else if (index !== 0) { + updatedVisibilityState = VisibilityState.HIDDEN; + } + // Show the active doc and hide other docs + if (updatedVisibilityState) { + this.setDocumentVisibility_(index, updatedVisibilityState); + } + }); // TODO(emarchiori): Consider updating position fixed elements. } + /** + * Manually overrides the document's visible state to the given state + * + * @param {number} documentIndex Index of the document to change + * @param {!../../../src/visibility-state.VisibilityState} visibilityState + * @private + */ + setDocumentVisibility_(documentIndex, visibilityState) { + const doc = this.documentRefs_[documentIndex]; + + if (doc && doc.amp && doc.amp['ampdoc']) { + doc.amp.setVisibilityState(visibilityState); + } + } + /** * @param {!DocumentRef} documentRef * @private From 8bc5944ffd2e76edef512eebeff5505243a1c01a Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Mon, 4 Nov 2019 00:11:23 -0800 Subject: [PATCH 02/16] Edge cases and tests --- .../amp-next-page/0.1/next-page-service.js | 13 ++- .../0.1/test/test-amp-next-page.js | 105 ++++++++++++------ 2 files changed, 84 insertions(+), 34 deletions(-) diff --git a/extensions/amp-next-page/0.1/next-page-service.js b/extensions/amp-next-page/0.1/next-page-service.js index 79aef1f546f6..36e29ab72ba4 100644 --- a/extensions/amp-next-page/0.1/next-page-service.js +++ b/extensions/amp-next-page/0.1/next-page-service.js @@ -181,7 +181,6 @@ export class NextPageService { url: win.document.location.href, title: win.document.title, canonicalUrl, - setVisibilityState: ampDoc.overrideVisibilityState.bind(ampDoc), }; this.documentRefs_.push(documentRef); @@ -552,9 +551,21 @@ export class NextPageService { * @private */ setDocumentVisibility_(documentIndex, visibilityState) { + // Prevent updating visibility of the host document + if (documentIndex === 0) { + return; + } + const doc = this.documentRefs_[documentIndex]; if (doc && doc.amp && doc.amp['ampdoc']) { + // Prevent hiding of documents that are being pre-rendered + if ( + !doc.amp.ampdoc.hasBeenVisible() && + visibilityState == VisibilityState.HIDDEN + ) { + return; + } doc.amp.setVisibilityState(visibilityState); } } diff --git a/extensions/amp-next-page/0.1/test/test-amp-next-page.js b/extensions/amp-next-page/0.1/test/test-amp-next-page.js index 85223dab47e2..1143ca82aa56 100644 --- a/extensions/amp-next-page/0.1/test/test-amp-next-page.js +++ b/extensions/amp-next-page/0.1/test/test-amp-next-page.js @@ -16,6 +16,7 @@ import * as DocFetcher from '../../../../src/document-fetcher'; import {AmpNextPage} from '../amp-next-page'; import {Services} from '../../../../src/services'; +import {VisibilityState} from '../../../../src/visibility-state'; import {getServicePromiseForDoc} from '../../../../src/service'; import {layoutRectLtwh} from '../../../../src/layout-rect'; import {macroTask} from '../../../../testing/yield'; @@ -110,12 +111,10 @@ describes.realWin( it('does not fetch the next document before 3 viewports away', function*() { const xhrMock = sandbox.mock(Services.xhrFor(win)); xhrMock.expects('fetch').never(); - sandbox.stub(viewport, 'getClientRectAsync').callsFake(() => { + sandbox + .stub(viewport, 'getClientRectAsync') // 4x viewports away - return Promise.resolve( - layoutRectLtwh(0, 0, sizes.width, sizes.height * 5) - ); - }); + .resolves(layoutRectLtwh(0, 0, sizes.width, sizes.height * 5)); win.dispatchEvent(new Event('scroll')); yield macroTask(); @@ -125,12 +124,10 @@ describes.realWin( it('fetches the next document within 3 viewports away', function*() { env.fetchMock.get('*', EXAMPLE_PAGE); - sandbox.stub(viewport, 'getClientRectAsync').callsFake(() => { + sandbox + .stub(viewport, 'getClientRectAsync') // 1x viewport away - return Promise.resolve( - layoutRectLtwh(0, 0, sizes.width, sizes.height * 2) - ); - }); + .resolves(layoutRectLtwh(0, 0, sizes.width, sizes.height * 2)); win.dispatchEvent(new Event('scroll')); yield macroTask(); @@ -146,12 +143,10 @@ describes.realWin( .returns(new Promise(() => {})) .once(); - sandbox.stub(viewport, 'getClientRectAsync').callsFake(() => { + sandbox + .stub(viewport, 'getClientRectAsync') // 1x viewport away - return Promise.resolve( - layoutRectLtwh(0, 0, sizes.width, sizes.height * 2) - ); - }); + .resolves(layoutRectLtwh(0, 0, sizes.width, sizes.height * 2)); win.dispatchEvent(new Event('scroll')); yield macroTask(); @@ -175,12 +170,8 @@ describes.realWin( sandbox .stub(viewport, 'getClientRectAsync') .onFirstCall() - .callsFake(() => { - // 1x viewport away - return Promise.resolve( - layoutRectLtwh(0, 0, sizes.width, sizes.height * 2) - ); - }); + // 1x viewport away + .resolves(layoutRectLtwh(0, 0, sizes.width, sizes.height * 2)); win.dispatchEvent(new Event('scroll')); yield macroTask(); @@ -216,12 +207,8 @@ describes.realWin( sandbox .stub(viewport, 'getClientRectAsync') .onFirstCall() - .callsFake(() => { - // 1x viewport away - return Promise.resolve( - layoutRectLtwh(0, 0, sizes.width, sizes.height * 2) - ); - }); + // 1x viewport away + .resolves(layoutRectLtwh(0, 0, sizes.width, sizes.height * 2)); win.dispatchEvent(new Event('scroll')); yield macroTask(); const shadowDoc = attachShadowDocSpy.firstCall.returnValue.ampdoc; @@ -249,12 +236,8 @@ describes.realWin( sandbox .stub(viewport, 'getClientRectAsync') .onFirstCall() - .callsFake(() => { - // 1x viewport away - return Promise.resolve( - layoutRectLtwh(0, 0, sizes.width, sizes.height * 2) - ); - }); + // 1x viewport away + .resolves(layoutRectLtwh(0, 0, sizes.width, sizes.height * 2)); win.dispatchEvent(new Event('scroll')); yield macroTask(); @@ -528,6 +511,62 @@ describes.realWin( expect(registerSpy.calledWith(element, config)).to.be.true; }); }); + + describe('manual visibility management', () => { + beforeEach(done => { + element.innerHTML = ` + `; + nextPage.buildCallback().then(done); + }); + + it('defaults to the prerender visibility state for the next document', function*() { + env.fetchMock.get('*', EXAMPLE_PAGE); + + const nextPageService = yield getServicePromiseForDoc( + ampdoc, + 'next-page' + ); + const attachShadowDocSpy = sandbox.spy( + nextPageService.multidocManager_, + 'attachShadowDoc' + ); + + sandbox + .stub(viewport, 'getClientRectAsync') + .onFirstCall() + // 1x viewport away + .resolves(layoutRectLtwh(0, 0, sizes.width, sizes.height * 2)); + + win.dispatchEvent(new Event('scroll')); + yield macroTask(); + + const shadowDoc = attachShadowDocSpy.firstCall.returnValue.ampdoc; + yield shadowDoc.whenReady(); + + expect(shadowDoc.getVisibilityState()).to.equal( + VisibilityState.PRERENDER + ); + }); + }); } ); From 76024bcc16d0f2e9f1b33c72ee8fd5c8a27ca81a Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Mon, 4 Nov 2019 16:20:13 -0800 Subject: [PATCH 03/16] Fixed types --- .../amp-next-page/0.1/next-page-service.js | 8 +++----- src/runtime.js | 18 +++++++++++++++++- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/extensions/amp-next-page/0.1/next-page-service.js b/extensions/amp-next-page/0.1/next-page-service.js index 36e29ab72ba4..b93696559e1d 100644 --- a/extensions/amp-next-page/0.1/next-page-service.js +++ b/extensions/amp-next-page/0.1/next-page-service.js @@ -175,7 +175,7 @@ export class NextPageService { canonicalUrl ); - // TODO(gharbiw) Establish parity with the shadow doc amp object + // TODO(wassgha): Establish parity with the shadow doc amp object documentRef.amp = { ampdoc: ampDoc, url: win.document.location.href, @@ -325,8 +325,7 @@ export class NextPageService { } this.resources_.mutateElement(container, () => { try { - const amp = this.attachShadowDoc_(shadowRoot, doc); - documentRef.amp = amp; + documentRef.amp = this.attachShadowDoc_(shadowRoot, doc); toggle(dev().assertElement(documentRef.recUnit.el), false); this.documentQueued_ = false; @@ -469,7 +468,7 @@ export class NextPageService { return; } - let documentIndex; + let documentIndex = i; let analyticsEvent = ''; switch (position.relativePos) { @@ -478,7 +477,6 @@ export class NextPageService { analyticsEvent = 'amp-next-page-scroll'; break; case 'bottom': - documentIndex = i; analyticsEvent = 'amp-next-page-scroll-back'; break; default: diff --git a/src/runtime.js b/src/runtime.js index 9259a8135915..1fdb9a139ab9 100644 --- a/src/runtime.js +++ b/src/runtime.js @@ -67,6 +67,22 @@ setReportError(reportErrorForWin.bind(null, self)); /** @const @private {string} */ const TAG = 'runtime'; +/** + * @typedef {{ + * url: string, + * ampdoc: !./service/ampdoc-impl.AmpDocShadow, + * setVisibilityState: (Function|undefined), + * postMessage: (Function|undefined), + * onMessage: (Function|undefined), + * close: (Function|undefined), + * getState: (Function|undefined), + * setState: (Function|undefined), + * toggleRuntime: (Function|undefined), + * resources: !./service/resources-interface.ResourcesInterface + * }} + */ +export let ShadowDoc; + /** * Applies the runtime to a given global scope for a single-doc mode. Multi * frame support is currently incomplete. @@ -420,7 +436,7 @@ export class MultidocManager { * @param {!Object|undefined} params * @param {function(!Object, !ShadowRoot, * !./service/ampdoc-impl.AmpDocShadow):!Promise} builder - * @return {!Object} + * @return {!ShadowDoc} * @private */ attachShadowDoc_(hostElement, url, params, builder) { From 346e3d8bc50eecb759a8bd7fc58820b4b7bd8fae Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Wed, 6 Nov 2019 12:58:39 -0800 Subject: [PATCH 04/16] Requested changes --- .../amp-next-page/0.1/next-page-service.js | 35 +++++++++---------- src/runtime.js | 17 +++++---- 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/extensions/amp-next-page/0.1/next-page-service.js b/extensions/amp-next-page/0.1/next-page-service.js index b93696559e1d..7aa423921365 100644 --- a/extensions/amp-next-page/0.1/next-page-service.js +++ b/extensions/amp-next-page/0.1/next-page-service.js @@ -522,19 +522,16 @@ export class NextPageService { setActiveDocument_(documentIndex) { this.documentRefs_.forEach((docRef, index) => { const {amp} = docRef; - let updatedVisibilityState; // Update the title and history - if (index == documentIndex) { + if (index === documentIndex) { this.win_.document.title = amp.title || ''; this.activeDocumentRef_ = docRef; this.setActiveDocumentInHistory_(docRef); - updatedVisibilityState = VisibilityState.VISIBLE; - } else if (index !== 0) { - updatedVisibilityState = VisibilityState.HIDDEN; - } - // Show the active doc and hide other docs - if (updatedVisibilityState) { - this.setDocumentVisibility_(index, updatedVisibilityState); + // Show the active document + this.setDocumentVisibility_(index, VisibilityState.VISIBLE); + } else { + // Hide other documents + this.setDocumentVisibility_(index, VisibilityState.HIDDEN); } }); @@ -555,17 +552,19 @@ export class NextPageService { } const doc = this.documentRefs_[documentIndex]; + const ampDoc = doc && doc.amp && doc.amp.ampdoc; - if (doc && doc.amp && doc.amp['ampdoc']) { - // Prevent hiding of documents that are being pre-rendered - if ( - !doc.amp.ampdoc.hasBeenVisible() && - visibilityState == VisibilityState.HIDDEN - ) { - return; - } - doc.amp.setVisibilityState(visibilityState); + // Prevent hiding of documents that are not shadow docs + if (!ampDoc) { + return; } + + // Prevent hiding of documents that are being pre-rendered + if (!ampDoc.hasBeenVisible() && visibilityState == VisibilityState.HIDDEN) { + return; + } + + doc.amp.setVisibilityState(visibilityState); } /** diff --git a/src/runtime.js b/src/runtime.js index 1fdb9a139ab9..9f3c0e55923f 100644 --- a/src/runtime.js +++ b/src/runtime.js @@ -70,14 +70,17 @@ const TAG = 'runtime'; /** * @typedef {{ * url: string, + * title: string, + * canonicalUrl: string, + * head: (Element|undefined), * ampdoc: !./service/ampdoc-impl.AmpDocShadow, - * setVisibilityState: (Function|undefined), - * postMessage: (Function|undefined), - * onMessage: (Function|undefined), - * close: (Function|undefined), - * getState: (Function|undefined), - * setState: (Function|undefined), - * toggleRuntime: (Function|undefined), + * setVisibilityState: (function()|undefined), + * postMessage: (function()|undefined), + * onMessage: (function()|undefined), + * close: (function()|undefined), + * getState: (function()|undefined), + * setState: (function()|undefined), + * toggleRuntime: (function()|undefined), * resources: !./service/resources-interface.ResourcesInterface * }} */ From b1f74f25477ef899a63a22792d576d4db2d27829 Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Wed, 6 Nov 2019 14:45:19 -0800 Subject: [PATCH 05/16] Fixed types --- .../amp-next-page/0.1/next-page-service.js | 18 ++++++++++++------ src/runtime.js | 14 +++++++------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/extensions/amp-next-page/0.1/next-page-service.js b/extensions/amp-next-page/0.1/next-page-service.js index 7aa423921365..c15ff1898ee4 100644 --- a/extensions/amp-next-page/0.1/next-page-service.js +++ b/extensions/amp-next-page/0.1/next-page-service.js @@ -41,7 +41,7 @@ const TAG = 'amp-next-page'; /** * @typedef {{ * ampUrl: string, - * amp: ?Object, + * amp: (?../../../src/runtime.ShadowDoc | undefined), * recUnit: {el: ?Element, isObserving: boolean}, * cancelled: boolean * }} @@ -175,13 +175,15 @@ export class NextPageService { canonicalUrl ); - // TODO(wassgha): Establish parity with the shadow doc amp object - documentRef.amp = { + // TODO(wassgha): Untype as ShadowDoc and tighten the ShadowDoc type spec + /** @type {!../../../src/runtime.ShadowDoc} */ + const amp = { ampdoc: ampDoc, url: win.document.location.href, title: win.document.title, canonicalUrl, }; + documentRef.amp = amp; this.documentRefs_.push(documentRef); this.activeDocumentRef_ = this.documentRefs_[0]; @@ -208,7 +210,7 @@ export class NextPageService { * Attach a ShadowDoc using the given document. * @param {!Element} shadowRoot Root element to attach the shadow document to. * @param {!Document} doc Document to attach. - * @return {?Object} Return value of {@link MultidocManager#attachShadowDoc} + * @return {?../../../src/runtime.ShadowDoc} Return value of {@link MultidocManager#attachShadowDoc} */ attachShadowDoc_(shadowRoot, doc) { if (this.hideSelector_) { @@ -226,12 +228,16 @@ export class NextPageService { removeElement(item); } + /** @type {!../../../src/runtime.ShadowDoc} */ const amp = this.multidocManager_.attachShadowDoc(shadowRoot, doc, '', { visibilityState: VisibilityState.PRERENDER, }); - installStylesForDoc(amp.ampdoc, CSS, null, false, TAG); + const ampdoc = /** @type {!../../../src/service/ampdoc-impl.AmpDoc} */ (dev().assert( + amp.ampdoc + )); + installStylesForDoc(ampdoc, CSS, null, false, TAG); - const body = amp.ampdoc.getBody(); + const body = ampdoc.getBody(); body.classList.add('i-amphtml-next-page-document'); return amp; diff --git a/src/runtime.js b/src/runtime.js index 9f3c0e55923f..aa3152cdfda8 100644 --- a/src/runtime.js +++ b/src/runtime.js @@ -69,19 +69,19 @@ const TAG = 'runtime'; /** * @typedef {{ - * url: string, - * title: string, - * canonicalUrl: string, + * url: (string|undefined), + * title: (string|undefined), + * canonicalUrl: (string|undefined), * head: (Element|undefined), - * ampdoc: !./service/ampdoc-impl.AmpDocShadow, - * setVisibilityState: (function()|undefined), + * ampdoc: (!./service/ampdoc-impl.AmpDoc | undefined), + * setVisibilityState: (function(!VisibilityState)|undefined), * postMessage: (function()|undefined), * onMessage: (function()|undefined), * close: (function()|undefined), * getState: (function()|undefined), * setState: (function()|undefined), * toggleRuntime: (function()|undefined), - * resources: !./service/resources-interface.ResourcesInterface + * resources: (!./service/resources-interface.ResourcesInterface | undefined) * }} */ export let ShadowDoc; @@ -593,7 +593,7 @@ export class MultidocManager { * @param {!Document} doc * @param {string} url * @param {!Object=} opt_initParams - * @return {!Object} + * @return {!ShadowDoc} */ attachShadowDoc(hostElement, doc, url, opt_initParams) { dev().fine(TAG, 'Attach shadow doc:', doc); From 8bbb0cfd074e08e1bd65aef7cc86200fa5981a20 Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Wed, 6 Nov 2019 15:19:34 -0800 Subject: [PATCH 06/16] Removed unnecessary cast --- extensions/amp-next-page/0.1/next-page-service.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/extensions/amp-next-page/0.1/next-page-service.js b/extensions/amp-next-page/0.1/next-page-service.js index c15ff1898ee4..a2a1ab655595 100644 --- a/extensions/amp-next-page/0.1/next-page-service.js +++ b/extensions/amp-next-page/0.1/next-page-service.js @@ -232,9 +232,7 @@ export class NextPageService { const amp = this.multidocManager_.attachShadowDoc(shadowRoot, doc, '', { visibilityState: VisibilityState.PRERENDER, }); - const ampdoc = /** @type {!../../../src/service/ampdoc-impl.AmpDoc} */ (dev().assert( - amp.ampdoc - )); + const ampdoc = dev().assert(amp.ampdoc); installStylesForDoc(ampdoc, CSS, null, false, TAG); const body = ampdoc.getBody(); From 986ed28a8dd6408b463620b0ccf0132efe306710 Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Thu, 7 Nov 2019 16:16:56 -0800 Subject: [PATCH 07/16] Switched to using refs instead of indices --- .../amp-next-page/0.1/next-page-service.js | 40 +++++++++---------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/extensions/amp-next-page/0.1/next-page-service.js b/extensions/amp-next-page/0.1/next-page-service.js index a2a1ab655595..86354f9487b5 100644 --- a/extensions/amp-next-page/0.1/next-page-service.js +++ b/extensions/amp-next-page/0.1/next-page-service.js @@ -19,7 +19,7 @@ import {MultidocManager} from '../../../src/runtime'; import {PositionObserverFidelity} from '../../../src/service/position-observer/position-observer-worker'; import {Services} from '../../../src/services'; import {VisibilityState} from '../../../src/visibility-state'; -import {dev, user, userAssert} from '../../../src/log'; +import {dev, devAssert, user, userAssert} from '../../../src/log'; import {dict} from '../../../src/utils/object'; import {getAmpdoc} from '../../../src/service'; import {installPositionObserverServiceForDoc} from '../../../src/service/position-observer/position-observer-impl'; @@ -232,7 +232,7 @@ export class NextPageService { const amp = this.multidocManager_.attachShadowDoc(shadowRoot, doc, '', { visibilityState: VisibilityState.PRERENDER, }); - const ampdoc = dev().assert(amp.ampdoc); + const ampdoc = devAssert(amp.ampdoc); installStylesForDoc(ampdoc, CSS, null, false, TAG); const body = ampdoc.getBody(); @@ -472,12 +472,12 @@ export class NextPageService { return; } - let documentIndex = i; + let ref = this.documentRefs_[i]; let analyticsEvent = ''; switch (position.relativePos) { case 'top': - documentIndex = i + 1; + ref = this.documentRefs_[i + 1]; analyticsEvent = 'amp-next-page-scroll'; break; case 'bottom': @@ -487,16 +487,13 @@ export class NextPageService { break; } - if ( - this.documentRefs_[documentIndex] && - this.documentRefs_[documentIndex].amp - ) { + if (ref && ref.amp) { this.triggerAnalyticsEvent_( analyticsEvent, - this.documentRefs_[documentIndex].ampUrl, + ref.ampUrl, this.activeDocumentRef_.ampUrl ); - this.setActiveDocument_(documentIndex); + this.setActiveDocument_(ref); } } @@ -520,22 +517,22 @@ export class NextPageService { /** * Sets the specified document as active, updating the document title and URL. * - * @param {number} documentIndex Index of the document to be activated + * @param {DocumentRef} ref Reference to the document to be activated * @private */ - setActiveDocument_(documentIndex) { - this.documentRefs_.forEach((docRef, index) => { + setActiveDocument_(ref) { + this.documentRefs_.forEach(docRef => { const {amp} = docRef; // Update the title and history - if (index === documentIndex) { + if (docRef === ref) { this.win_.document.title = amp.title || ''; this.activeDocumentRef_ = docRef; this.setActiveDocumentInHistory_(docRef); // Show the active document - this.setDocumentVisibility_(index, VisibilityState.VISIBLE); + this.setDocumentVisibility_(docRef, VisibilityState.VISIBLE); } else { // Hide other documents - this.setDocumentVisibility_(index, VisibilityState.HIDDEN); + this.setDocumentVisibility_(docRef, VisibilityState.HIDDEN); } }); @@ -545,18 +542,17 @@ export class NextPageService { /** * Manually overrides the document's visible state to the given state * - * @param {number} documentIndex Index of the document to change + * @param {DocumentRef} ref Reference to the document to change * @param {!../../../src/visibility-state.VisibilityState} visibilityState * @private */ - setDocumentVisibility_(documentIndex, visibilityState) { + setDocumentVisibility_(ref, visibilityState) { // Prevent updating visibility of the host document - if (documentIndex === 0) { + if (ref === this.documentRefs_[0]) { return; } - const doc = this.documentRefs_[documentIndex]; - const ampDoc = doc && doc.amp && doc.amp.ampdoc; + const ampDoc = ref && ref.amp && ref.amp.ampdoc; // Prevent hiding of documents that are not shadow docs if (!ampDoc) { @@ -568,7 +564,7 @@ export class NextPageService { return; } - doc.amp.setVisibilityState(visibilityState); + ref.amp.setVisibilityState(visibilityState); } /** From 5e71a1a8c2a9ba17e8a5f1c616b293695e6332ab Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Thu, 7 Nov 2019 16:32:59 -0800 Subject: [PATCH 08/16] Fixed non-nullable type --- extensions/amp-next-page/0.1/next-page-service.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/extensions/amp-next-page/0.1/next-page-service.js b/extensions/amp-next-page/0.1/next-page-service.js index 86354f9487b5..5bf78a52248e 100644 --- a/extensions/amp-next-page/0.1/next-page-service.js +++ b/extensions/amp-next-page/0.1/next-page-service.js @@ -517,7 +517,7 @@ export class NextPageService { /** * Sets the specified document as active, updating the document title and URL. * - * @param {DocumentRef} ref Reference to the document to be activated + * @param {!DocumentRef} ref Reference to the document to be activated * @private */ setActiveDocument_(ref) { @@ -542,7 +542,7 @@ export class NextPageService { /** * Manually overrides the document's visible state to the given state * - * @param {DocumentRef} ref Reference to the document to change + * @param {!DocumentRef} ref Reference to the document to change * @param {!../../../src/visibility-state.VisibilityState} visibilityState * @private */ @@ -552,7 +552,7 @@ export class NextPageService { return; } - const ampDoc = ref && ref.amp && ref.amp.ampdoc; + const ampDoc = ref.amp && ref.amp.ampdoc; // Prevent hiding of documents that are not shadow docs if (!ampDoc) { From 0c4eb0efafbddc99a12f09dc746670bb58c5dcd9 Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Sat, 2 Nov 2019 16:02:41 -0700 Subject: [PATCH 09/16] Manual management of document visibility --- .../amp-next-page/0.1/next-page-service.js | 85 +++++++++++++++---- 1 file changed, 67 insertions(+), 18 deletions(-) diff --git a/extensions/amp-next-page/0.1/next-page-service.js b/extensions/amp-next-page/0.1/next-page-service.js index 51dc3357c1eb..79aef1f546f6 100644 --- a/extensions/amp-next-page/0.1/next-page-service.js +++ b/extensions/amp-next-page/0.1/next-page-service.js @@ -18,6 +18,7 @@ import {CSS} from '../../../build/amp-next-page-0.1.css'; import {MultidocManager} from '../../../src/runtime'; import {PositionObserverFidelity} from '../../../src/service/position-observer/position-observer-worker'; import {Services} from '../../../src/services'; +import {VisibilityState} from '../../../src/visibility-state'; import {dev, user, userAssert} from '../../../src/log'; import {dict} from '../../../src/utils/object'; import {getAmpdoc} from '../../../src/service'; @@ -173,6 +174,16 @@ export class NextPageService { win.document.title, canonicalUrl ); + + // TODO(gharbiw) Establish parity with the shadow doc amp object + documentRef.amp = { + ampdoc: ampDoc, + url: win.document.location.href, + title: win.document.title, + canonicalUrl, + setVisibilityState: ampDoc.overrideVisibilityState.bind(ampDoc), + }; + this.documentRefs_.push(documentRef); this.activeDocumentRef_ = this.documentRefs_[0]; @@ -216,7 +227,9 @@ export class NextPageService { removeElement(item); } - const amp = this.multidocManager_.attachShadowDoc(shadowRoot, doc, '', {}); + const amp = this.multidocManager_.attachShadowDoc(shadowRoot, doc, '', { + visibilityState: VisibilityState.PRERENDER, + }); installStylesForDoc(amp.ampdoc, CSS, null, false, TAG); const body = amp.ampdoc.getBody(); @@ -457,24 +470,32 @@ export class NextPageService { return; } - let documentRef; + let documentIndex; let analyticsEvent = ''; - if (position.relativePos === 'top') { - documentRef = this.documentRefs_[i + 1]; - analyticsEvent = 'amp-next-page-scroll'; - } else if (position.relativePos === 'bottom') { - documentRef = this.documentRefs_[i]; - analyticsEvent = 'amp-next-page-scroll-back'; + switch (position.relativePos) { + case 'top': + documentIndex = i + 1; + analyticsEvent = 'amp-next-page-scroll'; + break; + case 'bottom': + documentIndex = i; + analyticsEvent = 'amp-next-page-scroll-back'; + break; + default: + break; } - if (documentRef && documentRef.amp) { + if ( + this.documentRefs_[documentIndex] && + this.documentRefs_[documentIndex].amp + ) { this.triggerAnalyticsEvent_( analyticsEvent, - documentRef.ampUrl, + this.documentRefs_[documentIndex].ampUrl, this.activeDocumentRef_.ampUrl ); - this.setActiveDocument_(documentRef); + this.setActiveDocument_(documentIndex); } } @@ -497,19 +518,47 @@ export class NextPageService { /** * Sets the specified document as active, updating the document title and URL. - * @param {!DocumentRef} documentRef Reference to the document to set as - * active. + * + * @param {number} documentIndex Index of the document to be activated * @private */ - setActiveDocument_(documentRef) { - const {amp} = documentRef; - this.win_.document.title = amp.title || ''; - this.activeDocumentRef_ = documentRef; - this.setActiveDocumentInHistory_(documentRef); + setActiveDocument_(documentIndex) { + this.documentRefs_.forEach((docRef, index) => { + const {amp} = docRef; + let updatedVisibilityState; + // Update the title and history + if (index == documentIndex) { + this.win_.document.title = amp.title || ''; + this.activeDocumentRef_ = docRef; + this.setActiveDocumentInHistory_(docRef); + updatedVisibilityState = VisibilityState.VISIBLE; + } else if (index !== 0) { + updatedVisibilityState = VisibilityState.HIDDEN; + } + // Show the active doc and hide other docs + if (updatedVisibilityState) { + this.setDocumentVisibility_(index, updatedVisibilityState); + } + }); // TODO(emarchiori): Consider updating position fixed elements. } + /** + * Manually overrides the document's visible state to the given state + * + * @param {number} documentIndex Index of the document to change + * @param {!../../../src/visibility-state.VisibilityState} visibilityState + * @private + */ + setDocumentVisibility_(documentIndex, visibilityState) { + const doc = this.documentRefs_[documentIndex]; + + if (doc && doc.amp && doc.amp['ampdoc']) { + doc.amp.setVisibilityState(visibilityState); + } + } + /** * @param {!DocumentRef} documentRef * @private From 4985510a417273371429b1725a86b3b392bc933b Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Mon, 4 Nov 2019 00:11:23 -0800 Subject: [PATCH 10/16] Edge cases and tests --- .../amp-next-page/0.1/next-page-service.js | 13 ++- .../0.1/test/test-amp-next-page.js | 105 ++++++++++++------ 2 files changed, 84 insertions(+), 34 deletions(-) diff --git a/extensions/amp-next-page/0.1/next-page-service.js b/extensions/amp-next-page/0.1/next-page-service.js index 79aef1f546f6..36e29ab72ba4 100644 --- a/extensions/amp-next-page/0.1/next-page-service.js +++ b/extensions/amp-next-page/0.1/next-page-service.js @@ -181,7 +181,6 @@ export class NextPageService { url: win.document.location.href, title: win.document.title, canonicalUrl, - setVisibilityState: ampDoc.overrideVisibilityState.bind(ampDoc), }; this.documentRefs_.push(documentRef); @@ -552,9 +551,21 @@ export class NextPageService { * @private */ setDocumentVisibility_(documentIndex, visibilityState) { + // Prevent updating visibility of the host document + if (documentIndex === 0) { + return; + } + const doc = this.documentRefs_[documentIndex]; if (doc && doc.amp && doc.amp['ampdoc']) { + // Prevent hiding of documents that are being pre-rendered + if ( + !doc.amp.ampdoc.hasBeenVisible() && + visibilityState == VisibilityState.HIDDEN + ) { + return; + } doc.amp.setVisibilityState(visibilityState); } } diff --git a/extensions/amp-next-page/0.1/test/test-amp-next-page.js b/extensions/amp-next-page/0.1/test/test-amp-next-page.js index 85223dab47e2..1143ca82aa56 100644 --- a/extensions/amp-next-page/0.1/test/test-amp-next-page.js +++ b/extensions/amp-next-page/0.1/test/test-amp-next-page.js @@ -16,6 +16,7 @@ import * as DocFetcher from '../../../../src/document-fetcher'; import {AmpNextPage} from '../amp-next-page'; import {Services} from '../../../../src/services'; +import {VisibilityState} from '../../../../src/visibility-state'; import {getServicePromiseForDoc} from '../../../../src/service'; import {layoutRectLtwh} from '../../../../src/layout-rect'; import {macroTask} from '../../../../testing/yield'; @@ -110,12 +111,10 @@ describes.realWin( it('does not fetch the next document before 3 viewports away', function*() { const xhrMock = sandbox.mock(Services.xhrFor(win)); xhrMock.expects('fetch').never(); - sandbox.stub(viewport, 'getClientRectAsync').callsFake(() => { + sandbox + .stub(viewport, 'getClientRectAsync') // 4x viewports away - return Promise.resolve( - layoutRectLtwh(0, 0, sizes.width, sizes.height * 5) - ); - }); + .resolves(layoutRectLtwh(0, 0, sizes.width, sizes.height * 5)); win.dispatchEvent(new Event('scroll')); yield macroTask(); @@ -125,12 +124,10 @@ describes.realWin( it('fetches the next document within 3 viewports away', function*() { env.fetchMock.get('*', EXAMPLE_PAGE); - sandbox.stub(viewport, 'getClientRectAsync').callsFake(() => { + sandbox + .stub(viewport, 'getClientRectAsync') // 1x viewport away - return Promise.resolve( - layoutRectLtwh(0, 0, sizes.width, sizes.height * 2) - ); - }); + .resolves(layoutRectLtwh(0, 0, sizes.width, sizes.height * 2)); win.dispatchEvent(new Event('scroll')); yield macroTask(); @@ -146,12 +143,10 @@ describes.realWin( .returns(new Promise(() => {})) .once(); - sandbox.stub(viewport, 'getClientRectAsync').callsFake(() => { + sandbox + .stub(viewport, 'getClientRectAsync') // 1x viewport away - return Promise.resolve( - layoutRectLtwh(0, 0, sizes.width, sizes.height * 2) - ); - }); + .resolves(layoutRectLtwh(0, 0, sizes.width, sizes.height * 2)); win.dispatchEvent(new Event('scroll')); yield macroTask(); @@ -175,12 +170,8 @@ describes.realWin( sandbox .stub(viewport, 'getClientRectAsync') .onFirstCall() - .callsFake(() => { - // 1x viewport away - return Promise.resolve( - layoutRectLtwh(0, 0, sizes.width, sizes.height * 2) - ); - }); + // 1x viewport away + .resolves(layoutRectLtwh(0, 0, sizes.width, sizes.height * 2)); win.dispatchEvent(new Event('scroll')); yield macroTask(); @@ -216,12 +207,8 @@ describes.realWin( sandbox .stub(viewport, 'getClientRectAsync') .onFirstCall() - .callsFake(() => { - // 1x viewport away - return Promise.resolve( - layoutRectLtwh(0, 0, sizes.width, sizes.height * 2) - ); - }); + // 1x viewport away + .resolves(layoutRectLtwh(0, 0, sizes.width, sizes.height * 2)); win.dispatchEvent(new Event('scroll')); yield macroTask(); const shadowDoc = attachShadowDocSpy.firstCall.returnValue.ampdoc; @@ -249,12 +236,8 @@ describes.realWin( sandbox .stub(viewport, 'getClientRectAsync') .onFirstCall() - .callsFake(() => { - // 1x viewport away - return Promise.resolve( - layoutRectLtwh(0, 0, sizes.width, sizes.height * 2) - ); - }); + // 1x viewport away + .resolves(layoutRectLtwh(0, 0, sizes.width, sizes.height * 2)); win.dispatchEvent(new Event('scroll')); yield macroTask(); @@ -528,6 +511,62 @@ describes.realWin( expect(registerSpy.calledWith(element, config)).to.be.true; }); }); + + describe('manual visibility management', () => { + beforeEach(done => { + element.innerHTML = ` + `; + nextPage.buildCallback().then(done); + }); + + it('defaults to the prerender visibility state for the next document', function*() { + env.fetchMock.get('*', EXAMPLE_PAGE); + + const nextPageService = yield getServicePromiseForDoc( + ampdoc, + 'next-page' + ); + const attachShadowDocSpy = sandbox.spy( + nextPageService.multidocManager_, + 'attachShadowDoc' + ); + + sandbox + .stub(viewport, 'getClientRectAsync') + .onFirstCall() + // 1x viewport away + .resolves(layoutRectLtwh(0, 0, sizes.width, sizes.height * 2)); + + win.dispatchEvent(new Event('scroll')); + yield macroTask(); + + const shadowDoc = attachShadowDocSpy.firstCall.returnValue.ampdoc; + yield shadowDoc.whenReady(); + + expect(shadowDoc.getVisibilityState()).to.equal( + VisibilityState.PRERENDER + ); + }); + }); } ); From d2e21facaab40ce00f80b272895f14153ad2a71c Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Mon, 4 Nov 2019 16:20:13 -0800 Subject: [PATCH 11/16] Fixed types --- .../amp-next-page/0.1/next-page-service.js | 8 +++----- src/runtime.js | 18 +++++++++++++++++- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/extensions/amp-next-page/0.1/next-page-service.js b/extensions/amp-next-page/0.1/next-page-service.js index 36e29ab72ba4..b93696559e1d 100644 --- a/extensions/amp-next-page/0.1/next-page-service.js +++ b/extensions/amp-next-page/0.1/next-page-service.js @@ -175,7 +175,7 @@ export class NextPageService { canonicalUrl ); - // TODO(gharbiw) Establish parity with the shadow doc amp object + // TODO(wassgha): Establish parity with the shadow doc amp object documentRef.amp = { ampdoc: ampDoc, url: win.document.location.href, @@ -325,8 +325,7 @@ export class NextPageService { } this.resources_.mutateElement(container, () => { try { - const amp = this.attachShadowDoc_(shadowRoot, doc); - documentRef.amp = amp; + documentRef.amp = this.attachShadowDoc_(shadowRoot, doc); toggle(dev().assertElement(documentRef.recUnit.el), false); this.documentQueued_ = false; @@ -469,7 +468,7 @@ export class NextPageService { return; } - let documentIndex; + let documentIndex = i; let analyticsEvent = ''; switch (position.relativePos) { @@ -478,7 +477,6 @@ export class NextPageService { analyticsEvent = 'amp-next-page-scroll'; break; case 'bottom': - documentIndex = i; analyticsEvent = 'amp-next-page-scroll-back'; break; default: diff --git a/src/runtime.js b/src/runtime.js index 95e45f286f52..2bfc7a5ac6f9 100644 --- a/src/runtime.js +++ b/src/runtime.js @@ -67,6 +67,22 @@ setReportError(reportErrorForWin.bind(null, self)); /** @const @private {string} */ const TAG = 'runtime'; +/** + * @typedef {{ + * url: string, + * ampdoc: !./service/ampdoc-impl.AmpDocShadow, + * setVisibilityState: (Function|undefined), + * postMessage: (Function|undefined), + * onMessage: (Function|undefined), + * close: (Function|undefined), + * getState: (Function|undefined), + * setState: (Function|undefined), + * toggleRuntime: (Function|undefined), + * resources: !./service/resources-interface.ResourcesInterface + * }} + */ +export let ShadowDoc; + /** * Applies the runtime to a given global scope for a single-doc mode. Multi * frame support is currently incomplete. @@ -420,7 +436,7 @@ export class MultidocManager { * @param {!Object|undefined} params * @param {function(!Object, !ShadowRoot, * !./service/ampdoc-impl.AmpDocShadow):!Promise} builder - * @return {!Object} + * @return {!ShadowDoc} * @private */ attachShadowDoc_(hostElement, url, params, builder) { From d275a103d3f387ad7c70d40ad3917d4e67e08f38 Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Wed, 6 Nov 2019 12:58:39 -0800 Subject: [PATCH 12/16] Requested changes --- .../amp-next-page/0.1/next-page-service.js | 35 +++++++++---------- src/runtime.js | 17 +++++---- 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/extensions/amp-next-page/0.1/next-page-service.js b/extensions/amp-next-page/0.1/next-page-service.js index b93696559e1d..7aa423921365 100644 --- a/extensions/amp-next-page/0.1/next-page-service.js +++ b/extensions/amp-next-page/0.1/next-page-service.js @@ -522,19 +522,16 @@ export class NextPageService { setActiveDocument_(documentIndex) { this.documentRefs_.forEach((docRef, index) => { const {amp} = docRef; - let updatedVisibilityState; // Update the title and history - if (index == documentIndex) { + if (index === documentIndex) { this.win_.document.title = amp.title || ''; this.activeDocumentRef_ = docRef; this.setActiveDocumentInHistory_(docRef); - updatedVisibilityState = VisibilityState.VISIBLE; - } else if (index !== 0) { - updatedVisibilityState = VisibilityState.HIDDEN; - } - // Show the active doc and hide other docs - if (updatedVisibilityState) { - this.setDocumentVisibility_(index, updatedVisibilityState); + // Show the active document + this.setDocumentVisibility_(index, VisibilityState.VISIBLE); + } else { + // Hide other documents + this.setDocumentVisibility_(index, VisibilityState.HIDDEN); } }); @@ -555,17 +552,19 @@ export class NextPageService { } const doc = this.documentRefs_[documentIndex]; + const ampDoc = doc && doc.amp && doc.amp.ampdoc; - if (doc && doc.amp && doc.amp['ampdoc']) { - // Prevent hiding of documents that are being pre-rendered - if ( - !doc.amp.ampdoc.hasBeenVisible() && - visibilityState == VisibilityState.HIDDEN - ) { - return; - } - doc.amp.setVisibilityState(visibilityState); + // Prevent hiding of documents that are not shadow docs + if (!ampDoc) { + return; } + + // Prevent hiding of documents that are being pre-rendered + if (!ampDoc.hasBeenVisible() && visibilityState == VisibilityState.HIDDEN) { + return; + } + + doc.amp.setVisibilityState(visibilityState); } /** diff --git a/src/runtime.js b/src/runtime.js index 2bfc7a5ac6f9..423347602b10 100644 --- a/src/runtime.js +++ b/src/runtime.js @@ -70,14 +70,17 @@ const TAG = 'runtime'; /** * @typedef {{ * url: string, + * title: string, + * canonicalUrl: string, + * head: (Element|undefined), * ampdoc: !./service/ampdoc-impl.AmpDocShadow, - * setVisibilityState: (Function|undefined), - * postMessage: (Function|undefined), - * onMessage: (Function|undefined), - * close: (Function|undefined), - * getState: (Function|undefined), - * setState: (Function|undefined), - * toggleRuntime: (Function|undefined), + * setVisibilityState: (function()|undefined), + * postMessage: (function()|undefined), + * onMessage: (function()|undefined), + * close: (function()|undefined), + * getState: (function()|undefined), + * setState: (function()|undefined), + * toggleRuntime: (function()|undefined), * resources: !./service/resources-interface.ResourcesInterface * }} */ From e11dd926bc8731fd4640c0e959927bcb5a0db4f5 Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Wed, 6 Nov 2019 14:45:19 -0800 Subject: [PATCH 13/16] Fixed types --- .../amp-next-page/0.1/next-page-service.js | 18 ++++++++++++------ src/runtime.js | 14 +++++++------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/extensions/amp-next-page/0.1/next-page-service.js b/extensions/amp-next-page/0.1/next-page-service.js index 7aa423921365..c15ff1898ee4 100644 --- a/extensions/amp-next-page/0.1/next-page-service.js +++ b/extensions/amp-next-page/0.1/next-page-service.js @@ -41,7 +41,7 @@ const TAG = 'amp-next-page'; /** * @typedef {{ * ampUrl: string, - * amp: ?Object, + * amp: (?../../../src/runtime.ShadowDoc | undefined), * recUnit: {el: ?Element, isObserving: boolean}, * cancelled: boolean * }} @@ -175,13 +175,15 @@ export class NextPageService { canonicalUrl ); - // TODO(wassgha): Establish parity with the shadow doc amp object - documentRef.amp = { + // TODO(wassgha): Untype as ShadowDoc and tighten the ShadowDoc type spec + /** @type {!../../../src/runtime.ShadowDoc} */ + const amp = { ampdoc: ampDoc, url: win.document.location.href, title: win.document.title, canonicalUrl, }; + documentRef.amp = amp; this.documentRefs_.push(documentRef); this.activeDocumentRef_ = this.documentRefs_[0]; @@ -208,7 +210,7 @@ export class NextPageService { * Attach a ShadowDoc using the given document. * @param {!Element} shadowRoot Root element to attach the shadow document to. * @param {!Document} doc Document to attach. - * @return {?Object} Return value of {@link MultidocManager#attachShadowDoc} + * @return {?../../../src/runtime.ShadowDoc} Return value of {@link MultidocManager#attachShadowDoc} */ attachShadowDoc_(shadowRoot, doc) { if (this.hideSelector_) { @@ -226,12 +228,16 @@ export class NextPageService { removeElement(item); } + /** @type {!../../../src/runtime.ShadowDoc} */ const amp = this.multidocManager_.attachShadowDoc(shadowRoot, doc, '', { visibilityState: VisibilityState.PRERENDER, }); - installStylesForDoc(amp.ampdoc, CSS, null, false, TAG); + const ampdoc = /** @type {!../../../src/service/ampdoc-impl.AmpDoc} */ (dev().assert( + amp.ampdoc + )); + installStylesForDoc(ampdoc, CSS, null, false, TAG); - const body = amp.ampdoc.getBody(); + const body = ampdoc.getBody(); body.classList.add('i-amphtml-next-page-document'); return amp; diff --git a/src/runtime.js b/src/runtime.js index 423347602b10..bc6bff6d8f83 100644 --- a/src/runtime.js +++ b/src/runtime.js @@ -69,19 +69,19 @@ const TAG = 'runtime'; /** * @typedef {{ - * url: string, - * title: string, - * canonicalUrl: string, + * url: (string|undefined), + * title: (string|undefined), + * canonicalUrl: (string|undefined), * head: (Element|undefined), - * ampdoc: !./service/ampdoc-impl.AmpDocShadow, - * setVisibilityState: (function()|undefined), + * ampdoc: (!./service/ampdoc-impl.AmpDoc | undefined), + * setVisibilityState: (function(!VisibilityState)|undefined), * postMessage: (function()|undefined), * onMessage: (function()|undefined), * close: (function()|undefined), * getState: (function()|undefined), * setState: (function()|undefined), * toggleRuntime: (function()|undefined), - * resources: !./service/resources-interface.ResourcesInterface + * resources: (!./service/resources-interface.ResourcesInterface | undefined) * }} */ export let ShadowDoc; @@ -598,7 +598,7 @@ export class MultidocManager { * @param {!Document} doc * @param {string} url * @param {!Object=} opt_initParams - * @return {!Object} + * @return {!ShadowDoc} */ attachShadowDoc(hostElement, doc, url, opt_initParams) { dev().fine(TAG, 'Attach shadow doc:', doc); From bd009a9ee7386c65883d493d30e4943a3582788c Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Wed, 6 Nov 2019 15:19:34 -0800 Subject: [PATCH 14/16] Removed unnecessary cast --- extensions/amp-next-page/0.1/next-page-service.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/extensions/amp-next-page/0.1/next-page-service.js b/extensions/amp-next-page/0.1/next-page-service.js index c15ff1898ee4..a2a1ab655595 100644 --- a/extensions/amp-next-page/0.1/next-page-service.js +++ b/extensions/amp-next-page/0.1/next-page-service.js @@ -232,9 +232,7 @@ export class NextPageService { const amp = this.multidocManager_.attachShadowDoc(shadowRoot, doc, '', { visibilityState: VisibilityState.PRERENDER, }); - const ampdoc = /** @type {!../../../src/service/ampdoc-impl.AmpDoc} */ (dev().assert( - amp.ampdoc - )); + const ampdoc = dev().assert(amp.ampdoc); installStylesForDoc(ampdoc, CSS, null, false, TAG); const body = ampdoc.getBody(); From 9698d58c2bdd6d2c2e5c90de2a30168eab4fe081 Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Thu, 7 Nov 2019 16:16:56 -0800 Subject: [PATCH 15/16] Switched to using refs instead of indices --- .../amp-next-page/0.1/next-page-service.js | 40 +++++++++---------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/extensions/amp-next-page/0.1/next-page-service.js b/extensions/amp-next-page/0.1/next-page-service.js index a2a1ab655595..86354f9487b5 100644 --- a/extensions/amp-next-page/0.1/next-page-service.js +++ b/extensions/amp-next-page/0.1/next-page-service.js @@ -19,7 +19,7 @@ import {MultidocManager} from '../../../src/runtime'; import {PositionObserverFidelity} from '../../../src/service/position-observer/position-observer-worker'; import {Services} from '../../../src/services'; import {VisibilityState} from '../../../src/visibility-state'; -import {dev, user, userAssert} from '../../../src/log'; +import {dev, devAssert, user, userAssert} from '../../../src/log'; import {dict} from '../../../src/utils/object'; import {getAmpdoc} from '../../../src/service'; import {installPositionObserverServiceForDoc} from '../../../src/service/position-observer/position-observer-impl'; @@ -232,7 +232,7 @@ export class NextPageService { const amp = this.multidocManager_.attachShadowDoc(shadowRoot, doc, '', { visibilityState: VisibilityState.PRERENDER, }); - const ampdoc = dev().assert(amp.ampdoc); + const ampdoc = devAssert(amp.ampdoc); installStylesForDoc(ampdoc, CSS, null, false, TAG); const body = ampdoc.getBody(); @@ -472,12 +472,12 @@ export class NextPageService { return; } - let documentIndex = i; + let ref = this.documentRefs_[i]; let analyticsEvent = ''; switch (position.relativePos) { case 'top': - documentIndex = i + 1; + ref = this.documentRefs_[i + 1]; analyticsEvent = 'amp-next-page-scroll'; break; case 'bottom': @@ -487,16 +487,13 @@ export class NextPageService { break; } - if ( - this.documentRefs_[documentIndex] && - this.documentRefs_[documentIndex].amp - ) { + if (ref && ref.amp) { this.triggerAnalyticsEvent_( analyticsEvent, - this.documentRefs_[documentIndex].ampUrl, + ref.ampUrl, this.activeDocumentRef_.ampUrl ); - this.setActiveDocument_(documentIndex); + this.setActiveDocument_(ref); } } @@ -520,22 +517,22 @@ export class NextPageService { /** * Sets the specified document as active, updating the document title and URL. * - * @param {number} documentIndex Index of the document to be activated + * @param {DocumentRef} ref Reference to the document to be activated * @private */ - setActiveDocument_(documentIndex) { - this.documentRefs_.forEach((docRef, index) => { + setActiveDocument_(ref) { + this.documentRefs_.forEach(docRef => { const {amp} = docRef; // Update the title and history - if (index === documentIndex) { + if (docRef === ref) { this.win_.document.title = amp.title || ''; this.activeDocumentRef_ = docRef; this.setActiveDocumentInHistory_(docRef); // Show the active document - this.setDocumentVisibility_(index, VisibilityState.VISIBLE); + this.setDocumentVisibility_(docRef, VisibilityState.VISIBLE); } else { // Hide other documents - this.setDocumentVisibility_(index, VisibilityState.HIDDEN); + this.setDocumentVisibility_(docRef, VisibilityState.HIDDEN); } }); @@ -545,18 +542,17 @@ export class NextPageService { /** * Manually overrides the document's visible state to the given state * - * @param {number} documentIndex Index of the document to change + * @param {DocumentRef} ref Reference to the document to change * @param {!../../../src/visibility-state.VisibilityState} visibilityState * @private */ - setDocumentVisibility_(documentIndex, visibilityState) { + setDocumentVisibility_(ref, visibilityState) { // Prevent updating visibility of the host document - if (documentIndex === 0) { + if (ref === this.documentRefs_[0]) { return; } - const doc = this.documentRefs_[documentIndex]; - const ampDoc = doc && doc.amp && doc.amp.ampdoc; + const ampDoc = ref && ref.amp && ref.amp.ampdoc; // Prevent hiding of documents that are not shadow docs if (!ampDoc) { @@ -568,7 +564,7 @@ export class NextPageService { return; } - doc.amp.setVisibilityState(visibilityState); + ref.amp.setVisibilityState(visibilityState); } /** From 92033039466ffdf800767c757bed70cc6ef2667f Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Thu, 7 Nov 2019 16:32:59 -0800 Subject: [PATCH 16/16] Fixed non-nullable type --- extensions/amp-next-page/0.1/next-page-service.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/extensions/amp-next-page/0.1/next-page-service.js b/extensions/amp-next-page/0.1/next-page-service.js index 86354f9487b5..5bf78a52248e 100644 --- a/extensions/amp-next-page/0.1/next-page-service.js +++ b/extensions/amp-next-page/0.1/next-page-service.js @@ -517,7 +517,7 @@ export class NextPageService { /** * Sets the specified document as active, updating the document title and URL. * - * @param {DocumentRef} ref Reference to the document to be activated + * @param {!DocumentRef} ref Reference to the document to be activated * @private */ setActiveDocument_(ref) { @@ -542,7 +542,7 @@ export class NextPageService { /** * Manually overrides the document's visible state to the given state * - * @param {DocumentRef} ref Reference to the document to change + * @param {!DocumentRef} ref Reference to the document to change * @param {!../../../src/visibility-state.VisibilityState} visibilityState * @private */ @@ -552,7 +552,7 @@ export class NextPageService { return; } - const ampDoc = ref && ref.amp && ref.amp.ampdoc; + const ampDoc = ref.amp && ref.amp.ampdoc; // Prevent hiding of documents that are not shadow docs if (!ampDoc) {