Modify restrictions for in-viewport expansion #25880
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glevitzky
changed the title
dvoytenko
suggested changes
Dec 18, 2019
| (parent.getLayoutWidth && parent.getLayoutWidth()) || | ||
| parent./*OK*/ getBoundingClientRect().width; | ||
| let cumulativeWidth = widthDiff; | ||
| for (let i = 0; i < parent.childElementCount; i++) { |
There was a problem hiding this comment.
What if this is a structure like this:
super-parent
/ \
kind-of-useless-parent actual-sibling
|
resizing-element
There was a problem hiding this comment.
I don't think this should be an issue. <resizing-element> will never grow wider than <kind-of-useless-parent>, so the invariant that width(<kind-of-useless-parent>) + width(<actual-sibling>) <= width(viewport) should hold.
|
Ok. I'm ok with this change, but please wait for @jridgewell to review as well. |
glevitzky
force-pushed
the
isWidthOnly-fix
branch
from
938fe34
to
912cceb
Compare
jridgewell
approved these changes
Jan 11, 2020
| state.resize = true; | ||
| }, | ||
| mutate: state => { | ||
| if (state.resize) { |
There was a problem hiding this comment.
Should overflowCallback be called?
There was a problem hiding this comment.
If I'm reading the code right, I don't think it's relevant in this case as there's no overflow element.
There was a problem hiding this comment.
Discussed with Dima; realized callback is necessary & added.
dvoytenko
approved these changes
Jan 14, 2020
glevitzky
requested review from
erwinmombay
and removed request for
erwinmombay
jridgewell
approved these changes
Jan 14, 2020
dreamofabear
approved these changes
Jan 14, 2020
estherkim
approved these changes
Jan 14, 2020
|
Thanks, all! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Flexible ad slot expansion was launched earlier this year in August. The new behavior enables large creatives to be rendered within smaller ad slots, necessarily requiring the runtime to expand the slot element at render-time. As preparation for the launch, we made a slight change to AMP’s no-reflow policy, to allow in-viewport expansion if the following criteria hold:
These conditions, however, are too restrictive to allow in-viewport expansion in many cases, resulting in cropped ad slots that load within the viewport. This PR will modify the criteria for in-viewport expansion to the following:
This should result in many fewer cropped ad slots, without introducing any risk of reflow.