RFC: Limit the number of tracking iframes that can be created using amp-iframe. #1947
Conversation
cramforce
force-pushed
the
analytics-iframes
branch
6 times, most recently
from
60818b7
to
c649bc4
Compare
| /** @private {!IntersectionObserver} */ | ||
| this.intersectionObserver_ = | ||
| new IntersectionObserver(this, this.iframe_); | ||
| if (!isTracking) { |
There was a problem hiding this comment.
We're not allowing tracking iframes to determine if they're being viewed?
There was a problem hiding this comment.
No. We don't want them at all. amp-analytics is available for that.
cramforce
force-pushed
the
analytics-iframes
branch
from
c649bc4
to
7ab00f7
Compare
…mp-iframe. This will try to identify iframes that are only used for tracking and only render one of them per page and remove them from the page 5 seconds after they loaded.
cramforce
force-pushed
the
analytics-iframes
branch
from
7ab00f7
to
df2d410
Compare
|
|
||
| iframe.onload = () => { | ||
| // Chrome does not reflect the iframe readystate. | ||
| iframe.readyState = 'complete'; | ||
| this.activateIframe_(); | ||
| if (isTracking) { | ||
| timer.promise(trackingIframeTimeout).then(() => { |
|
LGTM |
|
Do not merge this without checking with me first. |
|
@jmadler: You're assigned now. |
|
I want to share the thinking behind this change for those who are curious. The goal of this change is to help provide a path forward for analytics companies who can not move away from client-side instrumentation for AMP in the very short timeframe that fast publisher adoption has provided. Ultimately, it is intended to reduce the use of amp-iframe for analytics in the long-term. #1993 will be used as a temporary transition in amp-analytics for those who need it. We will not merge changes that break amp-iframe usage for analytics iframes until relevant analytics folks have reviewed it and provided comment, and no sooner than April 1st (one and a half months). |
|
Indeed, we do not have plans to break any existing usage of iframes for analytics purposes. This change only limits them to 1. This should be sufficient for everyone as a single iframe can load N analytics providers, but limiting to 1 keeps UI thread stall and memory usage to a minimum. #1993 is meant as a migration path, so one can use an iframe to back and The current plan is to merge a reviewed version of this ASAP (after it has been cleared that it is, indeed, not breaking any existing usage). This change currently warns for the 1st iframe. Maybe we should remove that warning? |
|
Removed warning for first iframe and improved tests. |
RFC: Limit the number of tracking iframes that can be created using amp-iframe.
|
This broke our amp-llightboxes that had iframes in them. How are you determining if an iframe is being used for analytics? I even disabled the analytics code we had in ours and it still broke (the iframe loaded about 75% of the time, randomly). |
|
@travismo123 Could you create jsbin showing the issue? It is size based. |
|
Some of our iframes do have pictures in them, but i am not noticing a specific pattern. Looking at https://www.everipedia.com/everipedia/amp/ on a mobile device and clicking on the blue links is what triggers it. I can try accommodating the size algorithm if need be. |
|
@travismo123 I haven't been able to trigger it. Would you mind filing a new issue. I believe this may be a bug and not intended. FMI: Are you using an AMP plugin with Mediawiki or is this a different wiki software? |
|
We built the wiki software from scratch. The whole point of the amp-lightboxes was to try to allow a user to preview a page instead of just using an anchor, thereby increasing the user experience. I had to do a little bit of coding to get it to work because of AMP's restrictions and the dynamic nature of the page. I have like a 1000 line python function just to AMP-sanitize the page (trying to work with Google here) because as you can imagine, there are a lot of things that can go wrong (tables, pictures, tag attributes, etc) and break the AMP validation. I already did file a bug. Thank you |
|
@travismo123 getting the same problem when clicking your link. |
This will try to identify iframes that are only used for tracking and only render one of them per page and remove them from the page 5 seconds after they loaded.