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

Add scroll trigger for amp-analytics #1490

Closed
rudygalfi opened this issue Jan 20, 2016 · 10 comments
Closed

Add scroll trigger for amp-analytics #1490

rudygalfi opened this issue Jan 20, 2016 · 10 comments

Comments

@rudygalfi
Copy link
Contributor

Offshoot from #871.

We'd like to add a scroll trigger that fires a specified request when the page is scrolled.

@rudygalfi
Copy link
Contributor Author

Slight twist to this proposed in #1511: A trigger when a scroll reaches a particular part of the page, e.g. 25% mark

@devdazed
Copy link

The only issue I see with firing an event on every window.scroll event is that it opens the possibility to drastically slow down the page and impact the user experience. This is largely because the window.scroll event fires pretty close to per-pixel.

I think if we made the event fire per % or per N pixels it will help ensure responsiveness. What are the overall thoughts on this?

@rudygalfi
Copy link
Contributor Author

For scroll specifically I had in mind something like only triggering on the scroll end event (or maybe you have separate triggers for scroll start and end). In any case, definitely not everything as the page scrolls.

If the trigger is instead keyed off of a pixel count or % of document perhaps it could be set up to fire mid-scroll. We'd need to figure out if there's a restriction on the intervals (e.g. you can't make events fire if they're within 100px of each other).

@avimehta What did you originally have in mind for the scroll trigger when you originally proposed it in #871?

@avimehta
Copy link
Contributor

I had in mind what @devdazed suggested in #1511. The reason being that I had only seem publishers keep track of quartiles. For implementation, we would create a measurePromise defined in src/vsync.js and check for scroll position. This way, we only do expensive calculations when it is cheap to do. I have not looked a lot more into implementation but if we had to look at scroll events, we would do it only once and will fire triggers for all vendors using that listener.

As for the config, we can make the trigger configurable such that the config can specify when it should be fired. To keep it manageable, we could limit it to multiples of 5 or 10. So something like:

{
  "triggers": {
    "scroll": {
      "on": "scroll",
      "scroll-config": {
        "trigger-boundaries": [5, 10, 90, 100]
      }
}

Open questions with such a design would be:

  • Does the trigger fire only the first time a user scrolls to a part of the page or does it fire multiple times?
  • Do we limit the number of boundaries a trigger can define?
  • What granularity do we look at?

@devdazed
Copy link

Does the trigger fire only the first time a user scrolls to a part of the page or does it fire multiple times?

The publisher in this case is trying to identify how far the user scrolled to determine how much of the article was read. I don't see a use case where the publisher would want to see who scrolls back up. I think firing only once as the user passes the trigger boundary is sufficient. This can also be a configurable for example:

{
  "triggers": {
    "scroll": {
      "on": "scroll",
      "scroll-config": {
        "trigger-boundaries": [5, 10, 90, 100],
        "trigger-once": true
      }
}

Furthermore, to ensure accuracy, we might want to fire all triggers if one was skipped. Given the event that a user loads a page that is already 50% scrolled, or 50% of the page is in view, then we may want to fire all triggers that are <= 50%.

Do we limit the number of boundaries a trigger can define?

I think the answer here is yes, especially if we fire all previous triggers that haven't been fired on the event. My thought is that 10 is a good number, it gives the opportunity for decile scroll tracking. If the user attempts to define 100 boundaries for percentile tracking then it is likely to impact the user experience.

What granularity do we look at?

I don't think going to fractions of percentages is something that is required. If we limit the number of boundaries then percentile is good, it will allow them to track at any level. If we don't limit the number of boundaries then every 5% would be the max I can see. Publishers generally are interested in quartiles, so 5% increments would allow that.

@avimehta
Copy link
Contributor

Will start working on this based on discussions so far. I hope to have working demo/PR mid to end of week. If others have any requests for changes to above, feel free to chime in.

@avimehta
Copy link
Contributor

Looking at the codebase, it looks like I could use Viewer.onChanged to track all changes in scroll and trigger based on that. This would result in amp-analytics running every time scroll happens. It will be less performant but easier to implement. It'll also be simpler to understand.

An alternative will be to combine onChanged with VSync.measurePromise or runPromise. This way, the expensive operations happen when they are cheaper to do.

@cramforce what do you think?

@cramforce
Copy link
Member

viewer.onChanged fires on scroll and resize. It is throttled and might be perfect for your use case. It never fires during very fast scrolling.

What do you need to measure during this event? For viewer.getScrollTop you can fire it directly in onChanged and do not need your own vsync.

@avimehta
Copy link
Contributor

I need to getScrollTop and then trigger the analytics events based on that.
Sounds like I should start with onChanged for now.

Thanks!

On Tue, Jan 26, 2016 at 2:05 PM, Malte Ubl notifications@github.com wrote:

viewer.onChanged fires on scroll and resize. It is throttled and might be
perfect for your use case. It never fires during very fast scrolling.

What do you need to measure during this event? For viewer.getScrollTop
you can fire it directly in onChanged and do not need your own vsync.


Reply to this email directly or view it on GitHub
#1490 (comment)
.

@cramforce
Copy link
Member

SGTM.

getScrollTop is always safe to call. We call if for you when we know it is as cheap to calculate as possible and returned value is cached.

@rudygalfi rudygalfi modified the milestones: Feb 13, M1 Feb 4, 2016
avimehta added a commit to avimehta/amphtml that referenced this issue Feb 5, 2016
Fixes ampproject#1490

-- Commit 2:
Rename properties with "-" to be camelCased instead.

Disabled a cid test that fails because of change in the way window
object is created.

This helps with consistency.
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