Consolidated frame attribute assigning. #7841
Conversation
src/iframe-attributes.js
Outdated
| mode: getModeObject(), | ||
| canary: isCanary(parentWindow), | ||
| hidden: !viewer.isVisible(), | ||
| initialIntersection: element.getIntersectionChangeEntry ? |
There was a problem hiding this comment.
Yes, this method does not exist for the A4A ad element. Without this check, it fails in the A4A cases.
There was a problem hiding this comment.
isn't "A4A ad element" the same as amp-ad element? It should have this method. are you saying this method does not exist in runtime? or just in test?
There was a problem hiding this comment.
No, I was referring to the <amp-a4a> element, not the <amp-ad> element. The <amp-a4a> element does not have getIntersectionChangeEntry defined on it.
There was a problem hiding this comment.
all AMP created custom element should have this method: https://github.com/ampproject/amphtml/blob/master/src/custom-element.js#L1105
There was a problem hiding this comment.
Hmm that's really weird then. I'll look into it a little deeper.
There was a problem hiding this comment.
Fixed the issue. I had thought that the A4A path didn't have the getIntersectionChangeEntry because in all of our tests the element didn't have it. But it turns out that this way because our tests weren't really making an AMP element, they were just making a dummy HTML element with some amount of AMP element methods attached to it. Didn't take that long to realize the test itself was what was broken, but took me and Taymon a fair while to actually get to the root cause.
Thanks for catching my error here, Hongfei! Showed the team some potential issues with our testing that we are going to try to fix as soon as we can.
* Consolidated frame attribute assigning. * Fixed broken test infrastructure; Fixed attributes * Add iframe-attributes.js to whitelist for getModeObject
* Consolidated frame attribute assigning. * Fixed broken test infrastructure; Fixed attributes * Add iframe-attributes.js to whitelist for getModeObject
Consolidated the attributes assigned on iframes to achieve parity between 3p and a4a.