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

Disable sync-xhr in amp-iframe/amp-ad #13766

Closed
RByers opened this Issue Mar 2, 2018 · 8 comments

Comments

@RByers
Copy link

RByers commented Mar 2, 2018

What's the issue?

Chrome 65+ allows synchronous XHR to be disabled. Blocking the UI thread on the network is a perf anti-pattern and so should probably not be allowed in AMP documents or embeds.

How do we reproduce the issue?

  1. Create an amp page with an amp-iframe to a site doing a sync-xhr
  2. View in the top stories carousel
  3. Observe jank if you try to swipe when the sync-XHR occurs (especially on a slow network - use throttling in devtools to emulate).

What browsers are affected?

Chrome for now, but at TPAC all other browsers indicated a desire to add this feature as a first step of trying to phase-out sync-xhr.

Which AMP version is affected?

All

/cc @clelland @cramforce with whom this has been discussed offline.

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Mar 2, 2018

I think we should launch this for amp-ad and amp-iframe.

@lannka Could you take this on?

@cramforce cramforce changed the title Disable sync-xhr in amp-iframe Disable sync-xhr in amp-iframe/amp-ad Mar 17, 2018

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Mar 17, 2018

to @jastiv for prioritization.

@jasti jasti added the Category: Ads label Mar 19, 2018

@jasti jasti modified the milestones: Prioritized FRs, New FRs Mar 19, 2018

@jasti jasti added this to Sprint Candidate in AMP Advertising Mar 19, 2018

@jasti

This comment has been minimized.

Copy link
Collaborator

jasti commented Mar 19, 2018

Given current commitments, it looks like we'll tackle this in the second half of April.
CC @zhouyx

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Mar 19, 2018

I'll try to implement on my flight back from Australia. I'll implement behind an experiment and then we can see how we can ship it.

@jasti

This comment has been minimized.

Copy link
Collaborator

jasti commented Jul 16, 2018

@cramforce did you have a chance to look at this? If not, we can probably reassign to someone on the ads team.

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Jul 16, 2018

Turned this on in canary 5 minutes ago in #16524

Implementation in #14481

cramforce added a commit to cramforce/amphtml that referenced this issue Aug 31, 2018

cramforce added a commit that referenced this issue Aug 31, 2018

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Aug 31, 2018

Shipping this with our next prod release. Let's see how it goes :)

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Sep 18, 2018

Shipped yesterday 🎉

@cramforce cramforce closed this Sep 18, 2018

Enriqe added a commit to Enriqe/amphtml that referenced this issue Nov 28, 2018

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