Skip to content

resources: fix loading sequence of layout=fill and intersection-observer#30167

Merged
samouri merged 1 commit intoampproject:masterfrom
samouri:fill-fix
Sep 10, 2020
Merged

resources: fix loading sequence of layout=fill and intersection-observer#30167
samouri merged 1 commit intoampproject:masterfrom
samouri:fill-fix

Conversation

@samouri
Copy link
Copy Markdown
Member

@samouri samouri commented Sep 9, 2020

summary
Fixes #29398.

Before the IO path of resources-impl, we would build then measure. In the IO branch, the order is measure then build. For most layout types this shouldn't matter, but layout=fill has a clash with i-amphtml-notbuilt. In order for layout=fill to be sized correctly, it must have position: absolute but the notbuilt gives it relative position.

Not ready to be merged as-is, need to research to see if there are other edge cases (layout types) to consider. Notable, all elements must have correct width/height before being built, unless they explicitly request remeasure later (e.g. via mutateElement method)

@amp-owners-bot amp-owners-bot bot requested a review from caroqliu September 9, 2020 17:40
@google-cla google-cla bot added the cla: yes label Sep 9, 2020
@samouri samouri requested review from dvoytenko and jridgewell and removed request for caroqliu September 9, 2020 17:47
@samouri samouri changed the title resources: fix loading sequence of fill and intersection-observer resources: fix loading sequence of layout=fill and intersection-observer Sep 9, 2020
}

.i-amphtml-layout-fill,
.i-amphtml-layout-fill.i-amphtml-notbuilt,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add a comment that this is specifically needed to give higher specificity to this ruleset (vs the .i-amphtml-notbuilt ruleset which is declared later and will win if this uses the same specificity).

Copy link
Copy Markdown
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

Additionally, let's see if we can add a test to cover this scenario so we do not regress.

Copy link
Copy Markdown
Contributor

@dvoytenko dvoytenko left a comment

Choose a reason for hiding this comment

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

Changes LG, but let's add tests.

@samouri
Copy link
Copy Markdown
Member Author

samouri commented Sep 10, 2020

What kind of test?

My first instinct is thinking an e2e test that goes through every relevant extension and ensures it has a nonzero width/height before calling buildCallback. Maybe two cases, one with i-amphtml-notbuilt and one without since there isn't a guarantee on the ordering.

@dreamofabear
Copy link
Copy Markdown

Good sleuthing!

@dvoytenko
Copy link
Copy Markdown
Contributor

@samouri E2e might be hard since you need to catch pre-built state. IMHO, just a simple test with runtimeOff=false would do to check that the computed size is as expected.

@dreamofabear
Copy link
Copy Markdown

I added an integration test for something similar in test-css.js, but I think a visual test might be more understandable. You can simulate unbuilt component by breaking the extension script URL.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

amp-iframe does not load sometimes

6 participants