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

Issue using amp-analytics with Shadow DOM #20400

Closed
enzo-giancola opened this issue Jan 17, 2019 · 19 comments · Fixed by #27035
Closed

Issue using amp-analytics with Shadow DOM #20400

enzo-giancola opened this issue Jan 17, 2019 · 19 comments · Fixed by #27035

Comments

@enzo-giancola
Copy link

Hi,

we are facing errors related to amp-analytics
raising when AMP code is imported through a shadow dom
based mechanism (in Polymer).

The problem we noticed:

amp-analytics events are fired within AMP content;
when AMP code is imported through a shadow dom based mechanism (in Polymer) events aren't fired;

How to reproduce:

<amp-analytics><script type="application/json">
        {
            "requests": {                    
                "host": "https://www.facebook.com",
                "base": "${host}/tr?noscript=1",
                "ViewPaywall" : "${base}&ev=ViewPaywall&id=${pixelId}&cd[meter_count]=0&cd[serviceId]=${serviceId}&cd[variant]=${variant}"
            },
            "vars": {                    
                "pixelId":"SOMEPIXELID",
                "variant": "VARIANT(swg-button-experiment)"
            },
            "triggers": {
                "accessAuthorizationFailed": {
                    "on": "subscriptions-access-denied",
                    "request": "ViewPaywall"
                }
            }
        }
    </script></amp-analytics>

Same problem with amp-analytics code like this:

<amp-analytics><script type="application/json"> {
            "requests":{
                "host": "https://pubads.g.doubleclick.net",
                "base": "${host}/activity",
                "ViewPaywall" : "${base};xsp=${xsp};ord=1;num=${num}?" 
            },
            "vars":{
                "xsp":"SOMEXSPCODE",
                "num":"RANDOM" 
            },
            "triggers": {
                "accessAuthorizationFailed": {
                    "on": "subscriptions-access-denied",
                    "request": "ViewPaywall"
                }
            },
            "transport": {
               "beacon": false,
               "xhrpost": false,
               "image": true
            }
        }
       	</script></amp-analytics>

Could you please check?

Best Regards,
Enzo

@jpettitt
Copy link
Contributor

If I turn on logging (append #log=3 to the url above) I see the event being fired by amp-subscriptions

image

On that basis I suspect the bug is in amp-analytics so I'm going to assign it to that team.

@lannka
Copy link
Contributor

lannka commented Jan 17, 2019

@jpettitt is this blocking anything at the moment? We can prioritize it in the next sprint (starting next week).

@jpettitt
Copy link
Contributor

Yes it's blocking a publisher launch so next sprint would be good.

@lannka lannka assigned zhouyx and unassigned lannka Jan 18, 2019
@lannka
Copy link
Contributor

lannka commented Jan 18, 2019

planned for next sprint. @zhouyx

@zhouyx
Copy link
Contributor

zhouyx commented Jan 26, 2019

Hello @enzo-giancola, thank you for filing the issue. I looked at the given example page. What I found is that the <amp-subscription> and the <amp-analytics> are in two different shadow root. In this case, they don't share the same the analytics service, so the custom trigger doesn't propagate to <amp-analytics> in another shadow root.

@jpettitt
Copy link
Contributor

That makes sense, it's not reasonable to expect a top level analytics config to know all the possible triggers a subsequent shadow doc might invoke so the amp-analytics tag would need to be scoped to and reside in the same shadow root that the events are originating from. Thank for debugging this. @enzo-giancola does this resolve your issue?

@enzo-giancola
Copy link
Author

Hi, let me explain better.

We have the tag amp-experiment scoped to the same shadow root as the amp-analytics one
as you can see on the attached screen capture.

Because of we was unable to resolve VARIANTS runtime variable within the PWA
we have configured an iframe with a minimal AMP content, in order to resolve runtime variable VARIANTS
duplicating the amp-experiment configuration.
This allows us to set a cookie we are using to customize the look&feel
based on current treatement on the experiment.

So, the original AMP content has amp-experiment and amp-analytics tags.
They are still available on the same root when loaded into a shadow-dom.

Could you please check?

Best Regards

amp-experiment_amp-analytics

@zhouyx
Copy link
Contributor

zhouyx commented Jan 29, 2019

@enzo-giancola I am a bit confused, Could you please clarify what the issue is?

Based on #20400 (comment), the analytics trigger is "subscriptions-access-denied", and that is a signal from the <amp-subscription> component. So the <amp-analytics> must be placed in the same shadow root with <amp-subcription>, not <amp-experiment>.

If I understand it right, you want to collect the subscripts-access-denied event and the VARIANTS variable. In that case, could you place have two seperate <amp-analytics> in the two different shadow roots. Thank you

@zhouyx
Copy link
Contributor

zhouyx commented Feb 7, 2019

@enzo-giancola I'm going to close the issue as the behavior is expected. Please feel free to reopen and let me know if there're more questions. Thank you.

@zhouyx zhouyx closed this as completed Feb 7, 2019
@enzo-giancola
Copy link
Author

@zhouyx, as attached a screen related to the following URL:

https://rep.repubblica.it/pwa/generale/2019/02/07/news/francia_macron_eliseo_gilet_gialli_m5s_di_maio_conte_mattarella_salvini-218582355/

where as you can see <amp-analytics> is placed in the same shadow root with <amp-subcription>.

Is this correct?

Thank you

ampproject-amphtml_20400

@zhouyx
Copy link
Contributor

zhouyx commented Feb 8, 2019

@enzo-giancola Thank you for the screenshot. So I took a look at the page again. Interesting I found two amp-subscriptions in different shadow roots. And one of them is embedded in nested shadow root.

first one:
image

The one @enzo-giancola referred to, note that it is in a nested shadow root
image

@jpettitt @dvoytenko How do the two amp-subscriptions work together?

@zhouyx
Copy link
Contributor

zhouyx commented Feb 8, 2019

And I found the exact <amp-analytics> element that is listening to 'subscriptions-access-denied' event. They seem to be placed under subscription guarded section properly. Thoughts?

image

@enzo-giancola
Copy link
Author

Hi @zhouyx, we are using the amp-subscriptions in the first shadow root
to manage the upper right corner login menu.

@zhouyx
Copy link
Contributor

zhouyx commented Feb 12, 2019

@jpettitt @dvoytenko I am not familiar with the amp-subscription usage. But analytics custom event trigger works well when I tested with this example where there're <amp-carousel> and <amp-analytics> in one shadow doc.

Do you see any thing from the screenshots? Thank you.

@jpettitt
Copy link
Contributor

I don't see anything obvious. If you load an AMP page with #log=3 all am subscriptions analytics events will be logged to the console (even if there is nothing listening for them)

@enzo-giancola
Copy link
Author

Hi all, is there any update on this?
We are still facing the same issue. Thank you.

@prateekbh
Copy link
Member

Hi @enzo-giancola, we recently worked with re.republica highlighting the problems around multiple shadow root in #21902.

I believed it was changed may be to use a single shadow root. Does that effect this issue of yours or are you still using multiple shadow roots?

@gilbertococchi
Copy link

I believe Repubblica is still using two Shadow Roots as they need to use it for the outer Login Subscription module within the page.

They did try using v0 and amp-subscription to remove one ShadowRoot but unfortunately it seems like Bento AMP + Shadow AMP approach is not compatible.

Do you know if this could be solved?

@gilbertococchi
Copy link

Hi all, any Bento AMP approach that will be available for components such as amp-subscription?

Rep Repubblica is measuring a really high TTI especially because of the late triggering of the amp-subscription module via shadow-amp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment