Open gravityrail opened 6 years ago
stale[bot]
commented
5 years ago This issue has been marked as stale. This happened because:
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
commented
5 years ago This should be fixed by https://github.com/Automattic/jetpack/pull/10945
westonruter
commented
5 years ago @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
commented
5 years ago My bad, you're right. I mis-read this in my eagerness to close stale issues.
westonruter
commented
5 years ago Also needing improved compatibility is the Comments module. See AMP validation errors that are generated here: https://github.com/ampproject/amp-wp/issues/1785#issuecomment-452551845
jeherve
commented
5 years ago 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
commented
5 years ago As long as these have noscript fallbacks then those can be used as the AMP version as well.
westonruter
commented
5 years ago 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
commented
5 years ago Just to link issues, for Related Posts see #9556.
jeherve
commented
5 years ago 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
commented
5 years ago The upcoming GIF block (scheduled for release tomorrow), for example, is very broken right now.
Opened #11318 to begin to address that.
swissspidy
commented
5 years ago FYI, element-level infinite scroll support is currently being worked on in AMP, see https://github.com/ampproject/amphtml/issues/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
commented
5 years ago 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
commented
5 years ago 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 https://github.com/Automattic/jetpack/pull/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 https://github.com/Automattic/jetpack/pull/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 https://github.com/Automattic/jetpack/pull/9458, https://github.com/Automattic/jetpack/pull/10945, https://github.com/Automattic/jetpack/pull/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 https://github.com/Automattic/jetpack/pull/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 https://github.com/Automattic/jetpack/pull/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 https://github.com/ampproject/amp-wp/issues/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 https://github.com/Automattic/jetpack/pull/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 (https://github.com/ampproject/amphtml/issues/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 https://github.com/Automattic/jetpack/pull/9458 and https://github.com/Automattic/jetpack/pull/10945. |
| Likes | 🚫🚫 | Currently short-circuited by https://github.com/Automattic/jetpack/pull/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 | See https://github.com/Automattic/jetpack/pull/13205 for fix to responsive-videos. |
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) | ✅ | Resolved by adding support for dev mode. See https://github.com/Automattic/jetpack/pull/13450. |
| Photon | ✅❔ | Compatible. Compat work done in https://github.com/Automattic/jetpack/pull/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 https://github.com/Automattic/jetpack/issues/9556#issuecomment-455718389 |
| 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 https://github.com/Automattic/jetpack/pull/9458). However, not implemented are: Pocket, Skype, Print, Reddit, Telegram, and Whatsapp. However, the style from the non-AMP version is not applied (see https://github.com/Automattic/jetpack/issues/9730#issuecomment-454599048). |
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 (https://github.com/ampproject/amp-wp/pull/1929) but Jetpack's shortcodes are done https://github.com/Automattic/jetpack/pull/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 https://github.com/Automattic/jetpack/pull/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 https://github.com/Automattic/jetpack/pull/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 https://github.com/Automattic/jetpack/pull/9458. The jetpack-helper.php in the AMP plugin should be able to be removed; see https://github.com/ampproject/amp-wp/pull/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 https://github.com/Automattic/jetpack/pull/10945 and https://github.com/Automattic/jetpack/pull/11088. For the feature to be used in AMP, will need to be re-written per description in https://github.com/Automattic/jetpack/pull/9458. |
jeherve
commented
5 years ago Thank you for that!
westonruter
commented
5 years ago I've finished the audit, except for the shortcode embeds ~and the premium modules~.
Update: Premium modules now included.
simison
commented
5 years ago 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
commented
5 years ago /cc @jessefriedman (Following up on our chat)
jeherve
commented
4 years ago This board aims to give us an overview of what needs to be done: https://github.com/Automattic/jetpack/projects/25
Master issues:
From @westonruter: