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

AMP Analytics multi-selector on amp-form-submit #28348

Closed
stephengardner opened this issue May 12, 2020 · 18 comments
Closed

AMP Analytics multi-selector on amp-form-submit #28348

stephengardner opened this issue May 12, 2020 · 18 comments

Comments

@stephengardner
Copy link

stephengardner commented May 12, 2020

The documentation provided (https://amp.dev/documentation/components/amp-form/?format=websites) does a good job of describing how to track form submissions for POST request forms using form-submit and form-submit-success along with amp-analytics

Unfortunately, while testing, we've found this doesn't work on GET requests. GET request forms are used frequently to do things like perform Search submissions on Shopify by redirecting to "/search?q=...".

If a user presses Enter, submitting the GET request form, it seems like nothing can be tracked. We can track every input change event from the form input, or track specific clicks on the actual submit button (which is rarely clicked), but neither of these solves the issue. We could also perform a POST and a redirect, but this causes the overhead of an actual endpoint needing to be used, and is far less than ideal.

What is the recommended way to track GET request form submissions?

@stephengardner
Copy link
Author

We're finding that the issue we were facing stemmed from the fact that an amp-form-submit "selector" does not operate similarly to a normal amp-analytics selector like click. The selector instead will target only the FIRST instance of a form. This causes some headache when attempting to track forms. Commented on here: #11380 but doesn't have a dedicated issue for the bug/feature request.

@micajuine-ho micajuine-ho changed the title AMP Analytics - tracking submission of GET (non-XHR) forms is unavailable? AMP Analytics multi-selector on amp-form-submit May 21, 2020
@micajuine-ho
Copy link
Contributor

micajuine-ho commented May 21, 2020

To recap from the other thread, we need to expand the amp-form-submit trigger's selector capabilities to capture all form-submit elements. Currently AMP pages will only capture the first element with the given selector.

Quick thoughts: we could apply the same changes that we did with the visibility trigger (allow for selectors within an array to capture multiple/all elements) or we could think about adding special new selector for this kind of use case. I don't think we should just make all selectors querySelectorAll, because that could cause current users to get an influx of (unidentified) pings.

I will look into the implementation, and see if any solution pops out to me.

@zhouyx
Copy link
Contributor

zhouyx commented May 21, 2020

To continue the discussion in #11380.

I thought more about this yesterday. I agreed with @stephengardner point that the current set up is not very efficient when one wants to group triggers together. I also realized the concept selector doesn't fit very well here.

When used in visibility trigger, we need to wait for all the to-be-tracked element to have been selected to call IntersectionObserver.observe(element).
This's not the case in customEvent trigger. Here Analytics doesn't actively track the amp-form-submit event, instead it depends on the <amp-form> component to trigger the event with the correct target. TBH I don't think we should expand the selector setting, but to introduce a feature like filter instead.

to @ampproject/wg-ui-and-a11y and @ampproject/wg-stories for ideas on CustomEventTracker and AmpStoryEventTracker .

@zhouyx
Copy link
Contributor

zhouyx commented May 21, 2020

@Enriqe How to filter and only send out storyEvents for selected elements?

@stephengardner
Copy link
Author

Thank you @micajuine-ho and @zhouyx. To add to the suggestions expressed, it would definitely have unexpected side-effects to existing implementations to change the "selector" to querySelectorAll.

Based on documentation here https://amp.dev/documentation/components/amp-analytics/#element-selector a non-breaking change would be adding another option to "selectionMethod" such as "querySelectorAll".

@Enriqe
Copy link
Contributor

Enriqe commented May 21, 2020

@Enriqe How to filter and only send out storyEvents for selected elements?

Hmm sorry, I'm a bit lost here. Are you asking how we currently filter and only fire events from a particular element?

Basically, we compare the tagName provided by the publisher's config with the element responsible firing the event:

On the event side:

const tagName = config['tagName'];
if (
tagName &&
eventDetails['tagName'] &&
tagName.toLowerCase() !== eventDetails['tagName']
) {
return;
}

On the story side, we pass the element with tagName or an element[ANALYTICS_TAG_NAME] property when firing the event so that we can make the comparison

if (element) {
details.tagName =
element[ANALYTICS_TAG_NAME] || element.tagName.toLowerCase();

@zhouyx
Copy link
Contributor

zhouyx commented May 21, 2020

What about the case where there are 3 <amp-foo> that are firing story-foo events.
Is it possible to have events from <amp-foo id=1> <amp-foo id=2> sent to endpoint1, while the other one send to endpoint2?

@Enriqe
Copy link
Contributor

Enriqe commented May 21, 2020

Oh no, I don't think that's possible today.

I can imagine expanding the concept of tagName and adding a new property and attribute to address the requirement here. Something like:

on: story-open,
tagName: amp-story-page-attachment,
category: categoryA,
request: endpoint1

on: story-open,
tagName: amp-story-page-attachment,
category: categoryB,
request: endpoint2
<amp-story-page-attachment category="categoryA" id="1">
<amp-story-page-attachment category="categoryA" id="2">
<amp-story-page-attachment category="categoryB" id="3">

But I guess this would be too similar to selector? And instead of a new attribute we could use the class in the element. But IIUC we can't use selector here because it will only detect the first element. So maybe it makes sense to introduce a new property?

@zhouyx
Copy link
Contributor

zhouyx commented May 22, 2020

Thanks @Enriqe. Sounds like an issue that applied to storyEvents as well.

I like the idea of expanding the selectionMethod. Let me think through this, and see what's a good way to add the support here.

@zhouyx
Copy link
Contributor

zhouyx commented May 28, 2020

Sounds like a filter feature to me. While I'm trying to come up with a design, I realize enabled may be able to solve the issue here.

What about

<amp-analytics type='fake'>
{
  'triggers': {
     'addToCart': {
       'on': 'amp-form-submit',
       'request': 'fake-add-to-cart',
       'enabled': '${addToCartForm}
     }
    ....
  }
}
</amp-analytics>
<amp-form data-vars-add-to-cart-form=true>
<amp-form data-vars-add-to-cart-form=true>
<amp-form data-vars-add-to-cart-form=false>

@stephengardner
Copy link
Author

Thanks @zhouyx, I don't see the enabled attribute documented in the amp analytics page. Is this a feature?

Also, just checking in on any intent to implement an otherwise selector-based approach

Thanks for all the assistance.

@zhouyx
Copy link
Contributor

zhouyx commented May 28, 2020

I don't see the enabled attribute documented in the amp analytics page. Is this a feature?

It's an AMP feature, not a Google Analytics feature. You're absolutely right, we failed to document it : ( Here you can find an example use case by GA. And we need to document the feature : )

Also, just checking in on any intent to implement an otherwise selector-based approach

After reviewing the code, imo this is not a selector. Because the analytics service is not actively selecting element, instead it's passively listening to events. I'm viewing the request as applying additional request filters, and that fits the enabled design concept.

I'm open to other approaches if it makes config setting easier. If possible let me know the proposal you have in mind. Thanks.

@stephengardner
Copy link
Author

Thanks @zhouyx
We spent some time playing with this, and it appears not to work.

Full reproduction example, with steps:
https://stackblitz.com/edit/amp-gtag-example-dmf2ii

Can you clarify what is misconfigured in this example? It appears to follow the method outlined.

@zhouyx
Copy link
Contributor

zhouyx commented May 29, 2020

Unfortunately data-vars is not supported for the on-form-submit event yet. I can fix it you think the approach works for you. (Your configuration looks correct, I'm able to get the request sent out with data vars hard coded)

@stephengardner
Copy link
Author

stephengardner commented May 29, 2020 via email

@zhouyx
Copy link
Contributor

zhouyx commented May 29, 2020

Great. I'll keep you updated on the upcoming support.

@zhouyx
Copy link
Contributor

zhouyx commented May 30, 2020

Created #28636 to track the work

@micajuine-ho
Copy link
Contributor

Closed via #28748

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

4 participants