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.

Sign up for GitHub

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

Jump to bottom

intersection-observer: don't use premeasure if there is a more recent measure #30188

Merged
merged 2 commits into from
Sep 10, 2020
Merged

intersection-observer: don't use premeasure if there is a more recent measure #30188

merged 2 commits into from
Sep 10, 2020

Conversation

samouri
Copy link
Member

@samouri samouri commented Sep 10, 2020 •
edited
Loading

summary
Fixes an issue with intersect-resources experiment where it can use an out-of-date rect. Specifically we were seeing a regression in amp-ad where a carousel 1.0 slides were not being laid out at all. This is due to a mismatch in isDisplayed between the premeasure rect and the more-recent measure within owners.

The sequence of events that causes this bug were:

  1. io callback premeasure occurs with zero height/width (this is valid / can happen for various reasons)
  2. explicit measure happens via the owners system, and a layout is scheduled
  3. an attempt at layout begins but the task is skipped due to checking isDisplayed with the premeasuredRect - see below for the snippet that does the skipping.
  4. io callback premeasure occurs again with positive width/height, but its too late and the slides wont be laid out until the next pass (which isn't scheduled).

amphtml/src/service/resources-impl.js

Lines 1547 to 1556 in cc14c96

if (this.intersectionObserver_) {
// With IntersectionObserver, peek at the premeasured rect to see
// if the resource is still displayed (has a non-zero size).
// The premeasured rect is most analogous to an immediate measure.
if (resource.hasBeenPremeasured()) {
stillDisplayed = resource.isDisplayed(
/* usePremeasuredRect */ true
);
}
} else {

There are numerous ways of trying to fix this. The first two I thought of were:

  1. schedule a new pass each time io callback fires, since we may need to lay items out that were skipped previously
  2. toss out any premeasures if we have something more recent

I went with (2) within this PR

repro html

<!doctype html>
<html amp lang="en">
  <head>
    <meta charset="utf-8">
    <script async src="https://cdn.ampproject.org/v0.js"></script>
    <title>DBM AMP Test Page</title>
    <link rel="canonical" href="http://example.ampproject.org/article-metadata.html">
    <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
    <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
  </head>
  <body>
    <h1> Test AMP Custom Bundle </h1>
    <amp-ad width=300 height=250
        type="doubleclick"
        data-slot="/93132893/suship_jordan_display_08222018">
    </amp-ad>
  </body>
</html>

@samouri samouri requested a review from jridgewell September 10, 2020 20:00
@google-cla google-cla bot added the cla: yes label Sep 10, 2020
@samouri samouri requested a review from dvoytenko September 10, 2020 20:00
jridgewell
jridgewell approved these changes Sep 10, 2020
View reviewed changes
src/service/resource.js Outdated Show resolved Hide resolved
@calebcordry
Copy link
Member

cc/ @sushan04

@samouri
Copy link
Member Author

samouri commented Sep 10, 2020

TODO: percy test amphtml-ads: friendly iframe static (page view) seems to flake on almost every run.

@samouri samouri merged commit c9d13b7 into ampproject:master Sep 10, 2020
@samouri samouri deleted the fix-owners-io branch September 10, 2020 20:58
@samouri samouri self-assigned this Sep 10, 2020
This was referenced Sep 10, 2020
Merged
Closed
@jridgewell jridgewell mentioned this pull request Sep 21, 2020
Closed
@samouri samouri mentioned this pull request Sep 21, 2020
Closed
@jridgewell jridgewell mentioned this pull request Sep 21, 2020
Closed
ampprojectbot pushed a commit that referenced this pull request Oct 1, 2020
@samouri @ampprojectbot
intersection-observer: don't use premeasure if there is a more recent…
f40c30f 
… measure (#30188)

* intersection-observer: dont use premeasure if there is a more recent measure

* fix

(cherry picked from commit c9d13b7)
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
@samouri @ed-bird
intersection-observer: don't use premeasure if there is a more recent…
f51a453 
… measure (ampproject#30188)

* intersection-observer: dont use premeasure if there is a more recent measure

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

@jridgewell jridgewell jridgewell approved these changes

@dvoytenko dvoytenko Awaiting requested review from dvoytenko

Assignees

@samouri samouri

Labels
cla: yes PR use: In Beta / Experimental
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@samouri @calebcordry @jridgewell @ampprojectbot