Skip to content
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

Minimal AMP support: don't invalidate AMP #1263

Closed
jmarantz opened this issue Feb 12, 2016 · 24 comments
Closed

Minimal AMP support: don't invalidate AMP #1263

jmarantz opened this issue Feb 12, 2016 · 24 comments
Assignees
Milestone

Comments

@jmarantz
Copy link
Contributor

@jmarantz jmarantz commented Feb 12, 2016

PageSpeed should never inject JavaScript into AMP pages, as that will invalidate them. I think to determine that we need to look for <html AMP>, although the spec which has a utf-8 lightning bolt character rather than "AMP": <html ⚡>. See https://www.ampproject.org/docs/get_started/about-amp.html . (how does that work when the site is in russian or chinese encoding???)

PageSpeed should optimize src in amp-img tags, which starts with adding some default UrlValuedAttribute settings. See https://www.ampproject.org/docs/reference/amp-img.html . But for the moment, PageSpeed should not transcode amp-img URLs to webp because AMP pages must be served as publicly cacheable without varying on user-agent. So some kind of element equivalent must be employed to allow for UA-dependent img tags.

@HansVanEijsden
Copy link

@HansVanEijsden HansVanEijsden commented Feb 16, 2016

Agreed. For now I disabled Pagespeed for /amp/:
pagespeed Disallow */amp/*;

@jmarantz
Copy link
Contributor Author

@jmarantz jmarantz commented Feb 22, 2016

I think you might need some stars for that to work, no?

pagespeed Disallow /amp/;

On Tue, Feb 16, 2016 at 12:25 PM, Hans van Eijsden <notifications@github.com

wrote:

Agreed. For now I disabled Pagespeed for /amp/:
pagespeed Disallow /amp/;


Reply to this email directly or view it on GitHub
#1263 (comment)
.

@jmaessen
Copy link
Contributor

@jmaessen jmaessen commented Feb 22, 2016

I think the lightning bolt is also legal. UTF-8 is required I think, and this is intended to enforce that.

@crowell crowell added this to the Pagespeed 2.0 milestone Feb 26, 2016
@jmarantz jmarantz self-assigned this Mar 8, 2016
@oschaaf
Copy link
Member

@oschaaf oschaaf commented Mar 15, 2016

Related post about http-equiv=refresh injection invalidating AMP:
https://groups.google.com/forum/#!topic/ngx-pagespeed-discuss/3T4HISDLbW4

jmarantz added a commit that referenced this issue Mar 16, 2016
going to filters, except for ones added as event_listeners.

Starts addressing #1263
@buro9
Copy link

@buro9 buro9 commented Mar 16, 2016

@oschaaf worth bearing in mind that the http-equiv=refresh meta tag is in fact HTTP equivalent... and you can simply remove the meta tag altogether and insert it as a HTTP header to achieve the same effect. To my knowledge AMP only specifies HTML, rather than HTTP, and this is a valid HTTP header.

@jmarantz
Copy link
Contributor Author

@jmarantz jmarantz commented Mar 16, 2016

mod_pagespeed uses the http-equiv=refresh meta tag in situations where it has already flushed response headers, and while streaming HTML discovers something unusual that makes it want the browser to start over...not a good day out for optimization.

One example where this might occur is when the server has lazyload_images enabled but JS is disabled in a browser, so the correct image will never replace the blank one. So we put the meta-refresh block in a tag to send the browser to the same page with lazyload and other js-dependent features disabled.

I don't know how I could replicate this behavior with an http header.

jmarantz added a commit that referenced this issue Mar 19, 2016
the AMP-ness is discovered.

Continues addressing
#1263 .

See also: ampproject/amphtml#2380
jmarantz added a commit that referenced this issue Mar 23, 2016
explicitly:
  1. AddInstrumentation, which needs to be completely disabled in AMP.
  2. responsive_images & rewrite_domains, which need to be partially
     disabled in AMP.

Partially addresses:
#1263 . It's possible
that this commmit fully addresses that bug, but validation is needed.
@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Oct 3, 2016

@jmarantz what's the status of this? With ad71403 and 7bf8083 submitted is this done?

(It doesn't look like ad71403 or 7bf8083 have been released as bugfixes, so it looks like this would be a fix with 1.12)

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Oct 11, 2016

@jmarantz confirms offline that this is done, we optimize amp-img, and there aren't any current cases where we're aware of breaking AMP

@jmarantz
Copy link
Contributor Author

@jmarantz jmarantz commented Oct 11, 2016

Sorry: I don't think we optimize amp-img by default yet, however I think we
can do that with a separate configuration line. Maybe add that to the
release notes for now?

On Tue, Oct 11, 2016 at 7:19 AM, Jeff Kaufman notifications@github.com
wrote:

@jmarantz https://github.com/jmarantz confirms offline that this is
done, we optimize amp-img, and there aren't any current cases where we're
aware of breaking AMP


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1263 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB2kPR_bNL5HQ2554GnOZqAfAJox3F6Lks5qy3DWgaJpZM4HZSZj
.

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Oct 11, 2016

I think UrlValuedAttribute amp-img src image will probably work, but I should test it. UrlValuedAttribute amp-image srcset ?? isn't possible though.

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Oct 11, 2016

This is probably a pretty small change to fix by default; let me look.

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Oct 11, 2016

Sent a CL out for review, but I think it's too complex to backport to v34 and include in 1.12. Filed a new bug for optimizing images in AMP: #1413

@jeffkaufman jeffkaufman changed the title Minimal AMP support: don't invalidate AMP, and do optimize images Minimal AMP support: don't invalidate AMP Oct 11, 2016
@Lofesa
Copy link
Contributor

@Lofesa Lofesa commented Dec 19, 2016

Sorry if anyone consider I am spaming this treat but I think is related.
In AMP pages the Prioritize Critical Css must be disabled. This filter inject a js script that is considered an error in the amp validation tool (and duplicates all the css)

@jmarantz
Copy link
Contributor Author

@jmarantz jmarantz commented Dec 19, 2016

Lofesa, are you saying that you have installed 1.12 and found that amp pages are being served with prioritize critical CSS enabled?

@hillsp
Copy link
Contributor

@hillsp hillsp commented Dec 19, 2016

It looks like CriticalSelectorFilter::GetScriptUsage does return kWillInjectScripts and isn't in RewriteOptions::kRequiresScriptExecutionFilterSet. It ought to have been disabled but clearly it wasn't.

My best guess is that something about the site is fooling our AMP detector. It's either that or the auto-disabling of JS filters for AMP isn't working.

@Lofesa
Copy link
Contributor

@Lofesa Lofesa commented Dec 19, 2016

Hi @hillsp
The site is a WP and get the AMP pages with these plugins:

https://wordpress.org/plugins/amp/
https://wordpress.org/plugins/accelerated-mobile-pages/

All pages and post works with /?amp and post with /amp/

Images url are not rewrited but the etag is like W/"PSAxxxxxxxx and had an x-original-content-length

@jmarantz
Copy link
Contributor Author

@jmarantz jmarantz commented Dec 20, 2016

Hi Lofesa -- I'll take a look at this. But can you open up a new issue (pointing to this one)? This one was already closed and it would be easier for us to track it as a new issue.

@jmarantz
Copy link
Contributor Author

@jmarantz jmarantz commented Dec 20, 2016

Lofessa -- I made a small testcase and could not repro in mod_pagespeed. Checking ngx_pagespeed now.

@jmarantz
Copy link
Contributor Author

@jmarantz jmarantz commented Dec 20, 2016

Can't repro this in ngx_pagespeed either. Can you send your config?

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Dec 20, 2016

If there's a bug in our amp detection and you need to turn off a filter for some pages, one way to do this in nginx would be roughly:

http {
   pagespeed ProcessScriptVariables on;
   ...
   server {
      ...
      set $disable_filters "";
      if ($request_uri ~ .*/amp/.*) {
          set $disable_filters "prioritize_critical_css";
      }
      pagespeed DisableFilters "$disable_filters";
      ...
   }
}

See https://developers.google.com/speed/pagespeed/module/system#nginx_script_variables and https://developers.google.com/speed/pagespeed/module/https_support#h2_configuration_nginx

I haven't tested this configuration, though

@Lofesa
Copy link
Contributor

@Lofesa Lofesa commented Dec 20, 2016

@jmarantz this work but I leave the config w/o it so, if needed, you can see the js injected

@djixas
Copy link

@djixas djixas commented Jul 16, 2019

Today I've had this issue where mod pagespeed started injecting custom javascript into every AMP page, causing every one of them to become invalid.

<noscript><meta HTTP-EQUIV="refresh" content="0;url='https://xxx.com amp/?PageSpeed=noscript'" /><style><!--table,div,span,font,p{display:none} --></style><div style="display:block">Please click <a href="https://xxx.com/---/amp/?PageSpeed=noscript">here</a> if you are not redirected within a few seconds.</div></noscript>

This should never happen.

@jmarantz
Copy link
Contributor Author

@jmarantz jmarantz commented Jul 16, 2019

@djixas it would be useful to know whether this is a case of:

  • Your AMP document is not recognized as AMP by PageSpeed
  • Your AMP document is recognized, but we are managing to inject a script anyway.

Related: what version of PageSpeed are you running?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants