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
Move the iframe's box, not the wrapping AMP Element's #6248
Move the iframe's box, not the wrapping AMP Element's #6248
Conversation
Imagine the iframe's box is smaller. If we moved the AMP Element's `box`, we would have the wrong `width`/`height`, because the iframe is smaller. So avoiding the allocation is the exact wrong thing to do.
@@ -199,9 +199,7 @@ export class AmpAd3PImpl extends AMP.BaseElement { | |||
if (!this.iframeLayoutBox_) { | |||
this.measureIframeLayoutBox_(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
measureIframeLayoutBox_()
returns iframe layoutRect relative to the amp-ad
or amp-iframe
element. However I searched our code and it seems to me the this.iframeLayoutBox_
is only used here. So instead of applying box offset and apply it back here. Why don't we just return layoutRect relative to doc in #measureIframeLayoutBox
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#5271. I should really add a test so someone doesn't "optimize" this away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Please add a comment though. or a link to #5271.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
#getIntersectionElementLayoutBox isn’t appropriate to get the bounding box.
These will both pass, since we're measuring the box just before.
c17dacb
to
d1f1f4a
Compare
* Move the iframe's box, not the wrapping AMP Element's Imagine the iframe's box is smaller. If we moved the AMP Element's `box`, we would have the wrong `width`/`height`, because the iframe is smaller. So avoiding the allocation is the exact wrong thing to do. * Use CustomElement#getLayoutBox #getIntersectionElementLayoutBox isn’t appropriate to get the bounding box. * Add test cases * Assert that the intersection is not the amp-iframe's size, but the iframe's * Comments * Fix assertion * Assert truthiness to pass type check These will both pass, since we're measuring the box just before. * Fix presubmit error * Move Ad3p test into 3p tests
* Move the iframe's box, not the wrapping AMP Element's Imagine the iframe's box is smaller. If we moved the AMP Element's `box`, we would have the wrong `width`/`height`, because the iframe is smaller. So avoiding the allocation is the exact wrong thing to do. * Use CustomElement#getLayoutBox #getIntersectionElementLayoutBox isn’t appropriate to get the bounding box. * Add test cases * Assert that the intersection is not the amp-iframe's size, but the iframe's * Comments * Fix assertion * Assert truthiness to pass type check These will both pass, since we're measuring the box just before. * Fix presubmit error * Move Ad3p test into 3p tests
Imagine the iframe's box is smaller. If we moved the AMP Element's
box
, we would have the wrongwidth
/height
, because the iframe issmaller. So avoiding the allocation is the exact wrong thing to do.