Skip to content

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

Merged
samouri merged 2 commits intoampproject:masterfrom
samouri:fix-owners-io
Sep 10, 2020
Merged

intersection-observer: don't use premeasure if there is a more recent measure#30188
samouri merged 2 commits intoampproject:masterfrom
samouri:fix-owners-io

Conversation

@samouri
Copy link
Copy Markdown
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
@calebcordry
Copy link
Copy Markdown
Member

calebcordry commented Sep 10, 2020

cc/ @sushan04

@samouri
Copy link
Copy Markdown
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