Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Delay adding the ampshare fragment parameter until after the document is first visible #17237

Merged
merged 4 commits into from
Aug 3, 2018

Conversation

ryjust
Copy link
Contributor

@ryjust ryjust commented Aug 1, 2018

When AMP CSI is enabled for CCT embedded documents, the hash fragment is updated by Chrome on click to include the click time. This process removes the ampshare parameter. The page becomes visible only after the fragment is updated so we need to delay adding the ampshare parameter until then.

@@ -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&ampshare=http%3A%2F%2Fwww.example.com%2F');
return Promise.resolve(() => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be viewer.whenFirstVisible(). If you change the test function from a closure to a generator function you can use yield.

it('should merge fragments within custom tab', function*() {
  ...
  yield viewer.whenFirstVisible();
  expect(windowApi...);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion! Much cleaner. Fixed!

@dreamofabear
Copy link

Thanks for contributing!

@dreamofabear
Copy link

Unit tests are unexpectedly failing on this test:

DESCRIBE => amp-a4a
  IT => should set height/width on iframe matching header value
    ✗ Error: Uncaught TypeError: win.getComputedStyle is not a function (node_modules/sinon/pkg/sinon.js:2538)

I'm getting the same error on #17128. The amp-a4a test probably implicitly depends on order of test execution. 😢 I'll dig into this and merge when ready.

/cc @rsimha

@rsimha
Copy link
Contributor

rsimha commented Aug 1, 2018

The version of sinon we use is pretty old. I'm working on migrating to the latest version, which fixes some bugs, and has a more intuitive API and clearer error messages.

Edit: This is now done.

@dreamofabear
Copy link

@ryjust I fixed the issue in master. Can you merge latest and push?

@ryjust
Copy link
Contributor Author

ryjust commented Aug 3, 2018

Everything is passing now. Thanks!

@dreamofabear dreamofabear merged commit 1143ef0 into ampproject:master Aug 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants