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

Setting amp-analytics parameters with ${ampState(___)} is broken in PWA #20853

Closed
mattwomple opened this issue Feb 14, 2019 · 3 comments
Closed

Comments

@mattwomple
Copy link
Member

What's the issue?

When an AMP page utilizing amp-analytics is loaded in a PWA, setting an analytics parameter value via ${ampState(___)} fails. The console reports No ampdoc found for [object HTMLHtmlElement].

How do we reproduce the issue?

I have created a minimal example that demonstrates the issue. When you click the "Send Analytics" button on the AMP page, favnum in extraUrlParams is set correctly (see the collect beacon in network requests). When you click the same button in the PWA, favnum is blank and the console reports No ampdoc found for [object HTMLHtmlElement].

What browsers are affected?

The following browsers have been tested and both exhibit the same behavior:

  • Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3626.96 Safari/537.36
  • Mozilla/5.0 (iPhone; CPU iPhone OS 12_0 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0 Mobile/15E148 Safari/604.1

Which AMP version is affected?

1902081532110

@mattwomple mattwomple changed the title amp-analytics with ${ampState(___)} is broken in PWA Setting amp-analytics with ${ampState(___)} is broken in PWA Feb 14, 2019
@mattwomple mattwomple changed the title Setting amp-analytics with ${ampState(___)} is broken in PWA Setting amp-analytics parameters with ${ampState(___)} is broken in PWA Feb 14, 2019
@torch2424
Copy link
Contributor

Triaging to @zhouyx , feel free to re-assign 😄

@jerimiahwelch
Copy link

Any update on this? This is affecting PWA enhanced ecommerce reporting and A/B testing reporting in Google Analytics for us.

@zhouyx
Copy link
Contributor

zhouyx commented Feb 21, 2019

Thank you for filing the issue and providing the examples.

Seem like runtime fails to find the ampdoc for win.document.

this.setAsync('AMP_STATE', key => {
return Services.bindForDocOrNull(this.ampdoc).then(bind => {
if (!bind) {
return '';
}

@choumx could you please take a look. Thank you
FYI: we have a manual test page https://github.com/ampproject/amphtml/blob/master/test/manual/amp-analytics/analytics-shadow.html for easier local testing. I copied @mattwomple's example https://android.cmphys.com/temp/analytics-setstate-amp.html to test/manual/amp-analytics/analytics-shadow-root-1.html and reproduced locally.

@zhouyx zhouyx assigned dreamofabear and unassigned zhouyx Feb 21, 2019
cvializ pushed a commit that referenced this issue Mar 6, 2019
…21283)

Use ampdoc.getRootNode() to identify the relevant document/shadow node
for Services.bindForDocOrNull() when making URL replacements in a PWA.

Fixes #20853
noranazmy pushed a commit to noranazmy/amphtml that referenced this issue Mar 22, 2019
…t#20853) (ampproject#21283)

Use ampdoc.getRootNode() to identify the relevant document/shadow node
for Services.bindForDocOrNull() when making URL replacements in a PWA.

Fixes ampproject#20853
bramanudom pushed a commit to bramanudom/amphtml that referenced this issue Mar 22, 2019
…t#20853) (ampproject#21283)

Use ampdoc.getRootNode() to identify the relevant document/shadow node
for Services.bindForDocOrNull() when making URL replacements in a PWA.

Fixes ampproject#20853
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants