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

[DK-Stern]

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.

amp-toolbox/packages/optimizer/lib/parseSizes.js

Lines 61 to 64 in b367a2d

	const size = sizeString.split(/\)\s+/);
	if (size.length !== 2) {
	throw new Error(`Invalid sizes definition '${string}'`);
	}

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.

amp-toolbox/packages/optimizer/lib/transformers/ApplyCommonAttributes.js

Lines 206 to 214 in b367a2d

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

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.

amp-toolbox/packages/optimizer/lib/transformers/ApplyCommonAttributes.js

Lines 246 to 250 in b367a2d

	for (const node of this.nodesToTransform) {
	for (const attribute of Object.keys(this.attributeTransformations)) {
	delete node.attribs[attribute];
	}
	}

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: #1305

[sebastianbenz]

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

[DK-Stern]

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]

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]

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]

Just published 2.8.9. Thanks for the reminder :-)