amp-script: Use updated method to get meta hashes by mdmower · Pull Request #26535 · ampproject/amphtml

mdmower · 2020-01-29T05:01:35Z

New getMetaByName method supports retrieving meta content values in
shadow docs as well.

- When .attachShadowDoc() is called, import <meta name="amp-script-src"> tags into the host document head - Amend amp-script to always look in win.doc.head (instead of ampdoc.getHeadNode) for amp-script-src meta tags. Note that there is no <head> in an AmpDocShadow instance and the only valid location for <meta> tags is in a <head> element. - When shadowDoc.close() is called, remove any <meta name="amp-script-src"> tags in the host document head

mdmower · 2020-01-29T16:27:37Z

extensions/amp-script/0.1/amp-script.js

+    // the specific type of ampdoc, the top document head must be referenced.
+    // For example, there is no <head> element in an AmpDocShadow instance.
+    const headNode = dev().assertElement(ampdoc.win.document.head);
+    const allowedHashes = headNode.querySelector('meta[name="amp-script-src"]');


Is this the best approach, or would it be better to specially handle just the AmpDocShadow case?
let headNode = ampdoc.getHeadNode(); if (headNode instanceof ShadowRoot) { headNode = ampdoc.win.document.head; }

There have been murmurings over time that maybe someday multiple AMP documents could be displayed at once in a PWA. Should we prepare for this possibility? I see a couple of options:

querySelectorAll('meta[name="amp-script-src"]')

When <meta name="amp-script-src" ...> is imported into <head>, if there already exists the same type of <meta> tag, concatenate their content values

@kevinkimball Can you take a look at this?

@mdmower there is support for multiple documents shown on one PWA today - that is the use case for the multidoc-manager.js file you edited, so I think we do need to support multiple meta tags to move forward

Ready for re-review. From my original review:

Opted to limit AmpScriptService changes to shadow docs

Opted for ii: combine multiple ampdoc meta tags into a single meta tag in owner doc

kevinkimball

Hi @mdmower , thanks for your contribution. This fix looks correct to me from an initial review. Can you please add some test coverage for your change? Thanks!

- Handle multiple shadow docs - Only revise handling of AmpDocShadow in amp-script

mdmower · 2020-02-03T05:00:28Z

There may still be room for improvement in the future w.r.t. multidoc handling, but this patchset is sufficient to bring amp-script support to PWAs, including after PWA navigation and when multiple docs are loaded. This minimal PWA can be dropped in the examples/ folder for testing:

dreamofabear

Thanks so much for identifying and fixing this bug!

dreamofabear · 2020-02-03T22:57:53Z

src/multidoc-manager.js

+                ' ' +
+                (n.getAttribute('content') || '')
+              ).trim();
+              el.setAttribute('content', content);


I'd prefer to keep the script hashes separate between documents (to avoid allowing a document to enable scripts on another). Could we instead just import these nodes and append them to the shadow root?

We do something similar for JSON scripts: https://github.com/ampproject/amphtml/pull/26535/files#diff-0a54674fbde60777f171c0027199f578L452

Then we can revert the amp-script.js changes.

/cc @dvoytenko

We could. I was avoiding insertion of meta tags into ShadowRoot since they should only appear under the <head> element. Do you have a preference for one of the following?

allow <meta> in ShadowRoot despite invalid HTML

import script hashes into a dedicated, invisible element in ShadowRoot

Fair point. We don't really need this to exist in the shadow DOM, so maybe we can add a metadata abstraction to the AmpDoc class instead.

I.e. similar to the existing AmpDoc.getParam(), add AmpDoc.getMeta(name) which returns the "content" attribute value for the meta tag with the corresponding name. AmpDocShadow would populate this in mergeShadowHead_, and AmpDocSingle would query its head node.

Revert "amp-script: Add shadow meta import/cleanup test" This reverts commit af2eefc. Revert "amp-script: Ensure head meta on import" This reverts commit d9ffc56. Revert "amp-script: More whitespace simplification" This reverts commit 6b48726. Revert "amp-script: Simplify meta import whitespace cleanup" This reverts commit 15be318. Revert "amp-script: Perform checkSha384 test in shadow" This reverts commit 44f68cd. Revert "amp-script: Revisions based on review" This reverts commit b12b167. Revert "amp-script: Import meta hashes in PWA" This reverts commit 7eb105c.

- New getMetaByName method supports retrieving meta content values in shadow docs as well.

mdmower · 2020-02-07T21:42:58Z

Ready for review.

samouri

I would wait for someone else's review as well before submitting, but these changes LGTM!

kevinkimball

LGTM

kevinkimball · 2020-02-10T18:04:54Z

extensions/amp-script/0.1/test/unit/test-amp-script.js

-  }
+        function createMetaHash(name, content) {
+          if (variant.ampdoc === 'shadow') {
+            env.ampdoc.setMetaByName(name, content);


nit, wouldn't this work for single doc also?

Thanks for the review. AmpDocSingle does not implement this method since the meta tags are expected to exist in <head>. .setMetaByName() is shadow-only for now.

Got it, thanks. I'll defer to the original discussion but it might be worth considering in the future to unify the API

dreamofabear · 2020-02-11T16:23:35Z

Looks great!

googlebot added the cla: yes label Jan 29, 2020

amp-owners-bot bot requested a review from jridgewell January 29, 2020 05:01

mdmower requested a review from dreamofabear January 29, 2020 05:02

kristoferbaxter requested a review from kevinkimball January 29, 2020 05:40

kristoferbaxter added Component: amp-script WG: performance labels Jan 29, 2020

mdmower commented Jan 29, 2020

View reviewed changes

kevinkimball suggested changes Jan 31, 2020

View reviewed changes

mdmower added 4 commits January 31, 2020 22:58

amp-script: Revisions based on review

b12b167

- Handle multiple shadow docs - Only revise handling of AmpDocShadow in amp-script

amp-script: Perform checkSha384 test in shadow

44f68cd

amp-script: Simplify meta import whitespace cleanup

15be318

amp-script: More whitespace simplification

6b48726

mdmower requested a review from kevinkimball February 1, 2020 07:28

mdmower added 2 commits February 1, 2020 08:28

amp-script: Ensure head meta on import

d9ffc56

amp-script: Add shadow meta import/cleanup test

af2eefc

dreamofabear reviewed Feb 3, 2020

View reviewed changes

mdmower mentioned this pull request Feb 4, 2020

ampdoc: Add get/setMetaByName methods #26609

Merged

mdmower added 3 commits February 6, 2020 12:40

Merge remote-tracking branch 'origin/master' into i26534

aa32060

amp-script: Use updated method to get meta hashes

73923a7

- New getMetaByName method supports retrieving meta content values in shadow docs as well.

mdmower changed the title ~~amp-script: Import meta hashes in PWA~~ amp-script: Use updated method to get meta hashes Feb 6, 2020

mdmower requested a review from dreamofabear February 7, 2020 21:41

samouri approved these changes Feb 7, 2020

View reviewed changes

mdmower mentioned this pull request Feb 9, 2020

Evaluate uses of meta.getAttribute('content') #26652

Closed

33 tasks

kevinkimball approved these changes Feb 10, 2020

View reviewed changes

mdmower merged commit c98e5f3 into ampproject:master Feb 11, 2020

mdmower deleted the i26534 branch February 11, 2020 06:02

ampprojectbot added the PR use: In Beta / Experimental label Feb 14, 2020

ampprojectbot added the PR use: In Stable label Feb 25, 2020

Conversation

mdmower commented Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

kevinkimball left a comment

Choose a reason for hiding this comment

Uh oh!

mdmower commented Feb 3, 2020

Uh oh!

dreamofabear 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!

Choose a reason for hiding this comment

Uh oh!

mdmower commented Feb 7, 2020

Uh oh!

samouri left a comment • 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!

kevinkimball left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdmower Feb 10, 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!

dreamofabear commented Feb 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mdmower commented Jan 29, 2020 •

edited

Loading

samouri left a comment •

edited

Loading

mdmower Feb 10, 2020 •

edited

Loading