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

More AMP compatibility #9730

Open
gravityrail opened this Issue Jun 11, 2018 · 18 comments

Comments

Projects
None yet
6 participants
@gravityrail
Copy link
Contributor

gravityrail commented Jun 11, 2018

From @westonruter:

FYI: When I run AMP on my blog with Jetpack it is catching these additional markup items as validation errors:
• Enqueued _inc/build/shortcodes/js/gist.min.js
• Inline script mentioning wpNotesIsJetpackClient
• Excessive CSS for modules/contact-form/css/grunion.css
• Enqueued modules/wpgroho.js
• Enqueued _inc/build/twitter-timeline.min.js

@stale

This comment has been minimized.

Copy link

stale bot commented Dec 9, 2018

This issue has been marked as stale. This happened because:

  • It has been inactive in the past 6 months.
  • It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this ticket has clear reproduction steps and it is still reproducible. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

@gravityrail

This comment has been minimized.

Copy link
Contributor Author

gravityrail commented Dec 11, 2018

This should be fixed by #10945

@westonruter

This comment has been minimized.

Copy link
Contributor

westonruter commented Dec 11, 2018

@gravityrail The PR #10945 would only address a subset of the above, if anything. An audit of all of the modules' AMP compatibility needs to be done, and this issue I think could serve as an epic to that end.

@gravityrail

This comment has been minimized.

Copy link
Contributor Author

gravityrail commented Dec 12, 2018

My bad, you're right. I mis-read this in my eagerness to close stale issues.

@westonruter

This comment has been minimized.

Copy link
Contributor

westonruter commented Jan 9, 2019

Also needing improved compatibility is the Comments module. See AMP validation errors that are generated here: ampproject/amp-wp#1785 (comment)

@jeherve

This comment has been minimized.

Copy link
Member

jeherve commented Jan 9, 2019

Adding to this, we'll also need to make sure our different blocks, especially those that use forms and JavaScript, can be used with AMP.

@westonruter

This comment has been minimized.

Copy link
Contributor

westonruter commented Jan 9, 2019

As long as these have noscript fallbacks then those can be used as the AMP version as well.

@westonruter

This comment has been minimized.

Copy link
Contributor

westonruter commented Jan 16, 2019

For social share, it is possible to style <amp-social-share> to look like Jetpack's normal sharing buttons, as can be seen here: https://timesofindia.indiatimes.com/blogs/short-simple/it-wasnt-a-defeat-indian-football-and-its-future-falter-hand-in-hand/

@westonruter

This comment has been minimized.

Copy link
Contributor

westonruter commented Jan 18, 2019

Just to link issues, for Related Posts see #9556.

@jeherve

This comment has been minimized.

Copy link
Member

jeherve commented Feb 4, 2019

We should probably look at the output of our different blocks as well:
https://jetpack.com/support/jetpack-blocks/

The upcoming GIF block (scheduled for release tomorrow), for example, is very broken right now.

@westonruter

This comment has been minimized.

Copy link
Contributor

westonruter commented Feb 10, 2019

The upcoming GIF block (scheduled for release tomorrow), for example, is very broken right now.

Opened #11318 to begin to address that.

jeherve added a commit that referenced this issue Feb 18, 2019

Short-circuit Asset CDN in AMP responses (#11374)
I noticed that when turning on the Asset CDN module that styles were broken in AMP responses. In looking at the AMP plugin's CSS manifest comment I saw that that the Dashicons were were way larger than expected:

```
 35249 B (90%): link#dashicons-css[rel=stylesheet][id=dashicons-css][href=https://c0.wp.com/c/5.0.3/wp-includes/css/dashicons.min.css][type=text/css][media=all]
```

This is due to the fact that the AMP plugin was loading the CSS from the remote URL as opposed to using the local copy. Recall that AMP requires all CSS to be inlined (except for CDN stylesheets). Also, AMP allows a max of 50KB. Since the `data:` URL for the Dashicons font is very large, the AMP plugin will instead try to reference the font file with `https:` instead of the `data:` URL. See previously in #9513. All of this to say, the Asset CDN is breaking this since it fetches the CSS from a remote location. And in the `style[amp-custom]` element I see unexpectedly:

```css
@font-face{font-family:dashicons;src:url("data:application/font-woff;charset=utf-8;base64,d09GRgABAAAAAGYMAA4AAAAAowAAAQ..."
```

When there should instead be:

```css
@font-face{font-family:dashicons;src:url("https://example.com/wp-includes/fonts/dashicons.woff") format("woff");
```

The `data:` URL is causing the theme's stylesheet to be excluded:

```
The following stylesheets are too large to be included in style[amp-custom]:
 23967 B (64%): link#twentyseventeen-parent-style-css[rel=stylesheet][id=twentyseventeen-parent-style-css][href=https://amp-jetpack-westonruter.pantheonsite.io/wp-content/themes/twentyseventeen/style.css?ver=1.1][type=text/css][media=all]
Total excluded size: 23,967 bytes (64% of 37,093 total after tree shaking)
```

So the fix is simply to short-circuit the Asset CDN from doing its thing when the response will be an AMP page. As noted in the comments, the Asset CDN is not relevant in AMP because custom JS is not allowed and CSS is inlined. Note also that it is not suitable to use the `jetpack_force_disable_site_accelerator` filter for this because it will be applied before the `wp` action, which is the point at which the queried object is available and we know whether the response will be AMP or not. This is particularly important for AMP-first (native AMP) pages where there are no AMP-specific URLs. For more on that, see #11195.

See #9730 which is the master issue for AMP compatibility.

#### Changes proposed in this Pull Request:

* Short-circuit Asset CDN module in AMP responses.

#### Testing instructions:

0. Activate the Twenty Seventeen theme.
1. Activate the [AMP plugin](https://github.com/ampproject/amp-wp).
2. On the AMP settings screen, turn on Native mode.
3. Enable the Asset CDN module.
4. View the frontend and notice that the page looks broken in terms of styling, and the AMP CSS unexpectedly includes a `data:` URL for the font.  
5. Switch to this branch and notice the styling is no longer broken and the AMP CSS no longer uses the `data:` URL for the Dashicons font.

#### Proposed changelog entry for your changes:

* Add AMP compatibility for the Asset CDN module.
@swissspidy

This comment has been minimized.

Copy link
Contributor

swissspidy commented Feb 19, 2019

FYI, element-level infinite scroll support is currently being worked on in AMP, see ampproject/amphtml#14060. It's experimentally available under amp-list-load-more and supports both automatic loading of items as well as manual loading using a "load more" button, see https://www.ampproject.org/docs/reference/components/amp-list#load-more-button.

This could potentially be used to make the Infinite Scroll module in Jetpack AMP compatible. I know that the Jetpack module is quite complex, making sure to enqueue scripts and calling the main query, etc. So this wouldn't be a one-stop solution, but perhaps this information is useful for people trying to replicate infinite scroll in their projects and looking at this GitHub repository.

@westonruter

This comment has been minimized.

Copy link
Contributor

westonruter commented Feb 19, 2019

A lot of the complexity in the Infinite Scroll module is to deal with script dependencies. Since AMP doesn't allow custom scripts, then I believe a lot of the complexity would be minimized.

@westonruter

This comment has been minimized.

Copy link
Contributor

westonruter commented Feb 27, 2019

Pull requests with the AMP label: https://github.com/Automattic/jetpack/pulls?q=is%3Apr+label%3AAMP
Issues with the AMP label: https://github.com/Automattic/jetpack/issues?q=is%3Aissue+label%3AAMP

I've going through the modules one-by-one to check them for AMP compatibility issues. Here's a table of the results I have so far, which I'll keep editing until I finish:

Module Compat Notes
Ads 🚫🚫 Not compatible due to adding script elements directly to the page. No fallbacks are in place. Needs to output amp-ad components instead.
Asset CDN Compatibility achieved in #11374 simply by short-circuiting in AMP, since not relevant.
Beautiful Math Compatible out of the box since an img gets converted to amp-img. Nevertheless, see #11375 for a future enhancement to use amp-mathml.
Block: Business Hours Fully compatible as-is.
Block: Contact Info: ⚠️ Block causes validation errors due to one enqueued script: contact-info/view.js with an inline script to define Jetpack_Block_Assets_Base_Url. In spite of this, there is visual parity. There doesn't seem to be a need to enqueue the JS file at all.
Block: GIF 🚫 There are styling problems in AMP as well as validation errors. See #11318 for a PR which fixes this.
Block: Map 🚫🚫 There are 11 AMP validation errors. The no-script fallback behavior does at least link to the marker address on Google Maps, but no map is shown at all. This will likely need a significant amount of work, since the block doesn't use an iframe fundamentally. So amp-iframe is not a drop-in replacement. May be a key area for amp-script to be employed.
Block: Markdown Fully compatible as-is.
Block: Slideshow 🚫🚫 Completely broken in AMP; no fallback content. There are 4 validation errors. In spite of this, the block should be straightforward to port to AMP via amp-carousel.
Block: Tiled Gallery 🚫 The styles for Circles and Square Tiles are compatible. However, the styles for Tiled Mosaic and Tiled Columns do not have the right spacing in the grid on the AMP page. So there is good fallback content, but full visual parity is hindered by the scripts it expects (tiled-gallery/view.js, token-list.min.js, i18n.min.js, etc). Interestingly, the AMP version looks much better than the non-AMP version with JS turned off (may be due to the image dimensions being supplied).
Carousel ⚠️ Functionality currently short-circuits via #9458, #10945, #11195; nevertheless, AMP's own amp-lightbox-gallery should be easily used to port the functionality rather than just disabling.
Comment Likes ⚠️ Currently short-circuited by #9458. Will require coordination with WordPress.com to come up with an AMP solution.
Comments 🚫🚫 Generates AMP Validation errors in Jetpack_Comments::watch_comment_parent() for an invalid script and in Jetpack_Comments::comment_form_after() for invalid name attribute on iframe. Given the iframe there may be work needed on the WordPress.com side to be compatible. In particular, the JS for moving the comment form iframe when doing inline replies, resizing the iframe when the textarea grows, and making sure submission works as expected. If there can be an alternative to an iframe that would be preferred.
Contact Form ⚠️ The Date Picker results in 6 scripts being added, 5 external scripts. Simply replacing this with amp-date-picker will make this Module fully AMP-compatible.
Copy Post n/a Nothing is enqueued on the frontend, so AMP-compatible.
Custom CSS The AMP plugin already handles Custom CSS feature in core, and the Custom CSS module just enhances it. So it is AMP compatible.
Custom content types AMP plugin enables access to single and archive templates like any other post type; shortcodes also work, ⚠️ although there is a stretching issue for the [testimonials] shortcode in Twenty Seventeen (at least).
Data Backups n/a Nothing added to frontend.
Enhanced Distribution n/a Only relevant to RSS feeds. No markup added to HTML pages.
Extra Sidebar Widgets: Authors Widget Fully compatible.
Extra Sidebar Widgets: Blog Stats Widget Fully compatible.
Extra Sidebar Widgets: Contact Info Widget Fully compatible with #11443.
Extra Sidebar Widgets: Cookies & Consent Banner 🚫 AMP validation error due to build/widgets/eu-cookie-law/eu-cookie-law.min.js being enqueued. No banner is shown at all. Needs to leverage AMP components to implement.
Extra Sidebar Widgets: Display WordPress Posts Widget Fully compatible.
Extra Sidebar Widgets: Facebook Page Plugin Widget 🚫 The AMP plugin is not currently converting the Facebook properly, as per ampproject/amp-wp#1259. Nevertheless, Jetpack could preempt the problem by outputting an amp-facebook-page from the start.
Extra Sidebar Widgets: Flickr Widget Fully compatible.
Extra Sidebar Widgets: Goodreads Widget 🚫🚫 1 validation error due to script. No noscript fallback behavior for AMP. Also, the HTML is being injected with innerHTML so no amp-iframe solution is viable.
Extra Sidebar Widgets: Google Translate Widget 🚫🚫 3 validation errors due to enqueued scripts. No fallback behavior, though we could devise an AMP solution by adding a form with a select dropdown of all languages, and then navigate the user to a URL like https://translate.google.com/translate?sl=en&tl=es&u=https%3A%2F%2Fexample.com%2F, for a blog in English being translated into Spanish.
Extra Sidebar Widgets: Gravatar Profile Widget Fully compatible.
Extra Sidebar Widgets: Internet Defense League 🚫 One validation error for campaign script. But otherwise AMP-compatible in appearance.
Extra Sidebar Widgets: MailChimp Subscriber Popup Widget 🚫 Can't figure out how to configure in order to test, but almost certainly results in a script being added to the page, so not AMP-compatible.
Extra Sidebar Widgets: Milestone Widget ⚠️ Compatible due to to milestone script being not enqueued due to change in #10945. Fallback behavior just prevents live updating of countdown in page. This is somewhere that the amp-date-countdown component can be used.
Extra Sidebar Widgets: My Community Widget 🚫 Visual parity in AMP, but there are validation errors due to non-standard attributes originals="240" and scale="1" on the Gravatar images. Will be easy to fix.
Extra Sidebar Widgets: RSS Links ⚠️ Compatible without any validation errors, but there is a small styling problem in Twenty Seventeen where link text is cut off when selecting "Text & Image Links" format.
Extra Sidebar Widgets: Social Icons ⚠️ Compatible without any validation errors, but social icons are not displayed horizontally as expected; they are displayed vertically in small icons even when selecting Large.
Extra Sidebar Widgets: Top Posts & Pages Widget Fully compatible.
Extra Sidebar Widgets: Twitter Timeline Widget 🚫 Validation error due to twitter-timeline.min.js script being enqueued. Only fallback content is link to user profile. Nevertheless, a Twitter timeline oEmbed URL in a Text widget does just work, as the AMP plugin does the conversion. So this may just be a matter of the widget outputting the the right amp-twitter from the start.
Extra Sidebar Widgets: Upcoming Events Widget Fully compatible.
Google Analytics 🚫🚫 Not compatible due to adding script to the page. Needs to switch to amp-analytics.
Gravatar Hovercards 🚫🚫 AMP validation errors due to enqueueing scripts like gprofiles.js and wpgroho.js. Fully depends on JS for showing the card on hover and for positioning it above the gravatar. No fallback behavior other than just showing the original gravatar.
Infinite Scroll 🚫🚫 Not compatible. Many enqueued scripts (tiled-gallery.min.js, spin.min.js, jquery.spin.min.js, infinity.min.js, etc). Will require AMP-based alternative, such as an element-level infinite scroll (ampproject/amphtml#14060).
JSON API n/a Fully compatible. Not related to HTML pages.
Lazy Images Lazy-loaded images are part of amp-img and so module is just disabled in AMP responses. Done in #9458 and #10945.
Likes 🚫🚫 Currently short-circuited by #9458. Will require coordination with WordPress.com to come up with an AMP solution.
Markdown -- See Markdown Block above.
Mobile Theme ⚠️ This module should be disabled entirely when AMP is active.
Module Extra: calypsoify TODO --
Module Extra: custom-post-types TODO --
Module Extra: geo-location TODO There is a [geo-location] shortcode that needs to be checked.
Module Extra: plugin-search TODO --
Module Extra: seo-tools TODO --
Module Extra: simple-payments TODO --
Module Extra: theme-tools TODO --
Module Extra: theme-tools TODO --
Module Extra: verification-tools TODO --
Module Extra: videopress TODO --
Module Extra: woocommerce-analytics TODO --
Monitor n/a Does not relate to markup in pages.
Notifications (Notes) 🚫🚫 Causes 9 validation errors, including excessive_css and invalid scripts. This module is checking the show_admin_bar filter too early for AMP, so it's not respecting the setting to not show the admin bar in AMP pages. As for actually porting the functionality to work when the admin bar is displayed, this will be a whole other thing. It's possible that amp-list could be used.
Photon Compatible. Compat work done in #9458. There does not seem to be any features that depend on the photon.js file, although this needs to be verifed.
Post by email n/a Nothing added to frontend.
Progressive Web Apps Adds Web App Manifest link.
Protect n/a Nothing added to frontend.
Publicize n/a Nothing added to frontend.
Related posts 🚫 Not compatible, but there is a prototype AMP-compatible implementation using amp-list found at #9556 (comment)
SEO Tools Compatible as it just adds meta tags.
Search 🚫 Adding Search widget search-widget.js as well as a script for maybe_render_sort_javascript.
Secure Sign On n/a Nothing added to frontend.
Sharing ⚠️ AMP-compatible sharing buttons added via amp-social-share for Twitter, Facebook, LinkedIn, Tumblr, and Pinterest (see #9458). However, not implemented are: Pocket, Skype, Print, Reddit, Telegram, and Whatsapp. However, the style from the non-AMP version is not applied (see #9730 (comment)).
Shortcode Embed: archiveorg-book TODO --
Shortcode Embed: archiveorg TODO --
Shortcode Embed: archives TODO --
Shortcode Embed: bandcamp TODO --
Shortcode Embed: brightcove TODO --
Shortcode Embed: crowdsignal/polldaddy The oEmbed is supported by the AMP plugin (ampproject/amp-wp#1929) but Jetpack's shortcodes are done #11484.
Shortcode Embed: dailymotion-channel TODO --
Shortcode Embed: dailymotion TODO --
Shortcode Embed: digg TODO --
Shortcode Embed: facebook TODO --
Shortcode Embed: flickr TODO --
Shortcode Embed: getty TODO --
Shortcode Embed: gist TODO --
Shortcode Embed: googleapps TODO --
Shortcode Embed: googlemaps TODO --
Shortcode Embed: googleplus TODO --
Shortcode Embed: googlevideo TODO --
Shortcode Embed: gravatar_profile TODO --
Shortcode Embed: gravatar TODO --
Shortcode Embed: houzz TODO --
Shortcode Embed: hulu TODO --
Shortcode Embed: instagram TODO --
Shortcode Embed: kickstarter TODO --
Shortcode Embed: lytro TODO --
Shortcode Embed: mailchimp_subscriber_popup TODO --
Shortcode Embed: medium TODO --
Shortcode Embed: mixcloud TODO --
Shortcode Embed: mixcloud TODO --
Shortcode Embed: presentation/slide TODO --
Shortcode Embed: quiz TODO --
Shortcode Embed: recipe, … TODO --
Shortcode Embed: scribd TODO --
Shortcode Embed: sitemap TODO --
Shortcode Embed: slideshare TODO --
Shortcode Embed: slideshow TODO --
Shortcode Embed: soundcloud TODO --
Shortcode Embed: spotify TODO --
Shortcode Embed: ted TODO --
Shortcode Embed: tweet TODO --
Shortcode Embed: twitchtv, … TODO --
Shortcode Embed: twitter-timeline TODO --
Shortcode Embed: untappd-menu TODO --
Shortcode Embed: upcomingevents TODO --
Shortcode Embed: ustream, … TODO --
Shortcode Embed: vimeo TODO --
Shortcode Embed: vine TODO --
Shortcode Embed: vr TODO --
Shortcode Embed: wordads TODO --
Shortcode Embed: wufoo TODO --
Shortcode Embed: youtube TODO --
Shortcode Embed: youtube TODO --
Site Stats Compatible as of #10945 due to use of amp-img in Admin Bar.
Site verification Compatible, as only adds meta tags.
Sitemaps n/a Nothing added to the frontend.
Spelling and Grammar n/a Nothing added to the frontend.
Subscriptions Enqueues styles and adds Blog Subscription widget. Full compatibility was done in #11159.
Tiled Galleries 🚫 See Tiled Gallery block above. In addition to the Slideshow block and Tiled Gallery block, there are also legacy galleries which also need to be accounted for. While a classic gallery with a Slideshow type gets the classic AMP plugin slideshow, the other gallery types also get forcibly made into a slideshow as well, when they should be showing as grids.
VideoPress ⚠️ Compatible when forced into “freedom” mode to output \VideoPress_Player::html5_static() via \Jetpack_AMP_Support::videopress_enable_freedom_mode(). The code that does this was originally in the AMP plugin but it has been moved as of #9458. The jetpack-helper.php in the AMP plugin should be able to be removed; see ampproject/amp-wp#1925. However, is the freedom mode always viable?
WP.me Shortlinks Compatible. Just adds a link to the head.
Widget Visibility Compatible. Does not add anything to frontend.
WordPress.com Toolbar ⚠️ Feature was short-circuited in #10945 and #11088. For the feature to be used in AMP, will need to be re-written per description in #9458.
@jeherve

This comment has been minimized.

Copy link
Member

jeherve commented Feb 27, 2019

Thank you for that!

@westonruter

This comment has been minimized.

Copy link
Contributor

westonruter commented Feb 28, 2019

I've finished the audit, except for the shortcode embeds and the premium modules.

Update: Premium modules now included.

@simison

This comment has been minimized.

Copy link
Member

simison commented Mar 4, 2019

Block causes validation errors due to one enqueued script: contact-info/view.js with an inline script to define Jetpack_Block_Assets_Base_Url. In spite of this, there is visual parity. There doesn't seem to be a need to enqueue the JS file at all.

Tracking this at https://github.com/Automattic/wp-calypso/issues/31179

Good catch!

@amedina

This comment has been minimized.

Copy link

amedina commented Mar 13, 2019

/cc @jessefriedman (Following up on our chat)

jeherve added a commit that referenced this issue Mar 22, 2019

Fix height of Contact Info map iframe in AMP (#11443)
See #9730.

#### Changes proposed in this Pull Request:

Since AMP has built-in responsiveness, the Contact Info widget's map iframe with 600⨉216 dimensions can the actual height to be much less. For example, in Twenty Seventeen the sidebar is 325px wide. This results in the iframe being 117px high, since 325:117 has the same aspect ratio as 600:216. To fix, this PR explicitly uses AMP's `fixed-height` layout for the `amp-iframe` so that the height will always be 216px with the width being responsive.

#### Testing instructions:

1. Activate Twenty Seventeen.
2. Install the AMP plugin and in the AMP settings switch to the paired mode. (You may also need to opt to hide the admin bar in AMP if there is too much CSS resulting.)
3. Add the Contact Info widget and click the checkbox to show the map (which requires a Google Maps API key).
4. Look at the frontend, and the non-AMP version should look like this:

> <img width="358" alt="screen shot 2019-02-28 at 11 17 01" src="https://user-images.githubusercontent.com/134745/53592773-d2660c80-3b4b-11e9-9c9a-2cfae4e434ae.png">

5. Switch to the AMP version (`?amp`) and in `master` it should look erroneously as this (with less vertical height):

> <img width="353" alt="screen shot 2019-02-28 at 11 17 12" src="https://user-images.githubusercontent.com/134745/53592873-148f4e00-3b4c-11e9-8935-50425af128e1.png">

6. Switch to this branch and in the AMP version it should look like the following, which is the same as the non-AMP version:

> <img width="356" alt="screen shot 2019-02-28 at 11 25 43" src="https://user-images.githubusercontent.com/134745/53592925-2a047800-3b4c-11e9-8e31-c98e2b32c49e.png">

The HTML markup for AMP should look like this (after ampproject/amp-wp#1913, without which the `iframe` inside the `noscript` is also converted to `amp-iframe`):

```html
<amp-iframe layout="fixed-height" width="auto" sandbox="allow-scripts allow-same-origin" height="216" frameborder="0"
            src="https://www.google.com/maps/embed/v1/place?q=3999+Mission+Boulevard%2CSan+Diego+CA+92109&amp;key=..."
            class="contact-map">
    <span placeholder>Loading map…</span>
    <noscript>
        <iframe width="600" height="216" frameborder="0"
                src="https://www.google.com/maps/embed/v1/place?q=3999+Mission+Boulevard%2CSan+Diego+CA+92109&amp;key=..."
                class="contact-map"></iframe>
    </noscript>
</amp-iframe>
```

#### Proposed changelog entry for your changes:

* Improve rendering of Contact Info widget map in AMP.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.