AMP package

SantosGuillamot · 5 March 2021 09:29

Not really Is there any “mode”, apart from AMP, that you have in mind?

David · 7 March 2021 18:06

I’ve finished making the @frontity/comscore package AMP-aware. The PR is ready to review.

SantosGuillamot · 23 March 2021 10:35

I’ve been testing the current version of the package against test.frontity.org and it works great so far. In that site we have a lot of dummy content prepared to test WordPress themes, so I think that if the package is valid for that content, it will cover most of the use cases. Moreover, the package is really easy to use. Congrats everyone

This how I validated the urls, in case it’s useful for someone:

How to validate your urls

In order to test it against test.frontity.org, I came up with a way of validating the content that could be useful for other users validating their webs. I tested it locally with a new Frontity project using mars-theme. This is what I did:

1. Install `@frontity/amp` package

In order to test the package I first installed it and add it at the top of the frontity.settings.js file. Apart from that, I created two sites: one for AMP urls and other for not AMP urls. This is how it looks like:

const settings = [
  {
    name: "not-amp",
    state: {
      frontity: {
        url: "http://localhost:3000",
        title: "Test Frontity Blog",
        description: "WordPress installation for Frontity development",
      },
    },
    packages: [
      {
        name: "@frontity/mars-theme",
        state: {
          theme: {
            menu: [
              ["Home", "/"],
              ["Nature", "/category/nature/"],
              ["Travel", "/category/travel/"],
              ["Japan", "/tag/japan/"],
              ["About Us", "/about-us/"],
            ],
            featured: {
              showOnList: true,
              showOnPost: true,
            },
          },
        },
      },
      {
        name: "@frontity/wp-source",
        state: {
          source: {
            url: "https://test.frontity.org",
          },
        },
      },
      "@frontity/tiny-router",
      "@frontity/html2react",
    ],
  },
  {
    name: "amp",
    match: ["\\/amp\\/?($|\\?|#)"],
    state: {
      frontity: {
        url: "http://localhost:3000",
        title: "Test Frontity Blog",
        description: "WordPress installation for Frontity development",
      },
    },
    packages: [
      "@frontity/amp",
      {
        name: "@frontity/mars-theme",
        state: {
          theme: {
            menu: [
              ["Home", "/"],
              ["Nature", "/category/nature/"],
              ["Travel", "/category/travel/"],
              ["Japan", "/tag/japan/"],
              ["About Us", "/about-us/"],
            ],
            featured: {
              showOnList: true,
              showOnPost: true,
            },
          },
        },
      },
      {
        name: "@frontity/wp-source",
        state: {
          source: {
            url: "https://test.frontity.org",
          },
        },
      },
      "@frontity/tiny-router",
      "@frontity/html2react",
    ],
  },
];

export default settings;

2. Make `mars-theme` AMP-aware

Right now, mars-theme is not AMP-aware, so I had to do some minor modifications before validating the content of the posts and pages. This is not official, just a workaround to make it work. These are the changes I made:

Remove !important from the post css, because it isn’t allowed in AMP.
Add canonical link to the Head. In the index.js file I added this line:

<link rel="canonical" href={state.frontity.url + data.link} />

Pass Featured Image through html. I was having some issues with our Image component so finally I change the <FeaturedMedia /> component and used html2react. I changed these lines for these ones:

<Html2React
    html={`<img
    alt="${media.title.rendered}"
    src="${media.source_url}"
    srcSet="${srcset}"
    width="${media.media_details.width}"
    height="${media.media_details.height}"
    layout="responsive"
    />`}
/>

3. Run AMP validator in bulk through our sitemap.xml

In AMP, it’s easy to validate one url with tools like the official AMP validator, but going one by one is a pain, so I looked for a way to run the whole sitemap. This is the best I could find:

3.1 Start the Frontity project in localhost

Once I made the changes needed for mars-theme, I started it in localhost:3000, and just by visiting .../amp/ urls, Frontity loads the AMP version of the page.

3.2 Install amphtml-validator library in the project

The AMP team is mainting amphtml-library, and it allows you to validate urls in the CLI. This means that, after installing the package with npm install amphtml-validator, if you run the following command, you get the result of the url (Pass or the proper error):

npx amphtml-validator http://localhost:3000/my-post/amp/

3.3 Add sitemap to our project

In order to go through all the urls of test.frontity.org, I create a new json file at the root of the project, sitemap.json, to easily iterate through the urls. For doing this I followed these steps:

Go to view-source:https://test.frontity.org/post-sitemap.xml and copy the XML. Here we are taking the posts, but this way is easy to change between the page-sitemap or the category-sitemap.
Go to XMLtoJSON tool and paste the XML. I had to remove the first commented line.
Copy the JSON and paste it in the sitemap.json file of our project.
Search and replace the url (https://test.frontity.org in this case) for http://localhost:3000.

3.4 Create a script to run urls in bulk

Now that I was able run that command in our project and I had the sitemap in JSON format, it was a matter of running it for all the urls. For that, I created a validator.js file in the root of the project, which looks like this:

const util = require("util");
const exec = util.promisify(require("child_process").exec);
const sitemap = require("./sitemap.json");

const urls = sitemap.urlset.url;
let urlsPass = 0;
let urlsError = 0;
let listErrorUrls = [];

const validate = async () => {
  for (const url of urls) {
    try {
      const { stdout } = await exec(
        "npx amphtml-validator " + url.loc + "amp/"
      );
      if (stdout) {
        urlsPass++;
        console.log(stdout);
      }
    } catch (err) {
      urlsError++;
      listErrorUrls.push(url.loc);
      console.log(err.stderr);
    }
  }
  console.log("Urls pass: " + urlsPass);
  console.log("Urls error: " + urlsError);
  console.log("List of error urls: " + listErrorUrls);
};

validate();

Once you have this script, you can run node validator.js and it will iterate through the array of urls defined in the sitemap. Depending on the sitemap format the logic may change a bit, but it should be pretty similar.

Of course, I just added some basic logic to console.log the results, but it could be adapted as well. Maybe you just want the urls that fail for example.

Once you have the list of urls failing, you can use the console or go to https://validator.ampproject.org/ to check them one by one.

SantosGuillamot · 23 March 2021 13:53

Based on the validation I have just explained, I came up with some questions/feedback. I’ve been testing some pieces of code, so after your feedback I can make a Pull Request if necessary.

Image

Most of the errors in the urls were related to images. As we are using old content, in several posts they don’t show and these are the errors returned:

The mandatory attribute ‘height’ is missing in tag ‘amp-img’.
The “height” attribute is missing.

In amp-img, we have to define an explicit size (as in width / height ) in advance, so that the aspect ratio can be known without fetching the image. Although in new posts it will probably be added by WordPress/Gutenberg by default, there will be a lot of old posts with it. I guess this is related to this other discussion we were having to avoid the layout shift.

Should we add a default width/height ratio or keep the errors to let the users know they should change that?

Apart from that, we might need to make our components AMP-aware because we are using it for the featured image in our themes for example. I had to add a workaround to make it AMP valid.

Iframe

I found some errors in the iframes in some urls:

The iframes are not loading.
Invalid value ‘840’ for attribute ‘width’ in tag ‘amp-iframe’ - for layout ‘FIXED_HEIGHT’, set the attribute ‘width’ to value ‘auto’.
The attribute ‘allowfullscreen’ in tag ‘amp-iframe’ is set to the invalid value ‘true’.

It seems that, to solve the second issue we just have width="auto", and to solve the third one allowFullScreen={undefined}.

Apart from that, to be able to run the scripts of the iframes (the reason why they are not showing), it seems we have to add sandbox attribute.

I’ve tested the following code in the amp-iframe processor and seems to solve all the issues:

<amp-iframe
    {...rest}
    title={title || ""}
    src={src}
    layout="fixed-height"
    width="auto"
    allowFullScreen={undefined}
    sandbox="allow-scripts allow-same-origin"
    height={parseInt(height, 10) || 150} // This is mimicking the browser default of 150px
/>

What do you think?

Moreover, I’ve seen that sometimes we’re sending some attributes that aren’t expected:

The attribute ‘marginheight’ may not appear in tag ‘amp-iframe’.
The attribute ‘marginwidth’ may not appear in tag ‘amp-iframe’.
The attribute ‘security’ may not appear in tag ‘amp-iframe’.

Should we delete them? Or add some logic to delete all the attributes that don’t match the allowed attributes: iframe-attributes and common-attributes?

Video

These are the current errors related to videos:

The attribute ‘autoplay’ in tag ‘amp-video’ is set to the invalid value ‘true’.
The attribute ‘loop’ in tag ‘amp-video’ is set to the invalid value ‘true’.
The attribute ‘muted’ in tag ‘amp-video’ is set to the invalid value ‘true’.
The attribute ‘controls’ in tag ‘amp-video’ is set to the invalid value ‘true’.

It seems that right now we are getting the attribute autoPlay (for example), from the REST API. But after processing it in AMP we are returning autoPlay="true", which it isn’t valid. It should be just autoPlay. It’s the same case for all the issues.

I’ve checked the code and we are just using ...props. Any idea how to solve this?

Audio

Invalid URL protocol ‘http:’ for attribute ‘src’ in tag ‘source’.

I think this is because I was working on localhost. This should be covered by this piece of code right?

Classnames

Error in classnames for videos, audios and iframes:

The attribute ‘classname’ may not appear in tag ‘amp-iframe’.
The attribute ‘classname’ may not appear in tag ‘amp-video’.
The attribute ‘classname’ may not appear in tag ‘amp-audio’.

This seems to be caused when we are creating a new component and we are using {...props} to send the attributes. I’ve seen that specifying the class the problem is solved.

Instead of this way we are using right now:

<amp-audio {...props} controls />

Use this:

<amp-audio {...props} class={className} controls />

We are doing the same in html2react. I’ve tested it and it works. What do you think?

Svg

In some posts I encountered this error:

The tag ‘g’ may only appear as a descendant of tag ‘svg’.

Not sure why we are using the <g> tag for some inline text in a couple of posts. Any idea how if this is common or not expected in a real site?

Form

In the posts with forms it returns this error:

The attribute ‘action’ may not appear in tag ‘form’.
The tag ‘form’ requires including the ‘amp-form’ extension JavaScript.

I guess this should be managed by the forms packages like contact form 7? Or do you think we should add a processor for this?

Navigation

I’ve realized that when you arrive to an amp url, an archive or a post, all the links are pointing to the non-AMP version, so when clicking on them, it stops using AMP.

I guess that with the link processor we could detect if AMP is active and try to reach the amp version first.

I’ve seen some AMP webs doing this an other ones just loading the non-AMP version. Any opinion about what we should do about this?

mmczaplinski · 7 April 2021 00:22

Hey @SantosGuillamot let me reply to your concerns here and thanks for testing this thorougly!

I’ve been looking for some ideas around this and I noticed this example in the AMP documentation: Example: How to support Images with unknown Dimensions. I think that we can use this approach: If the image does have width & height: great, use that to get the aspect ratio and we’re done. If not, use the layout="fill" combined with object-fit (described in the article).

Yep, the issue is that for attributes that are booleans, AMP does not like when you pass an actual value like allowFullScreen={true} even though it’s permitted in the HTML spec. So we have to resort to passing allowFullScreen={undefined} in those cases which ends up as just allowFullScreen in the HTML (a boolean attribute with no value).

I’m not sure why width="auto" is required. I don’t think I have seen that issue before and I’m sure I’ve tested the iframes with the layout="fixed-height". But if that works then yes, I think we can do that!

SantosGuillamot:

<amp-iframe
    {...rest}
    title={title || ""}
    src={src}
    layout="fixed-height"
    width="auto"
    allowFullScreen={undefined}
    sandbox="allow-scripts allow-same-origin"
    height={parseInt(height, 10) || 150} // This is mimicking the browser default of 150px
/>

Yup, although I think that it’s important that we don’t absolutely override any prop here and just provide the “working” values as defaults that the user can override (even if the user-provided value could lead to AMP problems). So, I think the code should be more like:

<amp-iframe
    {...rest}
    title={title || ""}
    src={src}
    layout="fixed-height"
    width="auto"
    allowFullScreen={props.allowFullScreen || undefined}
    sandbox={props.sandbox || "allow-scripts allow-same-origin"}
    height={parseInt(height, 10) || 150} // This is mimicking the browser default of 150px
/>

My opinion is that we shouldn’t do anything here because there is no good solution . If we use a blocklist of attributes then we cannot possible cover all “disallowed” attributes. the blocklist would have to be infinite! Conversely, if we use an allowlist then we might delete some attributes that a user needs to put on the element. For example, someone might want to put some data-* attributes or some js-* attributes to use as selectors in e2e tests.

Plus this “validation error” does not actually stop the AMP page from rendering!

FYI, in general, my approach when creating the processors was to not try to be “smarter” than the user if they provide invalid content because there is an almost infinite number of ways in which content can be invalid (according to AMP rules) and trying to catch some cases can cause more problems than solve

So this is the same problem as for the allowFullscreen prop for the amp-iframe (and elsewhere).

If the prop is a boolean (and this only works for booleans) I think that we could solve this with something like:

<amp-video
    autoplay={autoplay ? undefined : null}
/>

but I have not tested it.

I can’t say for sure why that is happening. Yes, the url should have been substituted as you noticed.

Yep, this is bug and your solution should work. It’s because AMP elements are custom components and they don’t accept the classname attribute!

Also no idea!

My opinion is that a package like contact form 7 should add that functionality to the processors for <form> in that case because forms do not just work out of the box with Frontity anyway. The user has to install some kind of a plugin in WordPress in order to create forms and each plugin works differently anyway.

Although that means that every “form” package would have to include some code to explicitly opt-in to being able to handle AMP. Which is not that ideal either, but I don’t think we can provide a one-size-fits-all solution that is guaranteed to work with any Frontity form package so I think it’s better to leave it up to the individual package.

Yeah, I think that we could/should do that. If state.frontity.mode === "amp" (PS I think we still have not settled on whether the name should be mode or something else) we can try to redirect to the AMP version of the page.

OK, in order to unblock this would be great to get to the bottom of the issue with SVG and the http error in amp-audio. Could you make a reproduction like a test case or a video?

All the other issues I think are clear and actionable.

@luisherranz

We should also decide on the name for the state.frontity.mode. As mode is already used to refer to embedded mode or decoupled mode and state.frontity.rendering is "ssr" or "csr" the next best thing to me seems to be state.frontity.renderingMode (I know, it’s a bit of a mouthful )

SantosGuillamot · 7 April 2021 07:27

Thanks a lot for taking a look at it Michal I’ve just opened a bug to track this on GitHub and I assigned it to you. If at some point you feel it makes sense to open different bugs for each error just let me know,

Yes, it makes sense. Moreover, I think the attributes I listed (marginheight, marginwidth and security) are obsolete and they shouldn’t be used, so I guess it’s a problem in the content. If, for any reason, users want to use some attributes in the non-AMP version but exclude them in the AMP version because they aren’t valid, they could just create a new processor for that right?

Regarding the svg: I think it’s just an error in the content because we’re using something old and I guess that should be changed in WordPress if users encounter the same issue. This is the post that is returning the error and this is an example of the non-valid HTML:

<div class="wp-block-cover__inner-container">
  <p class="has-text-align-center has-large-font-size">
    This is a
    <g
      class="gr_ gr_3 gr-alert gr_spell gr_inline_cards gr_run_anim ContextualSpelling multiReplace"
      id="3"
      data-gr-id="3"
      >full width</g
    >
    cover image with fixed background.
  </p>
</div>

Regarding the amp-audio: I was wrong and the error isn’t caused because I was working on localhost. In this page, we aren’t passing the src to the audio attribute but to its children source element, so that’s why this code is not being applied. I guess we could apply the same logic for the source element right? This is an example of the html of that page:

<audio
  class="wp-audio-shortcode"
  id="audio-211-1"
  preload="none"
  style="width: 100%"
  controls="controls"
>
  <source
    type="audio/mpeg"
    src="http://test.frontity.org/wp-content/uploads/2017/06/Peter-Griffin-TADA.mp3?_=1"
  />
  <a
    href="http://test.frontity.org/wp-content/uploads/2017/06/Peter-Griffin-TADA.mp3"
    >http://test.frontity.org/wp-content/uploads/2017/06/Peter-Griffin-TADA.mp3</a
  >
</audio>

luisherranz · 7 April 2021 14:39

Hey, great work here!!

I’ve never seen a publisher that prefers to keep their users in the AMP version more than what is strictly necessary.

I think that even the AMP team doesn’t expect that and encourages to go from AMP → PWA: Preload your PWA from your AMP pages

So my opinion is not to worry about this for now and leave it as it is. If someone requests it, a setting can be added in the future

Ok, let’s make a decision

We have used state.frontity.options.embedded = true for the Embedded mode, so I guess using state.frontity.mode = "amp" is less confusing now.

Let’s go with that option, which seems like everyone liked at the beginning.

Also, if I am not mistaken, defining mode in your frontity.settings.js file was already working to switch the entrypoint to whatever name you pass. Using amp means it will check for src/amp.js, src/amp/index.js, src/amp/server.js and so on before checking for src/index.js, so we get that for free

The only difference with this method is that we will need to add an extra instruction to the docs: Add mode: "amp" to your frontity.settings.js file:

const settings = [
  {
    name: "main-site",
    packages: [
      /*...*/
    ],
  },
  {
    name: "amp-site",
    mode: "amp",
    match: "\\/amp",
    packages: [
      "@frontity/amp",
      /*...*/
    ],
  },
];

EDIT: I think that no PR is needed for this. Only start using the already implemented mode of the frontity.settings.js file.

mmczaplinski · 7 April 2021 17:58

Yup, they could do that. I think we could even recommend that (with an example) in our docs as this might be a relatively common thing to todo.

yep, looks this is not a bug on our side.

oh, that’s right. I didn’t take into account that <audio> elements can also have child <source> elements. Actually, the same is true for <video> so we will have to add the same feature to the amp-video processor. Thanks!

oh, okay great, I didn’t realize this

About the mode, I’ve also remembered that we’re already using a property called mode to differentiate between the production and development modes of webpack

But maybe that’s not a big deal as this is not something that you pass to the frontity state. What do you think @luisherranz ?

luisherranz · 9 April 2021 10:24

Good point, although I’d still go with state.frontity.mode which is already working as we want

mmczaplinski · 20 April 2021 22:58

@SantosGuillamot our Image component from @frontity/components would need some small changes in order to be compatible with AMP but I notice that it’s not currently on the roadmap.

Do you think it’s something we should also work on?

SantosGuillamot · 21 April 2021 07:16

You’re totally right. I’ve created a new issue to make Frontity components AMP-aware. Thanks Michal