search

ampproject / amp-toolbox

A collection of AMP tools making it easier to publish and host AMP pages.
Apache License 2.0
449 stars 243 forks source link

SSR: Transforming of nodes with attribute 'heights' fails, if value contains multiple media queries #1304

Open DK-Stern opened 2 years ago

DK-Stern commented 2 years ago

Issue

A node fails transforming, if it has 'heights'-attribute which contains more than one media query. Also the attribute got removed on rendering, if it had failed on transforming.

For example

<amp-carousel
          width="1"
          height="1"
          heights="(min-width:768px) 56.25%, (min-width:576px) and (max-width:767px) 100%, 133%"
          layout="responsive"
          type="slides"
          role="region"
        >
[...]
</amp-carousel>

Will be rendered by amp framework, without any validation errors and with correct media queries. But on SSR with amp optimizer i get following error and the 'heights'-attribute will be removed on rendered amp page:

AMP Optimizer ServerSideRendering Cannot remove boilerplate. Failed transforming heights="(min-width:768px) 56.25%, (min-width:576px) and (max-width:767px) 100%, 133%". Error: Invalid sizes definition '(min-width:768px) 56.25%, (min-width:576px) and (max-width:767px) 100%, 133%'
    at parseSizes (/.../node_modules/@ampproject/toolbox-optimizer/lib/parseSizes.js:63:15)
    at HeightsTransformer.transform (/.../node_modules/@ampproject/toolbox-optimizer/lib/transformers/ApplyCommonAttributes.js:127:21)
    at ApplyCommonAttributes.apply (/.../node_modules/@ampproject/toolbox-optimizer/lib/transformers/ApplyCommonAttributes.js:207:76)
    at ServerSideRendering.transform (/.../node_modules/@ampproject/toolbox-optimizer/lib/transformers/ServerSideRendering.js:105:27)
    at /.../node_modules/@ampproject/toolbox-optimizer/lib/DomTransformer.js:208:61
    at DomTransformer.doProfile (/.../node_modules/@ampproject/toolbox-optimizer/lib/DomTransformer.js:188:14)
    at DomTransformer.transformTree (/.../node_modules/@ampproject/toolbox-optimizer/lib/DomTransformer.js:208:18)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async DomTransformer.transform (/.../node_modules/@ampproject/toolbox-optimizer/lib/DomTransformer.js:178:7)
    at async DomTransformer.transformHtml (/.../node_modules/@ampproject/toolbox-optimizer/lib/DomTransformer.js:183:12)

Reason

parseSizes(string) throws an error for strings which has more than one ')' and trailing characters.

https://github.com/ampproject/amp-toolbox/blob/b367a2db78e1934e5c3e72727777e7c29d6a3b35/packages/optimizer/lib/parseSizes.js#L61-L64

When function transform(node, id) of HeightsTransformer gets called on nodes with 'height'-attributes, which contain a value with multiple media queries, parseSizes(string) will throw an error and ApplyCommonAttributes will catch that error and logs them.

https://github.com/ampproject/amp-toolbox/blob/b367a2db78e1934e5c3e72727777e7c29d6a3b35/packages/optimizer/lib/transformers/ApplyCommonAttributes.js#L206-L214

In function applyToCustomStyles(head, customStyles) of ApplyCommonAttributes the attributes 'heights', 'media' and 'sizes' will be removed on nodes which are in array nodesToTransform and my own value in 'heights'-attribute (which could not be transformed) will be removed. So the result is, the rendered amp component has no 'heights' attribute at all.

https://github.com/ampproject/amp-toolbox/blob/b367a2db78e1934e5c3e72727777e7c29d6a3b35/packages/optimizer/lib/transformers/ApplyCommonAttributes.js#L246-L250

Solutions

First possible solution

One possible solution would be, that nodes which could not be transformed should be removed from array nodesToTransform, so the values could stay if transformation fails.

This could be done with following code:

this.nodesToTransform = this.nodesToTransform.filter(
  (nodeToTransform) => nodeToTransform !== node
);

In context it would look like that:

try {
  nodeHasBeenTransformed = nodeHasBeenTransformed || transformer.transform(node, id);
} catch (e) {
  this.log.debug(
    `Cannot remove boilerplate. Failed transforming ${attribute}="${node.attribs[attribute]}".`,
    e
  );
  this.nodesToTransform = this.nodesToTransform.filter(
    (nodeToTransform) => nodeToTransform !== node
  );
  this.canRemoveBoilerplate = false;
}

Second possible solution

An other solution would be, that function parseSize(string) would accept all values which amp framework does. Maybe for the second solution it would make sense, to ignore all nodes where parseSizes(string) could not parse values, too.

PR

I've created a PR with a fix described in first solution: https://github.com/ampproject/amp-toolbox/pull/1305

sebastianbenz commented 2 years ago

Thanks for the detailed write up! We should also fix the underlying issue of media condition parsing not being properly implemented.

DK-Stern commented 2 years ago

You're welcome! To fix the underlaying issue would be great, but it i think could be really hard to find a solution which interprets all possible media queries and transforms them to valid css. Is there any code from amp framework we could reuse?

sebastianbenz commented 2 years ago

Not that I'm aware of. There is https://github.com/albell/parse-sizes as a lightweight standalone solution (haven't tested it). If the sizes attribute is valid, we shouldn't have a problem transforming the attribute as we can use the whole media query. We need to be able to tell though if the sizes attribute is valid.

DK-Stern commented 2 years ago

Ok that sound great. 👍

Beside that, would it be possible to make a small release (e.g. v2.8.9) with the two bug fixes #1305 and #1303? In my current project many amp sites are broken, because of that bugs. 😅

sebastianbenz commented 2 years ago

Just published 2.8.9. Thanks for the reminder :-)