Add script tag support for enclosing a template #19346

alabiaga · 2018-11-15T19:46:04Z

fixes #19321, #11205

allow defining templates with a script tag. This allows text nodes used for amp-mustache templating not to be hoisted, destroying templating. See amp-mustache: Mustache tags don't work in tables #11205.
revamp test-amp-mustache to run test suite against template creation with supported types 'script' and 'template'.
add example of script type template in amp-list.amp.html example.
add TODO to handle nested script templates. I omitted this for now. I'll address in a following PR. This should fine as the validator won't allow for this change until I make the validator changes.

alabiaga · 2018-11-19T21:40:14Z

jridgewell · 2018-11-20T18:45:28Z

extensions/amp-mustache/0.1/amp-mustache.js

+      this.template_ = container./*OK*/innerHTML;
+    } else if (this.element.tagName == 'SCRIPT') {
+      //TODO(alabiaga): handle nested templates.
+      this.template_ = this.element.text.trim();


textContent

yes that is the data type but it is the template. I'd like to keep a name that can be used for both cases when either template is defined via tag or <script> tag. In both cases, it is still just a string representing the template.

I'd like to keep the name as I reuse it for the 'TEMPLATE' type use case as well. The data type is text but for both cases this.template_ is a string representing the template contents. Would this.templateString_ be a better name?

jridgewell · 2018-11-20T18:48:03Z

extensions/amp-mustache/0.1/amp-mustache.js

+      this.template_ = container./*OK*/innerHTML;
+    } else if (this.element.tagName == 'SCRIPT') {
+      //TODO(alabiaga): handle nested templates.
+      this.template_ = this.element.text.trim();


We don't trim in the <template> case.

right. mustache parsing won't care about the spaces anyway. removed in the script case.

right. removed. mustache parse will ignore anyway

jridgewell · 2018-11-20T18:48:44Z

extensions/amp-mustache/0.2/amp-mustache.js

+   *
+   * @param {!Element} content
+   */
+  processNestedScriptTemplates_(content) {


Why are we duplicating?

Initially, what was written v0.2 was already the same logic to what was in amp-mustache v0.1 and inlined and not just extracted into its own method. I just pulled it out to its own method. I will pull it out and the template versions can share the logic once I add support for nested templates in the next PR.

Hm, it's the diff that's screwed up. It's not showing me the old code.

This function isn't called anywhere.

removed. Thanks, initially was going to try and add support for nested script templates but deferring it as it's not a priority and I filed a ticket for it as mentioned in another review comment.

jridgewell · 2018-11-20T18:49:06Z

src/service/template-impl.js

-        'Template element must be a "template" tag %s', templateElement);
+    const templateTagName = templateElement.tagName;
+    user().assert(
+        templateTagName == 'TEMPLATE' || templateTagName == 'SCRIPT',


Assert script has type="template"

the template tag also has a type that is enforced by the validator but isn't in the code base.

e.g. <template type="amp-mustache>..</template>

Maybe it is enough to do that there and the script way of defining templates is going to follow the same model. I will do the validation changes in a separate PR.

<script type="template"> will need to be validated the same way a <template>. But, without this runtime check, I can pass a selector that will be a <script type="text/json">, which won't be validated. That would be a security issue.

You can pass in json data with the following markup <script type="text/json"> but the sanitizer which runs during mustache rendering will strip out unsafe markup, script tags included. But to be on the safe side, you're right we should validate the type. Though I was pointing out that during runtime..the type for template is not validated, which it is in the validator. I will not add this change, as this will probably break clients who currently do not set this attribute.

Nested templates via <script type="template/amp-mustache"> will be whitelisted and allowed in the sanitizer in my next PR when I add support for nested script tags.

Adding validation for checking <script type="template/amp-mustache"> rather than just <script type="template"> to be more in line with the template syntax we support, . Thanks

We should use a valid MIME type string to avoid future collisions, e.g. text/plain. We can use a non-standard attribute template="amp-mustache" to denote template syntax.

https://html.spec.whatwg.org/multipage/scripting.html#attr-script-type

jridgewell · 2018-11-20T18:50:05Z

src/service/template-impl.js

@@ -285,7 +288,9 @@ export class Templates {
    } else if (opt_querySelector) {
      return scopedQuerySelector(parent, opt_querySelector);
    } else {
-      return childElementByTag(parent, 'template');
+      const template = childElementByTag(parent, 'template');
+      const templateByScript = childElementByTag(parent, 'script');


Use a query selector to do this in one pass.

jridgewell · 2018-11-20T18:50:21Z

testing/describes.js

+ * @param {!Array} values
+ * @param {func} func
+ */
+export function testCases(values, func) {


I use this in the tests (test-amp-mustache.js) so that I can reuse all the tests and just run them using a template creation with either in one test run and then <script> in the next.
e.g.

`testCases(['script', 'template'], templateType => { beforeEach( document.createElement(templateType); ) })`

Did you try using describes.repeated?

I did not. I was looking and briefly mentioned this in a standup and was under the impression that it didn't exist. Changed! Thanks.

alabiaga

thanks Justin.

alabiaga · 2018-11-20T20:16:49Z

extensions/amp-mustache/0.1/amp-mustache.js

+      this.template_ = container./*OK*/innerHTML;
+    } else if (this.element.tagName == 'SCRIPT') {
+      //TODO(alabiaga): handle nested templates.
+      this.template_ = this.element.text.trim();


I'd like to keep the name as I reuse it for the 'TEMPLATE' type use case as well. The data type is text but for both cases this.template_ is a string representing the template contents. Would this.templateString_ be a better name?

alabiaga · 2018-11-20T20:17:06Z

extensions/amp-mustache/0.1/amp-mustache.js

+      this.template_ = container./*OK*/innerHTML;
+    } else if (this.element.tagName == 'SCRIPT') {
+      //TODO(alabiaga): handle nested templates.
+      this.template_ = this.element.text.trim();


right. removed. mustache parse will ignore anyway

jridgewell · 2018-11-20T21:35:07Z

extensions/amp-mustache/0.1/amp-mustache.js

+      this.template_ = container./*OK*/innerHTML;
+    } else if (this.element.tagName == 'SCRIPT') {
+      //TODO(alabiaga): handle nested templates.
+      this.template_ = this.element.text;


No, use element.textContent instead of .text. 😛

ah..Wish there was support for highlighting the exact code you are referring rather than just the line. Done. Thanks.

jridgewell · 2018-11-20T21:36:35Z

extensions/amp-mustache/0.1/test/test-amp-mustache.js

-  });
+  // This test suite runs twice. First by creating templates of type template
+  // and then by creating templates encapsulated within script.
+  testCases(['script', 'template'], templateType => {


You're looking for describes.repeated:

describes.repeated('toggle', { 'name': data }, (name, data) => { });

jridgewell · 2018-11-20T21:40:24Z

extensions/amp-mustache/0.2/amp-mustache.js

+   *
+   * @param {!Element} content
+   */
+  processNestedScriptTemplates_(content) {


Hm, it's the diff that's screwed up. It's not showing me the old code.

jridgewell · 2018-11-20T21:42:16Z

src/service/template-impl.js

-        'Template element must be a "template" tag %s', templateElement);
+    const templateTagName = templateElement.tagName;
+    user().assert(
+        templateTagName == 'TEMPLATE' || templateTagName == 'SCRIPT',


<script type="template"> will need to be validated the same way a <template>. But, without this runtime check, I can pass a selector that will be a <script type="text/json">, which won't be validated. That would be a security issue.

dreamofabear · 2018-11-26T14:04:37Z

extensions/amp-mustache/0.2/amp-mustache.js

+      container.appendChild(content);
+      this.template_ = container./*OK*/innerHTML;
+    } else if (this.element.tagName == 'SCRIPT') {
+      //TODO(alabiaga): handle nested templates.


This is lower priority, remove and file a tracking issue. No need to address yet. :)

done filed #19450

alabiaga · 2018-11-28T22:21:22Z

@choumx friendly ping since justin is away.

extensions/amp-mustache/0.1/amp-mustache.js

dreamofabear · 2018-11-29T22:52:55Z

extensions/amp-mustache/0.2/amp-mustache.js

+   *
+   * @param {!Element} content
+   */
+  processNestedScriptTemplates_(content) {


This function isn't called anywhere.

dreamofabear · 2018-11-29T23:02:37Z

src/service/template-impl.js

+    const templateTagName = templateElement.tagName;
+    user().assert(
+        templateTagName == 'TEMPLATE' || templateTagName == 'SCRIPT',
+        'Template must be a "template" tag or defined in a "script" tag %s',


"Template must defined in a <template> or <script type=text/plain> tag."

dreamofabear · 2018-11-29T23:03:57Z

src/service/template-impl.js

+          'Script template attribute must be "text/plain"');
+      user().assert(
+          templateElement.getAttribute('template') == 'amp-mustache',
+          'Script template attribute must be "amp-mustache"');


We shouldn't couple template-impl.js to amp-mustache. Mustache is just a flavor of template.

right. removed. In the future we can just add support for it when other template types are supported and just default to amp-mustache if omitted perhaps.

dreamofabear · 2018-11-29T23:04:38Z

src/service/template-impl.js

+    if (templateTagName == 'SCRIPT') {
+      user().assert(
+          templateElement.getAttribute('type') == 'text/plain',
+          'Script template attribute must be "text/plain"');


Just include this in the expression above. Also, what about case sensitivity?

done. tagName in DOM trees always return in uppercase. https://developer.mozilla.org/en-US/docs/Web/API/Element/tagName

dreamofabear · 2018-11-29T23:05:06Z

testing/describes.js

+ * @param {!Array} values
+ * @param {func} func
+ */
+export function testCases(values, func) {


Did you try using describes.repeated?

dreamofabear · 2018-11-29T23:07:04Z

examples/amp-list.amp.html

@@ -57,6 +57,24 @@
    </template>
  </amp-list>

+  <!-- ## Using a script to enclose the template. -->
+  <amp-list width="auto" height="100" layout="fixed-height" src="comments.example.json">
+    <script type="amp-mustache">


type="text/plain" template="amp-mustache", right? Did we test that this example works?

Let's also add an integration test to extensions/amp-list/0.1/test/integration/test-amp-list.js to make sure it doesn't stop working. :)

yes, I did test this but this was before the whole discussion about the changes to these attributes so yes this stopped working with the recent changes. Thanks.

made test changes.

dreamofabear · 2018-11-29T23:18:56Z

src/service/template-impl.js

@@ -285,7 +296,7 @@ export class Templates {
    } else if (opt_querySelector) {
      return scopedQuerySelector(parent, opt_querySelector);
    } else {
-      return childElementByTag(parent, 'template');
+      return parent.querySelector('template, script');


Why this change?

This change is required to find the script or template tag defined in an amp-list.

Example

<amp-list> <script id="amp-template-id" type="text/plain" template="amp-mustache"> ... </script> </amp-list>

Note that it will return the first match found. We can make the validator rule changes enforce that only 1 template type is defined so that we can't do the following.

<amp-list> <script type="text/plain" template="amp-mustache"> ... </script> <template type="amp-mustache"> ... </template> </amp-list>

Gotcha, missed the , script part.

alabiaga

thanks Will!

extensions/amp-mustache/0.1/amp-mustache.js

alabiaga · 2018-11-30T15:30:02Z

extensions/amp-mustache/0.2/amp-mustache.js

+   *
+   * @param {!Element} content
+   */
+  processNestedScriptTemplates_(content) {


removed. Thanks, initially was going to try and add support for nested script templates but deferring it as it's not a priority and I filed a ticket for it as mentioned in another review comment.

alabiaga · 2018-11-30T15:32:11Z

src/service/template-impl.js

+    const templateTagName = templateElement.tagName;
+    user().assert(
+        templateTagName == 'TEMPLATE' || templateTagName == 'SCRIPT',
+        'Template must be a "template" tag or defined in a "script" tag %s',


alabiaga · 2018-11-30T15:36:24Z

src/service/template-impl.js

+          'Script template attribute must be "text/plain"');
+      user().assert(
+          templateElement.getAttribute('template') == 'amp-mustache',
+          'Script template attribute must be "amp-mustache"');


right. removed. In the future we can just add support for it when other template types are supported and just default to amp-mustache if omitted perhaps.

alabiaga · 2018-11-30T15:58:37Z

src/service/template-impl.js

@@ -285,7 +296,7 @@ export class Templates {
    } else if (opt_querySelector) {
      return scopedQuerySelector(parent, opt_querySelector);
    } else {
-      return childElementByTag(parent, 'template');
+      return parent.querySelector('template, script');


This change is required to find the script or template tag defined in an amp-list.

Example

<amp-list> <script id="amp-template-id" type="text/plain" template="amp-mustache"> ... </script> </amp-list>

Note that it will return the first match found. We can make the validator rule changes enforce that only 1 template type is defined so that we can't do the following.

<amp-list> <script type="text/plain" template="amp-mustache"> ... </script> <template type="amp-mustache"> ... </template> </amp-list>

alabiaga · 2018-11-30T16:00:38Z

testing/describes.js

+ * @param {!Array} values
+ * @param {func} func
+ */
+export function testCases(values, func) {


I did not. I was looking and briefly mentioned this in a standup and was under the impression that it didn't exist. Changed! Thanks.

alabiaga · 2018-11-30T16:42:09Z

src/service/template-impl.js

+    if (templateTagName == 'SCRIPT') {
+      user().assert(
+          templateElement.getAttribute('type') == 'text/plain',
+          'Script template attribute must be "text/plain"');


done. tagName in DOM trees always return in uppercase. https://developer.mozilla.org/en-US/docs/Web/API/Element/tagName

alabiaga · 2018-11-30T16:46:12Z

examples/amp-list.amp.html

@@ -57,6 +57,24 @@
    </template>
  </amp-list>

+  <!-- ## Using a script to enclose the template. -->
+  <amp-list width="auto" height="100" layout="fixed-height" src="comments.example.json">
+    <script type="amp-mustache">


yes, I did test this but this was before the whole discussion about the changes to these attributes so yes this stopped working with the recent changes. Thanks.

made test changes.

jridgewell

Overall looks good to me. Please wait for @choumx to review.

jridgewell · 2018-12-01T00:29:25Z

src/service/template-impl.js

+        (templateTagName == 'TEMPLATE' || (templateTagName == 'SCRIPT'
+            && templateElement.getAttribute('type') === 'text/plain')),
+        'Template must be defined in a <template> or '
+        + '<script type="text/plain"> tag');


Maybe text/html?

Considered this but technically mustache templates aren't HTML, which is why table foster parenting semantics won't affect this etc.

dreamofabear

Please file an issue to add an integration test for amp-list for the new script case.

dreamofabear · 2018-12-03T15:29:10Z

extensions/amp-mustache/0.1/test/test-amp-mustache.js

-describe('amp-mustache 0.1', () => {
+describes.repeated('amp-mustache 0.1', {
+  'template script': {templateType: 'script'},
+  'template template': {templateType: 'template'},


'template template' is confusing.

'with script[type=text/plain][template=amp-mustache]' 'with template[type=amp-mustache]'

dreamofabear · 2018-12-03T15:29:56Z

extensions/amp-mustache/0.1/test/test-amp-mustache.js

+                'text before a template  text after a template');
+            done();
+          } else {
+            done();


Passing and calling done shouldn't be necessary here. Also, extract the isTemplateType conditional:

if (isTemplateType) { describe('with template[type=amp-mustache]') { it(...); it(...); } }

the test would hang otherwise on the the condition script[type=text/plain] [template=amp-mustache]. But extracting the condition outside and not running it at all does remove the need for done(), if that's you are saying. Thanks

dreamofabear · 2018-12-03T15:30:42Z

extensions/amp-mustache/0.1/test/test-amp-mustache.js

+        done();
+      }
+    });
+  });


Ditto, except do the same for the script case.

dreamofabear · 2018-12-03T15:31:30Z

extensions/amp-mustache/0.1/test/test-amp-mustache.js

+                '</tr>' +
+              '{{/replies}}' +
+              '</tbody>' +
+            '</table>');


Can we use a more minimal example than this? It's also worth running the same example on the template case to highlight the difference.

done. I removed some table data to simplify the example.

dreamofabear · 2018-12-03T15:31:59Z

src/service/template-impl.js

@@ -285,7 +296,7 @@ export class Templates {
    } else if (opt_querySelector) {
      return scopedQuerySelector(parent, opt_querySelector);
    } else {
-      return childElementByTag(parent, 'template');
+      return parent.querySelector('template, script');


Gotcha, missed the , script part.

alabiaga · 2018-12-03T16:43:06Z

integration test issue here #19583

extensions/amp-mustache/0.2/amp-mustache.js

dreamofabear · 2018-12-03T20:22:21Z

extensions/amp-mustache/0.1/test/test-amp-mustache.js

+    });
+    if (isTemplateTypeScript) {
+      it('should not foster text nodes in script template', () => {
+        allowConsoleError(() => {


You need to return this otherwise Mocha won't execute the embedded code.

dreamofabear · 2018-12-03T20:22:26Z

extensions/amp-mustache/0.1/test/test-amp-mustache.js

+    if (isTemplateTypeScript) {
+      it('should foster text nodes in template[type="amp-mustache"]'
+          + 'destroying the templating', () => {
+        allowConsoleError(() => {


dreamofabear

LGTM except let's double check that one test.

dreamofabear · 2018-12-05T17:19:12Z

extensions/amp-mustache/0.1/test/test-amp-mustache.js

+        });
+      });
+    }
+    if (isTemplateTypeScript) {


This should be isTemplateType right?

dreamofabear · 2018-12-05T17:19:35Z

extensions/amp-mustache/0.1/test/test-amp-mustache.js

+      });
+    }
+    if (isTemplateTypeScript) {
+      it('should foster text nodes in template[type="amp-mustache"]'


Nit: Missing trailing space.

dreamofabear · 2018-12-05T17:20:23Z

extensions/amp-mustache/0.1/test/test-amp-mustache.js

@@ -19,40 +19,76 @@ import * as service from '../../../../src/service';
 import {AmpMustache} from '../amp-mustache';
 import mustache from '../../../../third_party/mustache/mustache';

-describe('amp-mustache 0.1', () => {
+describes.repeated('amp-mustache 0.1', {
+  'with script[type=text/plain][template=amp-mustache':


Nit: Missing trailing ].

dreamofabear · 2018-12-05T17:20:48Z

extensions/amp-list/0.1/test/test-amp-list.js

-    sandbox.stub(Services, 'templatesFor').returns(templates);
+describes.repeated('amp-list', {
+  'template type script': {templateType: 'script'},
+  'template type': {templateType: 'template'},


Nit: Use the same repeated test labels as test-amp-mustache.

* merge * add todo for nested script templates * fix test * address jridgewell comments * lint * address comments * fix merge * fix example * lint * address choumx comments * address choumx comments * address choumx comments

googlebot added the cla: yes label Nov 15, 2018

alabiaga changed the title ~~[WIP] Add script tag support for enclosing a template~~ Add script tag support for enclosing a template Nov 19, 2018

alabiaga requested review from dreamofabear and jridgewell November 19, 2018 21:33

jridgewell requested changes Nov 20, 2018

View reviewed changes

alabiaga commented Nov 20, 2018

View reviewed changes

jridgewell reviewed Nov 20, 2018

View reviewed changes

dreamofabear reviewed Nov 26, 2018

View reviewed changes

jridgewell mentioned this pull request Nov 27, 2018

Dynamic <amp-list> with <option><select> with required validation fails in IE11 due to amp-mustache braces being ignored #19337

Closed

dreamofabear reviewed Nov 29, 2018

View reviewed changes

alabiaga commented Nov 30, 2018

View reviewed changes

jridgewell approved these changes Dec 1, 2018

View reviewed changes

dreamofabear reviewed Dec 3, 2018

View reviewed changes

alabiaga mentioned this pull request Dec 3, 2018

Add amp-list integration test for script template type #19583

Closed

jridgewell approved these changes Dec 3, 2018

View reviewed changes

extensions/amp-mustache/0.2/amp-mustache.js Show resolved Hide resolved

dreamofabear requested changes Dec 3, 2018

View reviewed changes

dreamofabear approved these changes Dec 5, 2018

View reviewed changes

alabiaga added 9 commits December 7, 2018 14:16

merge

230603e

add todo for nested script templates

1c7b65d

fix test

20269ad

address jridgewell comments

6634410

lint

43c5350

address comments

c7b22ea

fix merge

dddeb1e

fix example

6555d34

lint

c61e471

alabiaga added 4 commits December 7, 2018 14:27

address choumx comments

da88a7f

address choumx comments

2b9ee18

address choumx comments

927cd81

Merge branch 'master' into 113521737

251bfe0

alabiaga merged commit ce617b7 into ampproject:master Dec 10, 2018

Add script tag support for enclosing a template #19346

Add script tag support for enclosing a template #19346

Conversation

alabiaga commented Nov 15, 2018 • edited

alabiaga commented Nov 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear Nov 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alabiaga Nov 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alabiaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alabiaga commented Nov 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alabiaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alabiaga commented Dec 3, 2018

Choose a reason for hiding this comment

alabiaga commented Nov 15, 2018 •

edited

dreamofabear Nov 26, 2018 •

edited

alabiaga Nov 20, 2018 •

edited