SSR: Add support for media, sizes and heights attribute by sebastianbenz · Pull Request #763 · ampproject/amp-toolbox

sebastianbenz · 2020-05-11T22:00:11Z

This will make it possible to remove the boilerplate even if the media, sizes or heights attributes are used. These attributes will be transformed into CSS media queries instead.

sebastianbenz · 2020-05-11T22:39:24Z

@westonruter @schlessera PTAL - I haven't yet implemented the CSS size check.

westonruter

I reviewed what I could, but @schlessera is better suited to give more in-depth feedback.

westonruter · 2020-05-13T19:14:51Z

packages/optimizer/lib/transformers/ApplyCommonAttributes.js

+
+const parseSizes = require('../parseSizes');
+const {appendChild, createElement, insertText, hasAttribute} = require('../NodeUtils');
+const ID_PREFIX = 'i-amp-';


The prefix i-amphtml- is disallowed, but i-amp- is OK. It seems somewhat strange why i-amp- wasn't chosen rather for the internal prefix since it has fewer bytes.

packages/optimizer/lib/transformers/ApplyCommonAttributes.js

westonruter · 2020-05-13T19:30:17Z

packages/optimizer/lib/transformers/ApplyCommonAttributes.js

+      appendChild(head, customStyles);
+      insertText(customStyles, '');
+    }
+    customStyles.children[0].data += styles;


As noted in the description, we'll need to make sure this doesn't go above 75000 bytes.

packages/optimizer/lib/transformers/ApplyCommonAttributes.js

schlessera · 2020-05-15T13:30:32Z

packages/optimizer/lib/transformers/ApplyCommonAttributes.js

+
+  transform(node, id) {
+    // normalize whitespace
+    let mediaString = node.attribs.media.replace(/\s+/g, ' ');


Seems like this is already done within parseSizes() ?

parseSizes is unrelated to the media attribute or am I missing something?

Oh, nvm, I was thinking of the media conditions that are part of the sizes attribute.

packages/optimizer/lib/transformers/ApplyCommonAttributes.js

schlessera · 2020-05-15T13:57:04Z

packages/optimizer/lib/transformers/ApplyCommonAttributes.js

+   *
+   * @param {Node} node
+   */
+  getOrCreateId(node) {


This method doesn't check whether it is maybe creating duplicate IDs. This will break if it runs on markup that is already partially or fully optimized, or which happens to use the same prefix.

The PHP version will detect pre-existing IDs and iterate over them until it finds an ID that is not yet taken: https://github.com/ampproject/amp-wp/blob/63e99a63ca0624d5cff8f6e8d8d1ac85b89e337b/lib/common/src/Dom/Document.php#L1509-L1512

True. This is not straightforward to implement in our case unfortunately.

I'll ignore this case for now. Re-running the transformer would simply re-use the existing id. Pre-existing Ids should not be avoided by our prefix.

schlessera · 2020-05-15T14:05:19Z

...zer/spec/transformers/valid/ServerSideRendering/converts_heights_attribute_to_css/input.html

+    <noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
    <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;
 -moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes-amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes-amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes-amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes-amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style>
-    <noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>


Why the change in order here?

schlessera · 2020-05-15T14:06:00Z

.../transformers/valid/ServerSideRendering/converts_media_attribute_to_css/expected_output.html

+
+

Removal of the boilerplate shouldn't leave empty lines behind.

that's going to be removed by a different transformer (not used in the test)

This will be fixed by a different transformer

schlessera · 2020-05-15T14:14:10Z

packages/optimizer/lib/parseSizes.js

+ * See https://html.spec.whatwg.org/multipage/images.html#parsing-a-sizes-attribute
+ *
+ * @param {String} string
+ * @returns {!Sizes}


Why the "non-nullable" modifier ! here specifically? Wouldn't that be the default without the modifier anyways? It seems like that distinction isn't use anywhere else.

It's a closure compiler flag used by the AMP runtime. Removed it as we don't need it.

sebastianbenz · 2020-05-18T12:06:09Z

I've addressed the comments. In regards to checking for the CSS limit. I'm currently lending towards not including this check:

CSS optimization would need to be run twice (before, to be able to get the correct byte count and after, to optimize the generated CSS rules).
I'd prefer to fail on not being able to remove the boilerplate, the perf implications of not being able to remove the boilerplate because of this are a lot harder to detect otherwise.

sebastianbenz · 2020-05-18T12:06:27Z

@westonruter @schlessera PTAL

westonruter · 2020-05-19T22:44:00Z

In regards to checking for the CSS limit. I'm currently lending towards not including this check

Doesn't this run the risk of the Optimizer generating invalid AMP from valid AMP input? If the CSS is close to the limit and then optimization causes it to go over, that seems not ideal?

sebastianbenz

Optimizer changes CSS for the better and for the worse. This means you should run validation afterwards. I'm OK with failing validation if CSS impacts perf.

sebastianbenz · 2020-05-20T06:33:03Z

.../transformers/valid/ServerSideRendering/converts_media_attribute_to_css/expected_output.html

+
+

This will be fixed by a different transformer

sebastianbenz · 2020-05-20T06:33:18Z

packages/optimizer/lib/transformers/ApplyCommonAttributes.js

+   *
+   * @param {Node} node
+   */
+  getOrCreateId(node) {


sebastianbenz · 2020-05-28T17:30:43Z

Merging as there's no further feedback that needs to be addressed.

schlessera · 2020-05-29T10:06:12Z

@sebastianbenz There seems to be an issue with the CSS in here that was raised twice but always resolved without a comment, so I assume that GitHub resolved it because of an unrelated change: #763 (comment)

Or am I missing something maybe?

sebastianbenz · 2020-05-29T11:28:39Z

Hah! Not sure how this got lost. Fixed in #811

sebastianbenz · 2020-05-29T11:28:48Z

Thanks for picking up on this!

sebastianbenz added enhancement toolbox-optimizer labels May 11, 2020

googlebot added the cla: yes label May 11, 2020

sebastianbenz force-pushed the optimizer-media branch from 6546d5f to 7065ef1 Compare May 11, 2020 22:35

sebastianbenz marked this pull request as ready for review May 11, 2020 22:37

sebastianbenz requested a review from westonruter May 11, 2020 22:38

westonruter requested changes May 13, 2020

View reviewed changes

sebastianbenz force-pushed the optimizer-media branch from 7fd8734 to 7065ef1 Compare May 14, 2020 17:58

schlessera suggested changes May 15, 2020

View reviewed changes

sebastianbenz added 4 commits May 15, 2020 22:04

ssr: add support for media attribute

c809c8b

ssr: add support for sizes and heights attribute

07d09d2

Fix jsdoc and whitespace handling

ba45798

revert accidently changed test input

edc3e7b

Detect duplicate ids

50b0bdc

sebastianbenz force-pushed the optimizer-media branch from 6636719 to 50b0bdc Compare May 20, 2020 06:35

sebastianbenz commented May 20, 2020

View reviewed changes

sebastianbenz merged commit 410be25 into master May 28, 2020

sebastianbenz deleted the optimizer-media branch May 28, 2020 17:30

This was referenced May 29, 2020

Failing tests in ampproject/optimizer library ampproject/amp-wp#4798

Closed

Fix failing Optimizer spec tests with ID generation ampproject/amp-wp#4799

Merged

Conversation

sebastianbenz commented May 11, 2020

Uh oh!

sebastianbenz commented May 11, 2020

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

schlessera May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastianbenz commented May 18, 2020

Uh oh!

sebastianbenz commented May 18, 2020

Uh oh!

westonruter commented May 19, 2020

Uh oh!

sebastianbenz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastianbenz commented May 28, 2020

Uh oh!

schlessera commented May 29, 2020

Uh oh!

sebastianbenz commented May 29, 2020

Uh oh!

sebastianbenz commented May 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

schlessera May 15, 2020 •

edited

Loading