From b90234e190e69211af7fd29d86aeb591a7e303f5 Mon Sep 17 00:00:00 2001 From: ryjust Date: Wed, 1 Aug 2018 10:15:34 -0400 Subject: [PATCH 1/2] Delay adding #ampshare parameter until the document is visible. --- src/service/viewer-impl.js | 4 +++- test/functional/test-viewer.js | 42 ++++++++++++++++++++++------------ 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/src/service/viewer-impl.js b/src/service/viewer-impl.js index 4692250c5d86..04490cf2cb56 100644 --- a/src/service/viewer-impl.js +++ b/src/service/viewer-impl.js @@ -466,7 +466,9 @@ export class Viewer { // This fragment may get cleared by impression tracking. If so, it will be // restored afterward. - this.maybeUpdateFragmentForCct(); + this.whenFirstVisiblePromise_.then(() => { + this.maybeUpdateFragmentForCct(); + }); } /** diff --git a/test/functional/test-viewer.js b/test/functional/test-viewer.js index ba7790a70a4f..5779d6ff0d41 100644 --- a/test/functional/test-viewer.js +++ b/test/functional/test-viewer.js @@ -168,8 +168,10 @@ describe('Viewer', () => { windowApi.location.search = '?amp_gsa=1&_js_v=a0'; const viewer = new Viewer(ampdoc); expect(viewer.isCctEmbedded()).to.be.true; - expect(windowApi.history.replaceState).to.be.calledWith({}, '', - '#ampshare=http%3A%2F%2Fwww.example.com%2F'); + return Promise.resolve().then(() => { + expect(windowApi.history.replaceState).to.be.calledWith({}, '', + '#ampshare=http%3A%2F%2Fwww.example.com%2F'); + }); }); it('should merge fragments within custom tab', () => { @@ -180,8 +182,10 @@ describe('Viewer', () => { const viewer = new Viewer(ampdoc); expect(viewer.getParam('test')).to.equal('1'); expect(viewer.isCctEmbedded()).to.be.true; - expect(windowApi.history.replaceState).to.be.calledWith({}, '', - '#test=1&share=http%3A%2F%2Fwww.example.com%2F'); + return Promise.resolve(() => { + expect(windowApi.history.replaceState).to.be.calledWith({}, '', + '#test=1&share=http%3A%2F%2Fwww.example.com%2F'); + }); }); it('should not duplicate ampshare when merging', () => { @@ -192,8 +196,10 @@ describe('Viewer', () => { const viewer = new Viewer(ampdoc); expect(viewer.getParam('test')).to.equal('1'); expect(viewer.isCctEmbedded()).to.be.true; - expect(windowApi.history.replaceState).to.be.calledWith({}, '', - '#test=1&share=http%3A%2F%2Fwww.example.com%2F'); + return Promise.resolve(() => { + expect(windowApi.history.replaceState).to.be.calledWith({}, '', + '#test=1&share=http%3A%2F%2Fwww.example.com%2F'); + }); }); it('should remove multiple ampshares when merging', () => { @@ -206,8 +212,10 @@ describe('Viewer', () => { const viewer = new Viewer(ampdoc); expect(viewer.getParam('test')).to.equal('1'); expect(viewer.isCctEmbedded()).to.be.true; - expect(windowApi.history.replaceState).to.be.calledWith({}, '', - '#test=1&share=http%3A%2F%2Fwww.example.com%2F'); + return Promise.resolve(() => { + expect(windowApi.history.replaceState).to.be.calledWith({}, '', + '#test=1&share=http%3A%2F%2Fwww.example.com%2F'); + }); }); it('should remove extra ampshare even when it\'s first', () => { @@ -218,8 +226,10 @@ describe('Viewer', () => { const viewer = new Viewer(ampdoc); expect(viewer.getParam('test')).to.equal('1'); expect(viewer.isCctEmbedded()).to.be.true; - expect(windowApi.history.replaceState).to.be.calledWith({}, '', - '#ampshare=http%3A%2F%2Fwww.example.com%2F&test=1'); + return Promise.resolve(() => { + expect(windowApi.history.replaceState).to.be.calledWith({}, '', + '#ampshare=http%3A%2F%2Fwww.example.com%2F&test=1'); + }); }); it('should remove extra ampshare even when it\'s sandwiched', () => { @@ -233,8 +243,10 @@ describe('Viewer', () => { expect(viewer.getParam('test')).to.equal('1'); expect(viewer.getParam('note')).to.equal('ok'); expect(viewer.isCctEmbedded()).to.be.true; - expect(windowApi.history.replaceState).to.be.calledWith({}, '', - '#note=ok&share=http%3A%2F%2Fwww.example.com%2F&test=1'); + return Promise.resolve(() => { + expect(windowApi.history.replaceState).to.be.calledWith({}, '', + '#note=ok&share=http%3A%2F%2Fwww.example.com%2F&test=1'); + }); }); it('should clear fragment when click param is present', () => { @@ -257,8 +269,10 @@ describe('Viewer', () => { expect(windowApi.history.replaceState).to.be.calledWith({}, '', 'http://www.example.com'); expect(viewer.getParam('click')).to.equal('abc'); - expect(windowApi.history.replaceState).to.be.calledWith({}, '', - '#ampshare=http%3A%2F%2Fwww.example.com%2F'); + return Promise.resolve(() => { + expect(windowApi.history.replaceState).to.be.calledWith({}, '', + '#ampshare=http%3A%2F%2Fwww.example.com%2F'); + }); }); it('should configure visibilityState visible by default', () => { From 6e4286fc87411456d72f2a4dc4235edf99679e84 Mon Sep 17 00:00:00 2001 From: ryjust Date: Wed, 1 Aug 2018 12:42:22 -0400 Subject: [PATCH 2/2] Use generator functions in tests rather than returning a promise --- src/service/viewer-impl.js | 2 +- test/functional/test-viewer.js | 63 +++++++++++++++------------------- 2 files changed, 29 insertions(+), 36 deletions(-) diff --git a/src/service/viewer-impl.js b/src/service/viewer-impl.js index 04490cf2cb56..e0495931a9b6 100644 --- a/src/service/viewer-impl.js +++ b/src/service/viewer-impl.js @@ -466,7 +466,7 @@ export class Viewer { // This fragment may get cleared by impression tracking. If so, it will be // restored afterward. - this.whenFirstVisiblePromise_.then(() => { + this.whenFirstVisible().then(() => { this.maybeUpdateFragmentForCct(); }); } diff --git a/test/functional/test-viewer.js b/test/functional/test-viewer.js index 5779d6ff0d41..27a04ecdfdc4 100644 --- a/test/functional/test-viewer.js +++ b/test/functional/test-viewer.js @@ -161,20 +161,19 @@ describe('Viewer', () => { expect(viewer.getParam('test')).to.equal('1'); }); - it('should set ampshare fragment within custom tab', () => { + it('should set ampshare fragment within custom tab', function*() { windowApi.parent = windowApi; windowApi.location.href = 'http://www.example.com/'; windowApi.location.hash = ''; windowApi.location.search = '?amp_gsa=1&_js_v=a0'; const viewer = new Viewer(ampdoc); expect(viewer.isCctEmbedded()).to.be.true; - return Promise.resolve().then(() => { - expect(windowApi.history.replaceState).to.be.calledWith({}, '', - '#ampshare=http%3A%2F%2Fwww.example.com%2F'); - }); + yield viewer.whenFirstVisible(); + expect(windowApi.history.replaceState).to.be.calledWith({}, '', + '#ampshare=http%3A%2F%2Fwww.example.com%2F'); }); - it('should merge fragments within custom tab', () => { + it('should merge fragments within custom tab', function*() { windowApi.parent = windowApi; windowApi.location.href = 'http://www.example.com/#test=1'; windowApi.location.hash = '#test=1'; @@ -182,13 +181,12 @@ describe('Viewer', () => { const viewer = new Viewer(ampdoc); expect(viewer.getParam('test')).to.equal('1'); expect(viewer.isCctEmbedded()).to.be.true; - return Promise.resolve(() => { - expect(windowApi.history.replaceState).to.be.calledWith({}, '', - '#test=1&share=http%3A%2F%2Fwww.example.com%2F'); - }); + yield viewer.whenFirstVisible(); + expect(windowApi.history.replaceState).to.be.calledWith({}, '', + '#test=1&share=http%3A%2F%2Fwww.example.com%2F'); }); - it('should not duplicate ampshare when merging', () => { + it('should not duplicate ampshare when merging', function*() { windowApi.parent = windowApi; windowApi.location.href = 'http://www.example.com/#test=1&share=old'; windowApi.location.hash = '#test=1&share=old'; @@ -196,13 +194,12 @@ describe('Viewer', () => { const viewer = new Viewer(ampdoc); expect(viewer.getParam('test')).to.equal('1'); expect(viewer.isCctEmbedded()).to.be.true; - return Promise.resolve(() => { - expect(windowApi.history.replaceState).to.be.calledWith({}, '', - '#test=1&share=http%3A%2F%2Fwww.example.com%2F'); - }); + yield viewer.whenFirstVisible(); + expect(windowApi.history.replaceState).to.be.calledWith({}, '', + '#test=1&share=http%3A%2F%2Fwww.example.com%2F'); }); - it('should remove multiple ampshares when merging', () => { + it('should remove multiple ampshares when merging', function*() { windowApi.parent = windowApi; windowApi.location.href = 'http://www.example.com/#test=1&share=a&share=b&share=c'; @@ -212,13 +209,12 @@ describe('Viewer', () => { const viewer = new Viewer(ampdoc); expect(viewer.getParam('test')).to.equal('1'); expect(viewer.isCctEmbedded()).to.be.true; - return Promise.resolve(() => { - expect(windowApi.history.replaceState).to.be.calledWith({}, '', - '#test=1&share=http%3A%2F%2Fwww.example.com%2F'); - }); + yield viewer.whenFirstVisible(); + expect(windowApi.history.replaceState).to.be.calledWith({}, '', + '#test=1&share=http%3A%2F%2Fwww.example.com%2F'); }); - it('should remove extra ampshare even when it\'s first', () => { + it('should remove extra ampshare even when it\'s first', function*() { windowApi.parent = windowApi; windowApi.location.href = 'http://www.example.com/#ampshare=old&test=1'; windowApi.location.hash = '#ampshare=old&test=1'; @@ -226,13 +222,12 @@ describe('Viewer', () => { const viewer = new Viewer(ampdoc); expect(viewer.getParam('test')).to.equal('1'); expect(viewer.isCctEmbedded()).to.be.true; - return Promise.resolve(() => { - expect(windowApi.history.replaceState).to.be.calledWith({}, '', - '#ampshare=http%3A%2F%2Fwww.example.com%2F&test=1'); - }); + yield viewer.whenFirstVisible(); + expect(windowApi.history.replaceState).to.be.calledWith({}, '', + '#ampshare=http%3A%2F%2Fwww.example.com%2F&test=1'); }); - it('should remove extra ampshare even when it\'s sandwiched', () => { + it('should remove extra ampshare even when it\'s sandwiched', function*() { windowApi.parent = windowApi; windowApi.location.href = 'http://www.example.com/#note=ok&share=old&test=1'; @@ -243,10 +238,9 @@ describe('Viewer', () => { expect(viewer.getParam('test')).to.equal('1'); expect(viewer.getParam('note')).to.equal('ok'); expect(viewer.isCctEmbedded()).to.be.true; - return Promise.resolve(() => { - expect(windowApi.history.replaceState).to.be.calledWith({}, '', - '#note=ok&share=http%3A%2F%2Fwww.example.com%2F&test=1'); - }); + yield viewer.whenFirstVisible(); + expect(windowApi.history.replaceState).to.be.calledWith({}, '', + '#note=ok&share=http%3A%2F%2Fwww.example.com%2F&test=1'); }); it('should clear fragment when click param is present', () => { @@ -260,7 +254,7 @@ describe('Viewer', () => { expect(viewer.getParam('click')).to.equal('abc'); }); - it('should restore fragment within custom tab with click param', () => { + it('should restore fragment within custom tab with click param', function*() { windowApi.parent = windowApi; windowApi.location.href = 'http://www.example.com#click=abc'; windowApi.location.hash = '#click=abc'; @@ -269,10 +263,9 @@ describe('Viewer', () => { expect(windowApi.history.replaceState).to.be.calledWith({}, '', 'http://www.example.com'); expect(viewer.getParam('click')).to.equal('abc'); - return Promise.resolve(() => { - expect(windowApi.history.replaceState).to.be.calledWith({}, '', - '#ampshare=http%3A%2F%2Fwww.example.com%2F'); - }); + yield viewer.whenFirstVisible(); + expect(windowApi.history.replaceState).to.be.calledWith({}, '', + '#ampshare=http%3A%2F%2Fwww.example.com%2F'); }); it('should configure visibilityState visible by default', () => {