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

Simplify scroll tracking and greatly increase frequency of viewability events #1163

Merged
merged 1 commit into from
Dec 18, 2015

Conversation

cramforce
Copy link
Member

  • The scroll event now always fires in vsync and always measures
    scrollTop.
  • The viewport change event now happens at the latest 3 frames after
    scrolling ended.
  • Increased speed at which we send change event. Needed due to resolution at slow speed, but I also feel the value is better.

Changes ad intersection to fire "on scroll". Fixes #873

Fixes a bug where only the first viewability provider in an ad would get an initial viewability change record. Fixes #1126

this.scrollTop_ = newScrollTop;
this.scrollMeasureTime_ = timer.now();
timer.delay(() => this.scrollDeferred_(), 500);
this.vsync_.measure(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is vsync for throttling of for sync? Normally, when scroll event arrives, the value is already calculated by the browser and no sync needed. That, of course, mightn't always work with some of the bindings we have.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Moved out of vsync.

Actually, if you only measure and do not mutate, it is always fine to do that in an event handler. In the worst case it trigger recalc but it would make the next vsync cheaper since we start with the measure phase.

@cramforce cramforce force-pushed the viewability-frequency branch 2 times, most recently from 918aa71 to 05ebec2 Compare December 16, 2015 15:45
@cramforce
Copy link
Member Author

PTAL

/**
* This method is called about every 3 frames (assuming 60hz) and it
* is called in a vsync measure task.
* @private
Copy link
Contributor

Choose a reason for hiding this comment

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

Add @param

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@dvoytenko
Copy link
Contributor

LGTM with one comment to fix jsdoc with @param.

emit viewport change events on scroll.

- The scroll event now always fires in vsync and always measures
scrollTop.
- The viewport change event now happens at the latest 3 frames after
scrolling ended.
- Increased speed at which we send change event. Needed due to resolution at slow speed, but I also feel the value is better.

Changes ad intersection to fire "on scroll". Fixes ampproject#873

Fixes a bug where only the first viewability provider in an ad would get an initial viewability change record. Fixes ampproject#1126
cramforce added a commit that referenced this pull request Dec 18, 2015
Simplify scroll tracking and greatly increase frequency of viewability events
@cramforce cramforce merged commit 47f3ef7 into ampproject:master Dec 18, 2015
@cramforce cramforce deleted the viewability-frequency branch December 18, 2015 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants