Skip to content

amp-script: Support local scripts via "script" attribute#23043

Merged
dreamofabear merged 8 commits intoampproject:masterfrom
dreamofabear:inline-amp-script
Jun 28, 2019
Merged

amp-script: Support local scripts via "script" attribute#23043
dreamofabear merged 8 commits intoampproject:masterfrom
dreamofabear:inline-amp-script

Conversation

@dreamofabear
Copy link
Copy Markdown

@dreamofabear dreamofabear commented Jun 26, 2019

Fixes #22612.

Open to suggestions for an attribute name other than "script" which is a tad overloaded. Was following convention set by "template" attribute.

<amp-script script=myScript width=300 height=100>
</amp-script>

<script type=text/amp-script id=myScript>
</script>

Note these are only reference by id rather than inline (children) since I wanted to avoid the hydration filtering issue.

@dreamofabear dreamofabear changed the title amp-script: Support local scripts amp-script: Support local scripts via "script" attribute Jun 26, 2019
| </body>
>> ^~~~~~~~~
amp-script/0.1/test/validator-amp-script-not-origin-trial.html:34:6 The tag 'amp-experiment-token' is missing or incorrect, but required by 'amp-script'. (see https://amp.dev/documentation/components/amp-script) [AMP_TAG_PROBLEM]
amp-script/0.1/test/validator-amp-script-not-origin-trial.html:34:6 The tag 'amp-experiment-token' is missing or incorrect, but required by 'amp-script'. (see https://amp.dev/documentation/components/amp-script) [AMP_TAG_PROBLEM] No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional change?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this was auto-generated by gulp validator --update_tests. I think it's harmless.

1. Mutations are always accepted after a user action for a user action interval of 5 seconds.
2. The 5 seconds interval is extended if the user script performs a `fetch()` operation.
3. Smaller `amp-script` elements with height under `300px` and non-`container` layout are allowed unlimitted mutations.
3. Smaller `amp-script` elements with height under `300px` and non-`container` layout are allowed unlimited mutations.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch.

@dreamofabear dreamofabear merged commit 5ed34f1 into ampproject:master Jun 28, 2019
@dreamofabear dreamofabear deleted the inline-amp-script branch June 28, 2019 16:36
@twifkak
Copy link
Copy Markdown
Member

twifkak commented Jul 1, 2019

This affects how https://github.com/ampproject/amppackager#security-considerations are applied. I'm going to send a change to amppkg not to sign pages with inline amp-scripts, unless I hear a compelling alternative. I was hoping to have time to make a less rushed decision re: this.

Gregable pushed a commit that referenced this pull request Jul 2, 2019
* cl/255021843 Add a test file illustrating the issue in #18091

* cl/255463519 Revision bump for #22679

* cl/256058522 Revision bump for #23043

* cl/256067928 Revision bump for #23135

* cl/256199406 Revision bump for #23147
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[amp-script] Support inline JS

5 participants